-
Wichert Akkerman <wichert@...> writes: > On 6/29/09 4:49 PM, Ross Patterson wrote: >> Wichert Akkerman<wichert@...> writes: >> >>> I am just catching up on Ross's work last week, and I am not sure >>> about the changes in this changeset: >>> >>> http://dev.plone.org/old/collective/changeset/88642/Products.membrane> >> This changeset was proposed and discussed well in advance. It would >> have been vastly preferable to bring up debate *before* a contributor >> put the time in to implement. > > I was not aware of it. I looked a while ago and could not find a > membrane mailinglist anywhere, so I don't think I can be blamed for > that. Even today there is no mention at all in the membrane package > that the remember list is used for membrane development. On http://plone.org/products/membrane the "Support" link points to the remember list. I'll add an explicit mention into the package as well. >>> I see the problem Ross is trying to solve: generating a list of >>> everything an object can be adapted to is expensive (which is why the >>> same thing was removed from the Plone 3.x tree just before 3.0). >>> >>> This solution moves the penalty to the developer: instead of only >>> having to write an adapter developers are now forced to register both >>> an adapter and a new marker interface. I feel quite strongly that this >>> repetition is not desirable, so I want to investigate alternatives. >> >> If by "register" you mean the "<interface>" registration, the developer >> would not need to do that. All the developer would need to do is make >> sure the class implements the marker interface which can be done in the >> class level implements declaration or in ZCML. This is one of the most >> simple declarations possible. > > Sure, but my point is that this shoudl not be necessary. Why should we > force developers to both register a marker interface and an adapter? > That is the kind of extra repetition and extra work that we are now > working hard on removing from Plone. In this case we are trying to support the capacity to adapt content to a membrane interface as well as to allow the content to provide it directly. If we want to have it both ways, then I think this repetition is appropriate. >>> Plone offers a fairly rich system of catalog indexing helper routines >>> which we can leverage. Instead of iterating over the ZCA registrations >>> to build a list of interface an object implements or can be adapted to >>> we can do simpler things. For example we could have a feature catalog >>> that looks like this: >> >> With the current implementation, we don't iterate over the ZCA >> registrations, we lookup interfaces that have been registered as >> providing the IMembraneQueryableInterface. >> >>> @indexer(Interface, membrane_tool.IMembraneTool) >>> def features(obj): >>> _features = dict(user = IMembraneUserObject, >>> auth = IMembraneUserAuth, >>> props = IMebraneUserProperties) >>> >>> result=[] >>> for (ft, iface) in _features.items(): >>> a=iface(obj, None) >>> if a is not None: >>> result.append(ft) >>> return ft >>> >>> This should be fast enough for indexing purposes since we can use all >>> the fast paths and optimisations from the ZCA, and gives us much >>> simpler catalog data: we will no longer need to index all the >>> interfaces that are not relevant for membrane. There is an obvious >>> optimization here: if an object does not adapt to IMembraneUserObject >>> we can short-circuit and stop processing immediately. My main comment about this I forgot to include. This implementation still looks up the adapters which I believe was a significant part of the performance problem before. At any rate, it not only looks up the adapters, but actually calls the factory as well. Since the factory is arbitrary code, this is too large a performance *risk*. Ross
-
On 6/29/09 5:11 PM, Ross Patterson wrote: > Wichert Akkerman<wichert@...> >> Sure, but my point is that this shoudl not be necessary. Why should we >> force developers to both register a marker interface and an adapter? >> That is the kind of extra repetition and extra work that we are now >> working hard on removing from Plone. > > In this case we are trying to support the capacity to adapt content to a > membrane interface as well as to allow the content to provide it > directly. If we want to have it both ways, then I think this repetition > is appropriate. I don't see why. The previous approach managed to handle both cases without repetition, and both my suggestions don't need any form of repetition either, without loss of any functionality. >>>> Plone offers a fairly rich system of catalog indexing helper routines >>>> which we can leverage. Instead of iterating over the ZCA registrations >>>> to build a list of interface an object implements or can be adapted to >>>> we can do simpler things. For example we could have a feature catalog >>>> that looks like this: >>> With the current implementation, we don't iterate over the ZCA >>> registrations, we lookup interfaces that have been registered as >>> providing the IMembraneQueryableInterface. >>> >>>> @indexer(Interface, membrane_tool.IMembraneTool) >>>> def features(obj): >>>> _features = dict(user = IMembraneUserObject, >>>> auth = IMembraneUserAuth, >>>> props = IMebraneUserProperties) >>>> >>>> result=[] >>>> for (ft, iface) in _features.items(): >>>> a=iface(obj, None) >>>> if a is not None: >>>> result.append(ft) >>>> return ft >>>> >>>> This should be fast enough for indexing purposes since we can use all >>>> the fast paths and optimisations from the ZCA, and gives us much >>>> simpler catalog data: we will no longer need to index all the >>>> interfaces that are not relevant for membrane. There is an obvious >>>> optimization here: if an object does not adapt to IMembraneUserObject >>>> we can short-circuit and stop processing immediately. > > My main comment about this I forgot to include. This implementation > still looks up the adapters which I believe was a significant part of > the performance problem before. At any rate, it not only looks up the > adapters, but actually calls the factory as well. Since the factory is > arbitrary code, this is too large a performance *risk*. The old code was incredibly painful since it essentially did a slow iteration over all adapter registrations and manually tested every adapter to see if it applies to the current object. It's a bit similar to doing a getAdapter for every possible adapter. This version does between 1 and 4 adapter calls, each of which should be quite fast since we can use the normal ZCA infrastructure. If that is slow than we have much much bigger problems, since Plone uses dozens if not hundreds of adapter calls for every request. Have you ever seen an adapter constructor that did more than set a few instance variables to the parameters passed in? I don't consider that risk to be realistic, or very problematic since it is trivially worked around. I think our opinions are pretty firmly rooted and not likely to change. I'ld like to hear some other opinions on this as well! Wichert.
-
Wichert Akkerman wrote: > I think our opinions are pretty firmly rooted and not likely to change. > I'ld like to hear some other opinions on this as well! i agree w/ you, wichert; i'm not super fond of forcing the developers to use yet more marker interfaces in order to keep using membrane. but i'm finding some amusing irony in this conversation, b/c the idea of using the I*able marker interfaces was originally MY idea, and it was actually ross who talked me out of it. here's my recollection of what happened, based on memory and my digging through my email archives: i originally implemented the "index-what-the-objects-can-be-adapted-to" version of the object_implements index back when i did the original membrane refactoring at a snow sprint over 3 years ago. it was a clever idea, but it was naive and horribly inefficient, and had a major detrimental impact on membrane's performance. originally the index made use of the apidoc API; helge tesdal was kind enough to reimplement the index, making it much much faster, at the cost of using internal ZCA APIs that we probably shouldn't actually be using. b/c the index is still very bloated, however, and b/c it relies on private APIs, i still felt like something should change. i proposed the marker interface solution that ross implemented in a private email discussion that we had when i originally handed over the membrane / remember maintainership. this is probably why ross mistakenly thought the conversation had happened on the mailing list... i haven't dug through the archives, but i don't think this conversation ever made it to the public list. anyway, in that conversation, ross made a suggestion that i thought was quite clever, which removed the bloat from the index and just generally made everything much simpler, so much so that my objection to the index evaporated. it's similar in spirit to what you're proposing, wichert, but different in details. hopefully ross won't mind that i include it here: <ROSS> A quick audit of membrane and remember indicates that the following interfaces are being queried on: IMembraneUserAuth, IUserAuthProvider, IMembraneUserGroups, IGroup, IMembraneUserRoles, IMembraneUserChanger, IMembraneUserDeleter, IMembraneUserProperties, and IReferenceable. Are all of these queries likely to be valid and necessary? Whatever interfaces we do need to query on, maybe we can use zope.component.interface.provideInterface to register all those interfaces as utilities providing IMembraneQueryableInterface: zope.component.interface.provideInterface( '', IMembraneUserAuth, IMembraneQueryableInterface) zope.component.interface.provideInterface( '', IGroup, IMembraneQueryableInterface) alternatively, if ZCML doesn't make you puke: <interface interface="Products.membrane.interfaces.IMembraneUserAuth" type="Products.membrane.interfaces.IMembraneQueryableInterface" /> <interface interface="Products.membrane.interfaces.IGroup" type="Products.membrane.interfaces.IMembraneQueryableInterface" /> Then we can convert the object_implements index to a more focused one: def object_implements(object, portal, **kw): return tuple( iface.__identifier__ for iface in component.getUtilitiesFor( IMembraneQueryableInterface, context=portal) if iface.providedBy(object)) This means we don't have to add a gazillion boolean indexes for each interface we want to look for providers of. It's easily extensible using zope.component.interface.provideInterface or <interface>. It's explicitly limited to the interfaces we know we need query on. The code using the index won't have to be changed. Would this approach address the problems? </ROSS> i don't feel strongly re: ross's version vs. wichert's version, but i do think that it's a better idea to rework the index so that it's focused only on the membrane-specific interfaces, rather than forcing the developer to juggle yet more marker interfaces. -r-
On 6/29/09 9:27 PM, Rob Miller wrote: > Wichert Akkerman wrote: >> I think our opinions are pretty firmly rooted and not likely to >> change. I'ld like to hear some other opinions on this as well! > > i agree w/ you, wichert; i'm not super fond of forcing the developers to > use yet more marker interfaces in order to keep using membrane. but i'm > finding some amusing irony in this conversation, b/c the idea of using > the I*able marker interfaces was originally MY idea, and it was actually > ross who talked me out of it. So when the to of you swapped maintainer and developer roles you swapped opinion as well? :) > anyway, in that conversation, ross made a suggestion that i thought was > quite clever, which removed the bloat from the index and just generally > made everything much simpler, so much so that my objection to the index > evaporated. it's similar in spirit to what you're proposing, wichert, > but different in details. hopefully ross won't mind that i include it here: > > <ROSS> > A quick audit of membrane and remember indicates that the following > interfaces are being queried on: IMembraneUserAuth, IUserAuthProvider, > IMembraneUserGroups, IGroup, IMembraneUserRoles, IMembraneUserChanger, > IMembraneUserDeleter, IMembraneUserProperties, and IReferenceable. Are > all of these queries likely to be valid and necessary? > > Whatever interfaces we do need to query on, maybe we can use > zope.component.interface.provideInterface to register all those > interfaces as utilities providing IMembraneQueryableInterface: > > zope.component.interface.provideInterface( > '', IMembraneUserAuth, IMembraneQueryableInterface) > zope.component.interface.provideInterface( > '', IGroup, IMembraneQueryableInterface) > > alternatively, if ZCML doesn't make you puke: > > <interface > interface="Products.membrane.interfaces.IMembraneUserAuth" > type="Products.membrane.interfaces.IMembraneQueryableInterface" /> > <interface > interface="Products.membrane.interfaces.IGroup" > type="Products.membrane.interfaces.IMembraneQueryableInterface" /> > > Then we can convert the object_implements index to a more focused one: > > def object_implements(object, portal, **kw): > return tuple( > iface.__identifier__ for iface in > component.getUtilitiesFor( > IMembraneQueryableInterface, context=portal) > if iface.providedBy(object)) That assumes the object provides that interface directly. How easily can that be changed to handle adaptibility as well? This is ZCA magic I haven't encountered before so I'm not sure how exactly that would work. It's a bit more magic than my proposal, but I do like the easy extensibility. I definitely prefer this above the current I*Avail approach. I suppose that means that Ross should have been more stubborn :) Wichert.
-
Wichert Akkerman wrote: > On 6/29/09 9:27 PM, Rob Miller wrote: >> Wichert Akkerman wrote: >>> I think our opinions are pretty firmly rooted and not likely to >>> change. I'ld like to hear some other opinions on this as well! >> >> i agree w/ you, wichert; i'm not super fond of forcing the developers to >> use yet more marker interfaces in order to keep using membrane. but i'm >> finding some amusing irony in this conversation, b/c the idea of using >> the I*able marker interfaces was originally MY idea, and it was actually >> ross who talked me out of it. > > So when the to of you swapped maintainer and developer roles you swapped > opinion as well? :) yup. we're switching cars later this month, and are working on the paperwork to trade houses. another year or so and we should have completed the transition. >> anyway, in that conversation, ross made a suggestion that i thought was >> quite clever, which removed the bloat from the index and just generally >> made everything much simpler, so much so that my objection to the index >> evaporated. it's similar in spirit to what you're proposing, wichert, >> but different in details. hopefully ross won't mind that i include it >> here: >> >> <ROSS> >> A quick audit of membrane and remember indicates that the following >> interfaces are being queried on: IMembraneUserAuth, IUserAuthProvider, >> IMembraneUserGroups, IGroup, IMembraneUserRoles, IMembraneUserChanger, >> IMembraneUserDeleter, IMembraneUserProperties, and IReferenceable. Are >> all of these queries likely to be valid and necessary? >> >> Whatever interfaces we do need to query on, maybe we can use >> zope.component.interface.provideInterface to register all those >> interfaces as utilities providing IMembraneQueryableInterface: >> >> zope.component.interface.provideInterface( >> '', IMembraneUserAuth, IMembraneQueryableInterface) >> zope.component.interface.provideInterface( >> '', IGroup, IMembraneQueryableInterface) >> >> alternatively, if ZCML doesn't make you puke: >> >> <interface >> interface="Products.membrane.interfaces.IMembraneUserAuth" >> type="Products.membrane.interfaces.IMembraneQueryableInterface" /> >> <interface >> interface="Products.membrane.interfaces.IGroup" >> type="Products.membrane.interfaces.IMembraneQueryableInterface" /> >> >> Then we can convert the object_implements index to a more focused one: >> >> def object_implements(object, portal, **kw): >> return tuple( >> iface.__identifier__ for iface in >> component.getUtilitiesFor( >> IMembraneQueryableInterface, context=portal) >> if iface.providedBy(object)) > > That assumes the object provides that interface directly. ah, right, i noticed that originally as well, and commented on it in the original thread from which i pulled that quote. > How easily can > that be changed to handle adaptibility as well? this is all untested, but that seems at first glance a trivial change: def object_implements(object, portal, **kw): return tuple( iface.__identifier__ for iface in component.getUtilitiesFor(IMembraneQueryableInterface, context=portal) if iface(obj, None) is not None) > This is ZCA magic I > haven't encountered before so I'm not sure how exactly that would work. conceptually this is almost exactly what you suggested. the only thing that's different is that you hardcoded a set of "features", by mapping feature names to interfaces, where ross uses the ZCA to "mark" certain interfaces rather than hardcoding. > It's a bit more magic than my proposal, but I do like the easy > extensibility. i don't think there's a lot of magic here, really. it seems to me a typical to-ZCA-or-not-to-ZCA choice; one is more clear, b/c the set of pertinent membrane interfaces is written out in code right in front of you. the other is more extensible, b/c you can register any interface as a membrane interface in the config. > I definitely prefer this above the current I*Avail > approach. I suppose that means that Ross should have been more stubborn :) indeed. or i should've. hard to tell at this point... ;) -r
-
-
-