Community
Participate
Working Groups
Hi Ed, According to our recent discussion I took the time and played around a bit. Now I'd like to come up with a proposal (projects to be attached later). Approach: Each getter will get a prologue: /** * @generated */ public String getName() { if (ePreNotificationRequired()) { eClass().getEPackage().preNotifyReadAccess(this); } return name; } And each setter: /** * @generated */ public void setName(String newName) { if (ePreNotificationRequired()) { eClass().getEPackage().preNotifyWriteAccess(this); } String oldName = name; name = newName; if (eNotificationRequired()) { eNotify(new ENotificationImpl(this, Notification.SET, HookPackage.HOOK__NAME, oldName, name)); } } The ePreNotificationRequired() method is implemented in EObjectImpl: public static final int PRE_NOTIFICATION_FLAG = ELAST_NOTIFIER_FLAG << 1; public boolean ePreNotificationRequired() { if ((eFlags & PRE_NOTIFICATION_FLAG) == 0) { return false; } return eClass().getEPackage().isPreNotificationRequired(); } public void eSetPreNotificationRequired(boolean on) { if (on) { eFlags |= PRE_NOTIFICATION_FLAG; } else { eFlags &= ~PRE_NOTIFICATION_FLAG; } } Please note that ePreNotificationRequired() does a two-stage check: 1) eFlags 2) eClass().getEPackage() This greatly optimizes the default case!! In the second check particularly "eClass().getEPackage()" takes much more time. Finally EPackageImpl has this: private transient List<PreNotificationListener> preNotificationListeners = new ArrayList<PreNotificationListener>(); private transient volatile boolean preNotificationRequired; public void addPreNotificationListener(PreNotificationListener listener) { synchronized (preNotificationListeners) { if (!preNotificationListeners.contains(listener)) { preNotificationListeners.add(listener); preNotificationRequired = true; } } } public void removePreNotificationListener(PreNotificationListener listener) { synchronized (preNotificationListeners) { if (preNotificationListeners.contains(listener)) { if (preNotificationListeners.remove(listener)) { if (preNotificationListeners.isEmpty()) { preNotificationRequired = false; } } } } } public PreNotificationListener[] getPreNotificationListeners() { synchronized (preNotificationListeners) { return preNotificationListeners.toArray(new PreNotificationListener[preNotificationListeners.size()]); } } public boolean isPreNotificationRequired() { return preNotificationRequired; } public void preNotifyReadAccess(EObject object) { for (PreNotificationListener listener : getPreNotificationListeners()) { // Propagate possible exceptions here: listener.handleReadAccess(object); } } public void preNotifyWriteAccess(EObject object) { for (PreNotificationListener listener : getPreNotificationListeners()) { // Propagate possible exceptions here: listener.handleWriteAccess(object); } } I tested/measured it with the following scenario: - Create 1000000 objects - Call a setter on each object (twice) -> dump duration - Call a getter on each object (twice) -> dump duration I tested four different configurations: 1) normal generated code, i.e. no prologues generated Millis for 2000000 writes: 48 Millis for 2000000 reads: 24 2) with the aforementioned prologues generated 2.1) PRE_NOTIFICATION_FLAG == false Millis for 2000000 writes: 48 Millis for 2000000 reads: 28 2.2) PRE_NOTIFICATION_FLAG == true 2.2.1) no listeners registered with the package Millis for 2000000 writes: 90 Millis for 2000000 reads: 67 2.2.2) one simple listener registered with the package (NOOP) Millis for 2000000 writes: 638 Millis for 2000000 reads: 560 I think the most important result is the comparison of 1) and 2.1)! I leave it up to you to judge but I feel that the difference is so minimal that we should dare it. I hope that you don't have more general concerns (aka poisoned fountain) ;P Open points: - More info about the structural feature and the new value in case of write access? - Lists are not handled correctly (I thought for a performance proof normal accessors are ok) - Possible reflective access is not considered (implicitely covered?) - Not too much care has been taken to properly set visibility of new API/impl - The naming is a bit strange ...
Created attachment 112393 [details] the test project
Created attachment 112394 [details] changes to ecore
+1!
Update: - We want to reuse the existing eAdapters list (instead of going through a singleton list in EPackage) - PreNotifications will not be chained - The existing Notification interface will *not* be used - Therefore we will not call this new feature "PreNotification" anymore to avoid confusion. - I propose the names PreReadCallback and PreWriteCallback (to indicate that they don't receive Notification instances as messages) - These new interfaces will extend Adapter.Internal - No event-like object will be created but just methods called (perf) - Both callback types (PreRead and PreWrite) will be passed only the EObject (Notifier subtype) and the eStructuralFeature that causes the pre-access callback. - There will be two new eFlags to optimze the legacy case (i.e. eAdapters w/o any callbacks) - There will be one new eFlag to optimze the existing post-notifications (i.e. eAdapters w/o any ordinary adapter). This will optimize existing code and has the potential to make the new code (with this feature) even faster than without! - There will be only one eFlag to switch *all* delivery on/off (the existing flag) - Most of the callback logic for lists will be implemented in NotifyingListImpl. Since this class is not aware of an owning EObject all the new functionality will be disabled through protected methods that have to be overridden by EcoreEList which is the highest-level notifying list with a sense for an owning EObject. - The JET templates will be slightly changed: the pre-access callback calls are injected at the beginning of every single-valued getter and every setter. - The generator model receives two new model-level properties: "Suppress Pre Read Callbacks" and "Suppress Pre Write Callbacks". - The default for these generator properties will be *false* since we do not foresee major impact! I'll start now with a new patch. I'm open for comments ;-)
Update: - The new callback interfaces won't extend Adapter.Internal but only Adapter
Created attachment 112490 [details] changes to emf Open issues: - Make the value of the new genmodel queriable at runtime (EPackage.isPreReadCallbackSuppressed or so) - Provide wrapped lists for pre-access callbacks - Optimize a bit (check for redunadnteIsDeliver() calls and so)
Created attachment 112491 [details] the test project
Open issues: - The code prologues to be generated into the accessore should be moved from the *.javajetinc files to the proper Class.javajet file. - Before or after the inclusion? - Ignore many-valued getters?
(In reply to comment #4) > - We want to reuse the existing eAdapters list (instead of going through a > singleton list in EPackage) Why? This made it especially interesting for my use case. I would like to use the pre-read callback to attach listeners so that I can have listeners attached automatically. Singletons are bad though, how about an eAdapters list at the Resource level? Btw, you could also use this for implementing software-based transactional memory as a way to deal with concurrency when accessing EMF models. As always with these kinds of things, the possibilities are endless...
Created attachment 112504 [details] A functional prototype Eike and I pretty much have a working prototype including support in the generated (unconditional still)...
Created attachment 112509 [details] the test project for the functional prototype
Eike, I was starting to think whether we shouldn't put the things we added to InternalEObject in a separate interface from which InternalEObject inherits so that we could implement these interfaces even in ResourceImpl and ResourceSetImpl... Probably we should consider creating a subclass of the new list that implements EStructuralFeature.Setting only in the subclass. And of course we have our TODOs. I'm also keen to explore the reduced footprint EObjectImpl alternative we talked about... So much to do and so little time. Frank, Dave, Marcelo, and I would also like to focus on a final push to get the darned book finished in the next couple of weeks...
(In reply to comment #12) > I was starting to think whether we shouldn't put the things we added to > InternalEObject in a separate interface from which InternalEObject inherits so > that we could implement these interfaces even in ResourceImpl and > ResourceSetImpl... Yes, I think it is not completely senseless to add the new mechanism to other subtypes of Notifier and for Resource and ResourceSet it seems to make particular sense. Since I removed this point from my wishlist already twice I'm not so interested in it, but it makes sense ;-) > Probably we should consider creating a subclass of the new list that implements > EStructuralFeature.Setting only in the subclass. Since my CDOResources are proper EObjects they even have EStructuralFeatures, but for others it could make sense. > And of course we have our TODOs. > > I'm also keen to explore the reduced footprint EObjectImpl alternative we > talked about... Yes, I was just about to play a little in this area when I decided first to make use of this new feature to verify that nothing important is missing. > So much to do and so little time. Would you like me to do something to make this feature appear more quickly?
Good effort. With an eye to issue #247226, it'd be great if this one could receive some priority. Thanks v much.
This could help to close some holes in the EMF Transaction component. Great initiative!
Hi ! i followed the discussion because i'm very interesting about modification's notification of emf models. if i understood you have design a very useful functionality to add listeners on read and write actions ? but i don't see how a listener can stop the read/write action when it intercepts it in fact i found this topic via the newsgroup where somebody talks about locking in CDO and i don't understand how it can be implemented by this way :/ thank you Tristan FAURE
Tristan, I'm a bit concerned about the veto aspect. The basic idea is to throw a runtime exception in the callback and that this interrupts the access. For read access that's certainly fine and for simple write access, that's fine too. But when bidirectional updates are involved, it's problematic if the veto (a thrown exception) happens part way through the various updates of the ends because that would leave the model in an inconsistent state.
Ed, Thanks for you answer I now see how i can interrupt an operation in fact i've to design an emf functionality maybe based on CDO to offer the possibility to grant privileges on EMF models. I think i'll search some ways for my problem because i need to have some flags to know if a user can modify or not an element (i'll certainly ask my problem on the general newsgroup to not pollute the topic Regards (In reply to comment #17) > Tristan, > > I'm a bit concerned about the veto aspect. The basic idea is to throw a > runtime exception in the callback and that this interrupts the access. For > read access that's certainly fine and for simple write access, that's fine too. > But when bidirectional updates are involved, it's problematic if the veto (a > thrown exception) happens part way through the various updates of the ends > because that would leave the model in an inconsistent state. >
(In reply to comment #18) Maybe this brand new Bugzilla is also related: Bug #249278
Created attachment 114722 [details] another test project Check this out - it's a test project that actually does something interesting: new ControlUpdater<Label>(new Label(shell, SWT.NONE)) { protected void doUpdate(Label label) { // this will be re-executed whenever any of the EMF objects that // are accessed are changed later label.setText(library.getBranches().size() + " branches "); System.out.println("label updated"); } };
Created attachment 117927 [details] Updates Here are significant design changes. I've not tested them at all and they are still incomplete. Just wanted to keep them in a safe place.
This is a very nice feature, we at Xtext would like to do lazy linking with that. As with plain text you don't have a resource URI in all cases without loading referenced resources, we can't just use the usual proxy mechanism, because this causes loading of referenced Resources in a transitive manner. I've had a look at the last patch from comment #21. I know that it's hard to add stuff without breaking API, but IMHO it would be much nicer if the new callback could be a part of the Adapter concept without introducing a new hierarchy in parallel. I've been thinking of the I*Extensions4711-Pattern, but with a more meaningful name. Proposal: public interface PreAccessListener extends Adapter.Internal { // ... index constants and documentation ... void notifyAccess(Notifier notifier, int featureID, int index, boolean modify); } In addition the AdapterImpl class needs to implement this interface and have a default no-op implementation. I wonder if it then would be sufficient, to check and downcast the adapters to PreAccessListener at the different locations where these events should be triggered. Could this work in principal or did I overlook important backward compatibility constraints or requirements? Additional Note: I'm hoping that the feature will also work for dynamic models, when it's done. :-)
Hi Sven, Can you describe a bit more detailed your concerns with the current design? Is it about footprint or more generally about good re-usable design? We first tried to do it by re-using the adapter design but then decided that the two issues are unrelated enough to separate the design/implementation, too. IIRC our combined approach was not exactly the one you're proposing. We primarily wanted to avoid the creation event objects like for normal adapter notifications. With a combined design it would be harder for users (if possible at all) to specify adapters that only listen to the pre-events. And internally it would be much harder to optimize the notification through the use of eFlags (count interfaces instead of just cache !isEmpty). Another argument could be that we don't want to proliferate excessive and unintended usage of this feature because it can have a major impact on the performance. This feature is expected to work with all types of models except: 1) generated models that have explicitely disabled the default 2) generated Ecore itself (for performance reasons) What do you think?
> Can you describe a bit more detailed your concerns with the current design? Is > it about footprint or more generally about good re-usable design? It's because I think it is conceptually very similar to the 'notifyChanged' callback and as most EMF users know about this callback and have understood (more or less ;-)) how adapters work it would be very intuitive to have the additional callbacks next to 'notifyChanged'. At least I was expecting it there :-) > > We first tried to do it by re-using the adapter design but then decided that > the two issues are unrelated enough to separate the design/implementation, too. What makes the 'notifyChanged' callback so different from the notifications described here? Isn't this just an extension of the Observer pattern implemented in Adapter? > IIRC our combined approach was not exactly the one you're proposing. We > primarily wanted to avoid the creation event objects like for normal adapter > notifications. > > With a combined design it would be harder for users (if possible at all) to > specify adapters that only listen to the pre-events. Why is that? What's the problem with just ignoring the callbacks you're not interested in? > And internally it would be > much harder to optimize the notification through the use of eFlags (count > interfaces instead of just cache !isEmpty). Sorry, I'm not that much into it. Could you explain this a bit more? Or at least give me one or two pointers to the code you're talking about? > > Another argument could be that we don't want to proliferate excessive and > unintended usage of this feature because it can have a major impact on the > performance. > > This feature is expected to work with all types of models except: > 1) generated models that have explicitely disabled the default > 2) generated Ecore itself (for performance reasons) > > What do you think? > I think that this would be a very valuable feature to do things like - lazy binding (as mentioned we need a way to compute URIs for proxies lazily) - having defaults computed from other state (I know the other bug about derived features, but flagging such features as 'derived' would be a bit too invasive and restrictive in the context of Xtext.) - doing tracing for i.e. impact analysis or just debugging I'm not that concerned about the performance aspect, as it seems that you're doing a pretty good job on that ;-). But for sure it is a very important aspect and actually performance is the reason why we want to have lazy computation of crossref URIs. I just don't like seeing such many new concepts being introduced and also having this IMHO important API "hidden" in InternalEObject, if it could be done with much fewer changes and much more reuse of what's already there and learned.
(In reply to comment #24) > I just don't like seeing such many new concepts being introduced and also > having this IMHO important API "hidden" in InternalEObject, if it could be done > with much fewer changes and much more reuse of what's already there and > learned. > And just to make it clear: I don't think that it could be done the way I asked in my first comment. I really don't know and just wanted to ask. :-)
Hi Sven, I think the most important difference between normal adapter notification and this new kind of notification this that normal read access to the model can be intercepted. An average application calls (directly or indirectly) many many more accessors than mutators. I could well imagine that the factor ranges up to thousands or millions. To make this new feature acceptable by clients we need to meet two criteria: [1] In case this feature is not used by clients it must incur nearly no overhead. [2] In case it is used it must have a minimal influence on performance. To fulfill [1] we need to determine *very* quickly if there are currently "observers" present. Neither should the observers list be created lazily at that time nor should it be iterated to determine if there is one of the new observers present. That's why we decided to maintain a separate list and a separate EFLAG. To fulfill [2] we decided not to create event objects but rather call the observers directly and pass the associated information as arguments. Since I did not have the time look at the latest patch I can't give you more concrete pointers at the moment. Please ask Ed if you don't find it out yourself ;-)
Ok, know I understand what you were talking of. Thanks :-) Not having notification objects but passing integers to the callbacks seems to be very reasonable, yes. Wouldn't it be sufficient to manage the flags by checking what interfaces an adapter implements (i.e. is it implementing PreAccessListener?) while adding or removing an adapter to or from an EObject?
That would have two drawbacks: 1) Notifiers are forced to fire traditional notifications to observers that are only interested in the new notifications 2) An *additional* counter must be stored/managed (flags can't do this)
Also, the notification lists are carefully tuned so in fact there is a typed array uses to manage the adapters, which means that if you have to notify 100 adapters, you only have to cast once the whole array, not 100 times each element. Adding instanceof checks and casting to the loops would certainly not help improve performance; though we could manage separate arrays internally to deal with that. In any case, such an approach would certainly force an access listener to be an adapter and hence get notifications they might not be interested in. I suppose we could implement a marker interfaces indicating that something is purely an adapter not interested in notification... I'll give it all some more though (in my copious spare time).
I would rather prefer to have it for Ecore as well :P
Created attachment 128980 [details] Improved updates MinimalEObjectImpl supports the new interfaces with these changes. There's a GenModel flag to enable the code generation. Dynamic models should work as well. It's not well tested...
This invasive approach will never get done.
