Bug #72829 for Class-XSAccessor: Cached accessors

Wed Nov 30 02:43:16 2011 ribasushi [...] leporine.io - Ticket created

Subject:

Cached accessors - minor problem

I just saw 1.12_01 being released - the cached accessor stuff is an awesome awesome idea. It in fact obviates the need for me to deal with the CAG inherited type we were talking about (more detail about implementation here: https://metacpan.org/source/RIBASUSHI/Class-Accessor-Grouped-0.10004/lib/Class/Accessor/Grouped.pm#L237) However there is a problem - the method names. First of all they are short and common enough to virtually guarantee clashes with existing software. For example I can not add a _set method to the DBIC ResultClass inheritance chain, as there is an extremely high chance an actual user of said library already has such methods. I propose something like _xsa_cached_get / _xsa_cached_set. Furthermore the way you implemented this it will be quite complicated to retrofit it into larger frameworks like Moo or Class::Accessor::Grouped, as we now need to implement a flexible accessor dispatch table inside the _set/_get subs (i.e. what to do on 'foo', what to do on 'bar', etc). Again CAG is a good example on why this is a problem: https://metacpan.org/source/RIBASUSHI/Class-Accessor-Grouped-0.10004/lib/Class/Accessor/Grouped.pm#L100. We currently only use XSA for the 'simple' type (and go to hreat lengths to make sure we do not short-circuit a user-defines get/set_simple, e.g. https://metacpan.org/source/RIBASUSHI/Class-Accessor-Grouped-0.10004/lib/Class/Accessor/Grouped.pm#L654) Is it possible to compile the accessor name into the method as well? Something like _xsa_cached_get_foo / _xsa_cached_set_foo? Cheers!

Wed Nov 30 14:25:27 2011 rt-cpan [...] trout.me.uk - Correspondence added

On Wed Nov 30 02:43:16 2011, RIBASUSHI wrote: Show quoted text

> Furthermore the way you implemented this it will be quite complicated > to > retrofit it into larger frameworks like Moo or > Class::Accessor::Grouped, > as we now need to implement a flexible accessor dispatch table inside > the _set/_get subs (i.e. what to do on 'foo', what to do on 'bar', > etc). > Again CAG is a good example on why this is a problem: > https://metacpan.org/source/RIBASUSHI/Class-Accessor-Grouped- > 0.10004/lib/Class/Accessor/Grouped.pm#L100. > We currently only use XSA for the 'simple' type (and go to hreat > lengths > to make sure we do not short-circuit a user-defines get/set_simple, > e.g. > https://metacpan.org/source/RIBASUSHI/Class-Accessor-Grouped- > 0.10004/lib/Class/Accessor/Grouped.pm#L654) > Is it possible to compile the accessor name into the method as well? > Something like _xsa_cached_get_foo / _xsa_cached_set_foo?

It seems to me that what I really want is to be able to pass a method name (and, if possible, have a choice of passing a coderef as well) plus some static arguments to be passed thereto - and then you're effectively defaulting the names to _get/_set and the arguments to $attr_name. The other thing that I'd really like to have here is to be able to have -only- the _get or _set behaviour on any given accessor - for example I could use _set only to implement set-with-type-checks or _get only to implement lazyily-calculated-defaults.

Wed Nov 30 14:25:28 2011 The RT System itself - Status changed from 'new' to 'open'

Thu Dec 01 02:06:45 2011 smueller [...] cpan.org - Correspondence added

Subject:	Re: [rt.cpan.org #72829] Cached accessors - minor problem
Date:	Thu, 01 Dec 2011 08:06:29 +0100
To:	bug-Class-XSAccessor [...] rt.cpan.org
From:	Steffen Mueller <smueller [...] cpan.org>

On 11/30/2011 08:25 PM, MSTROUT via RT wrote: Show quoted text

> Queue: Class-XSAccessor > Ticket<URL: https://rt.cpan.org/Ticket/Display.html?id=72829> > > On Wed Nov 30 02:43:16 2011, RIBASUSHI wrote:

>> Furthermore the way you implemented this it will be quite complicated >> to >> retrofit it into larger frameworks like Moo or >> Class::Accessor::Grouped, >> as we now need to implement a flexible accessor dispatch table inside >> the _set/_get subs (i.e. what to do on 'foo', what to do on 'bar', >> etc). >> Again CAG is a good example on why this is a problem: >> https://metacpan.org/source/RIBASUSHI/Class-Accessor-Grouped- >> 0.10004/lib/Class/Accessor/Grouped.pm#L100. >> We currently only use XSA for the 'simple' type (and go to hreat >> lengths >> to make sure we do not short-circuit a user-defines get/set_simple, >> e.g. >> https://metacpan.org/source/RIBASUSHI/Class-Accessor-Grouped- >> 0.10004/lib/Class/Accessor/Grouped.pm#L654) >> Is it possible to compile the accessor name into the method as well? >> Something like _xsa_cached_get_foo / _xsa_cached_set_foo?

> > It seems to me that what I really want is to be able to pass a method > name (and, if possible, have a choice of passing a coderef as well) plus > some static arguments to be passed thereto - and then you're effectively > defaulting the names to _get/_set and the arguments to $attr_name. > > The other thing that I'd really like to have here is to be able to have > -only- the _get or _set behaviour on any given accessor - for example I > could use _set only to implement set-with-type-checks or _get only to > implement lazyily-calculated-defaults.

This reply applies to both of you, not just Matt. I didn't do any of this because that requires hacking on the C-level storage for the hash keys. Right now, it sports exactly 1-to-1 mapping of hash key strings to integers with lookup either way. Shoving more info in there without a) Slowing down the simple cases (possibly by quite a bit) b) Breaking the terrible and likely fragile code that implements the out-of-perl storage. c) Breaking it specifically wrt. thread-safety. Now, all of these may be irrational fear, so don't let me discourage you from hacking on it (and cleaning it up as you see fit, but please be available for debugging weird issues if you break it). For me, however, I simply needed exactly the behaviour of the current implementation of cached accessors for $work (we have a weirdly twisted variant of a formerly-Class::DBI ORM that we can't migrate away from and don't generally give a flying fuck about anyway). What you propose is a superset of what I need, so that'll work for me, too. Just be aware that a) I don't have the time to implement it and b) any call back into Perl from C is going to be so mind-numbingly slow that you could have done the whole thing in Perl from the get-go. So make sure the common case doesn't have to do that. Cheers, Steffen

Thu Dec 01 03:23:47 2011 smueller [...] cpan.org - Correspondence added

Subject:	Re: [rt.cpan.org #72829] Cached accessors - minor problem
Date:	Thu, 01 Dec 2011 09:23:27 +0100
To:	bug-Class-XSAccessor [...] rt.cpan.org
From:	Steffen Mueller <smueller [...] cpan.org>

On 11/30/2011 08:43 AM, Peter Rabbitson via RT wrote: Show quoted text

> However there is a problem - the method names. First of all they are > short and common enough to virtually guarantee clashes with existing > software. For example I can not add a _set method to the DBIC > ResultClass inheritance chain, as there is an extremely high chance an > actual user of said library already has such methods. I propose > something like _xsa_cached_get / _xsa_cached_set.

That would be easy and I'd be happy to do that. Show quoted text

> Furthermore the way you implemented this it will be quite complicated to > retrofit it into larger frameworks like Moo or Class::Accessor::Grouped,

Large framework like Moo? IMO, that's a failure right there. No offense, Matt! Show quoted text

> as we now need to implement a flexible accessor dispatch table inside > the _set/_get subs (i.e. what to do on 'foo', what to do on 'bar', etc). > Again CAG is a good example on why this is a problem: > https://metacpan.org/source/RIBASUSHI/Class-Accessor-Grouped-0.10004/lib/Class/Accessor/Grouped.pm#L100. > We currently only use XSA for the 'simple' type (and go to hreat lengths > to make sure we do not short-circuit a user-defines get/set_simple, e.g. > https://metacpan.org/source/RIBASUSHI/Class-Accessor-Grouped-0.10004/lib/Class/Accessor/Grouped.pm#L654)

Show quoted text

> Is it possible to compile the accessor name into the method as well? > Something like _xsa_cached_get_foo / _xsa_cached_set_foo?

Not easily/efficiently. --Steffen

Tue Dec 06 10:37:44 2011 smueller [...] cpan.org - Correspondence added

On Wed Nov 30 14:25:27 2011, MSTROUT wrote: Show quoted text

> On Wed Nov 30 02:43:16 2011, RIBASUSHI wrote:

> > Furthermore the way you implemented this it will be quite

complicated Show quoted text

> > to > > retrofit it into larger frameworks like Moo or > > Class::Accessor::Grouped, > > as we now need to implement a flexible accessor dispatch table

inside Show quoted text

> > the _set/_get subs (i.e. what to do on 'foo', what to do on 'bar', > > etc). > > Again CAG is a good example on why this is a problem: > > https://metacpan.org/source/RIBASUSHI/Class-Accessor-Grouped- > > 0.10004/lib/Class/Accessor/Grouped.pm#L100. > > We currently only use XSA for the 'simple' type (and go to hreat > > lengths > > to make sure we do not short-circuit a user-defines get/set_simple, > > e.g. > > https://metacpan.org/source/RIBASUSHI/Class-Accessor-Grouped- > > 0.10004/lib/Class/Accessor/Grouped.pm#L654) > > Is it possible to compile the accessor name into the method as well? > > Something like _xsa_cached_get_foo / _xsa_cached_set_foo?

> > It seems to me that what I really want is to be able to pass a method > name (and, if possible, have a choice of passing a coderef as well)

plus Show quoted text

> some static arguments to be passed thereto - and then you're

effectively Show quoted text

> defaulting the names to _get/_set and the arguments to $attr_name. > > The other thing that I'd really like to have here is to be able to

have Show quoted text

> -only- the _get or _set behaviour on any given accessor - for example

I Show quoted text

> could use _set only to implement set-with-type-checks or _get only to > implement lazyily-calculated-defaults.

I thought some more about the general problem of adding more meta- information to the generated accessors. It's a bit nasty. Since that's thinking I've done before, I added some comments to the code for the Steffen of the future to read instead of redoing the whole thought thing: https://github.com/tsee/Class- XSAccessor/commit/5590225024cb52adf187b45113544ff36a5516fa Hope that makes some sense. So if we want this further parameterization, we'll have to reconsider more of the implementation and algorithms in the module. BTW: The thread-safety issue that I discovered the other day is fixed in the master branch now. Maybe this will allow us to recover cygwin! Yay! On further reflection, it should be possible to include the hash key name into the _get/_set-alike method to call without too much pain. But that would be most elegantly solved by further parametization to avoid some of the ludicrous code duplication that's already going on. So I'm punting on it for the time being. Cheers, Steffen

Sun Nov 04 15:13:08 2012 ribasushi [...] leporine.io - Status changed from 'open' to 'stalled'

Sun Nov 04 15:13:08 2012 ribasushi [...] leporine.io - Severity Wishlist added

Bug #72829 for Class-XSAccessor: Cached accessors - minor problem