This Bugzilla instance is a read-only archive of historic NetBeans bug reports. To report a bug in NetBeans please follow the project's instructions for reporting issues.
Quite often listeners are designed to listen only on certain repository objects, not on a hierarchy of objects. The current MDR event handling mechanism forwards events to parent objects in a hierarchy defined in http://www.netbeans.org/download/mdr/apis/org/netbeans/api/mdr/events/MDRChangeSource.html This can disturb in certain cases because of the overhead of many needless listener calls and unnecessary comparisions in listener implementions. Hence I propose to add a method void addHierarchyListener(MDRChangeListener listener) to MDRChangeSource, which gets the semantics of the old addListener method. Implementations shall implement addListener(MDRChangeListener) in such a way that only events concerning this object directly are forwarded. The complete proposal consists of: 1) Change MDRChangeSource and its Javadoc accordingly 2) Change the implementation of MDRChangeSource accordingly 3) Review all calls of MDRChangeSource.addListener(..) to check if addListener or addHierarchyListener must be called. 4) Review all listener implementations if unneeded comparisions etc. may be removed. 3) and 4) comprise e.g. changes to MDRExplorer.
mmatula added as CC because I would like him to review this message. It's about changing the API: I start working on this issue and will provide a patch for parts 1) and 2) soon. The patch will change the API and is not backwards-compatibel, because the meaning of `addListener' will be changed. All calls to `addListener' will be renamed to `addHierarchyListener'. I will also rename all occurrences of `addListener' in the MDR sources to `addHierarchyListener'. Please intercept ASAP if this API change is undesirable. I could leave the meaning of `addListener' as it is and add a new method like `addRestrictedListener' which listens only on the object itself. I think my proposal is simpler to understand for future users, but will not argue this case if API stability is more important. In this case I would ask to propose an alternative name instead of the ugly `addRestrictedListener'.
Created attachment 6483 [details] patch to provide hierarchy listeners
I've appended the patch which should implement hierarchy listeners. I kindly ask for review, testing and check-in. I regret that I didn't yet set up the testing environment for MDR patches, so its totally untested (even uncompiled). On the other hand the changes are so straightforward, that I do not expect any bigger problems. The patch should fix points 1), 2) and 3). Additionally the patch contains some Javadoc and structuring comments for class MDREventHandler which I added in the attempt to understand how event handling is done in MDRExplorer.
Sorry that I haven't reviewed your proposal soon enough. I am currently in U.S. so it was very soon in the morning here when you asked me to do so. I will go through the patch and apply it during the weekend. One minor change I would like to propose about the new API is that I would rather have an additional addListener method with one more parameter - recursive (or something similar) instead of addHierarchyListener. The default addListener method will add non-hierarchical listener as you suggested, addListener with the additional parameter set to true would add hierarchy listener. (I think this will be nicer - I do not like the name HierarchyListener :)).
No problem. I do not like the name `HierarchyListener', too. I will provide a modified patch within the next hours.
Holger, please don't do that. I can easily handle it while applying this patch. I would like to go through all the changes anyway...
To late, already done.
Created attachment 6490 [details] modified patch with addListener(.., boolean recursive) instead of addHierarchyListener(..)
Hi Holger, IMO non-hierarchical events as implemented by the patch are not that usable. Here is what are the problems I see: 1) registering listeners on package is not usable without seting recursive to true (should the recursive be attribute be implicit?) 2) besides attribute values, change of links to the instance should also be reported to instances (even in non- recursive case) 3) delete events are currently fired on the object that is being deleted, create events are fired on the factory for given object (repository for packages, class proxy for instances) - i.e. to catch creations and deletions of objects using non-hierarchical listeners one would need to register to two different objects. If one registers on class proxy, they will receive also attribute changes of class-level attributes. (But we can consider this as not too big overhead as there is not so many classes with classifier-level attributes) We should consider use-cases for using non-hierarchical listeners - if we don't design them properly, people will still need to use hierarchical listeners to solve their problems and they will be of no use. Following are the changes I would like to propose: 1) propagate association events to instances also in non- recursive case 2) propagate delete events to class proxy/repository also in non-recursive case That should be it. I would also like Constantine Plotnikov to review this as he contributed to the design of the original events API and possibly Novosoft will implement it in their nsmdf.
(I've added Svata as he should also review this) Another problem (I think the major and the last one :) with non-hierarchical listeners is that registering listeners when one creates a new object is problematic. The new instance of a class/package gets propagated to chage events only (not pre-change). However these events are asynchronous and thus registering listeners to newly created objects on these events does not work as you may already have lost some of the events fired by the time the asynchronous change event gets to you. Solution to this problem would be adding another type of event (synchronous post-change). However this makes it more complicated and possibly slow. We should find out how to solve this by not slowing MDR down too much - i.e. we should minimize unnecessary calls, so the questions to answer are - if someone implements pre-change, will he also need post-change?, if someone implements post-change, will he also need change? What do you think?
Hi Martin, thanks for your review. Concerning your objection from 2002-06-30: You're right, my patch was to schematically. Considering all your arguments I would propose an API like: void MDRChangeSource.addListener(MDRChangeListener listener, int flag); or void MDRChangeSource.addListener(MDRChangeListener listener); int MDRChangeListener.getFlag(); We would have flags like: LISTEN_ON_ATTRIBUTE_CHANGE LISTEN_ON_INSTANCE_DELETION LISTEN_ON_INSTANCE_CREATION LISTEN_ON_LINK_DELETION LISTEN_ON_LINK_CREATION LISTEN_ON_ALL etc. and corresponding propagation rules. As ever, please propose better names. EventNotifier.Abstract then would need only two HashMap as in the current implementation. But when the listeners are collected, it must be checked, if the flag, they are registered for (resp. they have), applies to the current event. Concerning 2002-07-01, 00:00: I don't see this to be a problem and I don't see this to be connected to the introduction of non-recursive listeners at all. In use cases like MDRExplorer it is not a problem, because you will connect the view (e.g. node) for the new instance with the new instance only after the instance has been created. However you do this, you can add a listener in the same read or readwrite transaction in which you establish the connection view <-> instance. Other use cases which would suffer from the problem you mentioned should attach a (recursive or partially recursive, see above) listener on the class proxy. Am I right ? How to integrate Constantive Plotnikov into this discussion ?
Hi Holger, sorry, but I don't like the flag. Would it be a problem to say that association events are always fired on instances and delete events are always fired on proxies? Regarding the second issue, I agree that for MDRExplorer even much simpler event model would be enough. However I don't think that it is a good example - MDRExplorer can live purely with change events (it does not need to use preChange events). I was more concerned about usage of events in Java module (that's why I cc'ed Svata) and other applications that need to make use of "preChange" events because of their synchronousness :). I think that these are typically MDR clients that would break if they would miss some event. The problem with registering listener also on a class proxy is, that to catch event soon enough on a class proxy you need to register the listener also on the whole repository (to catch createExtent immediately followed by refCreateInstance). But then you got to the purely recursive case and that's only because of this simple problem which - if solved - can be handled by non- hierarchical listeners. Once we got into this "non- hierarchical listeners" discussion, I would like to solve this problem, otherwise in many cases the non-hierarchical listeners will not be usable. BTW: I've CC'ed Constatnine yesterday - that's how he got integrated :)
Why don't you like the flag ? I thought it's the most consistent approach. Why strive to have more fine-grained event handling, that would be the most fine-grained approach without serious implementation problems. Even more: code which otherwise must be repeated in every listener implementation (compare if it's the right listener for this event) would be written only once in EventNotifier. The listener implementations would be less error-prone. On the other hand: there is no real problem with your proposal. I must admit I have problems to follow your second argument. My thoughts are like follows: Either we have a specific listener for a specific instance or we have generic listeners which listen e.g. on all instance of a class. All use cases, where we have specific listeners, should be solvable this way: Add the listener immediately after instance creation in the same transaction in which the instance is created or add the listener immediately after the object, which shall listen, is created, whatever first. The appropriate and more performant solution for all use cases with generic listeners is recursive listening. Extent creation can be put into one transaction with the addition of the recursive listeners to class proxies. The only events you can't catch in this case are the one you caused yourself in this very transaction between extent creation and listener addition. You wrote: "Another problem (I think the major and the last one :) with non-hierarchical listeners is that registering listeners when one creates a new object is problematic. The new instance of a class/package gets propagated to change events only (not pre-change)." Yes, but the method called to create the instance synchroneously returns the created instance and, therefore, you can add a listener immediately after instance creation.
Holger, isn't it quite straightforwar that many other implementations that are not creators of the object will need to listen to it? I.e. registration of listeners will be independent of code that creates instances? How do you want to ensure that the code that creates an object is invoked in the same transaction as the code that registers the listener? Regarding flags I don't think it is a good idea to make listeners more granular using flags as it will lead to unreasonable growing of number of listeners. I think that usually people will use one listener for handling similar things and thus they will use "if" clauses inside of them anyway. Flags make the API only more complicated providing options that are IMO not necessary.
Other objects not being the creators have to listen: ==================================================== That's clear. Only I cannot imagine a real-world use case where it is important for such a listener to listen from the very beginning of the objects existence. If it's a generic listener like an Undo-Manager, it should listen from the very beginning, yes. But if it is specific for this single object (like a explorer node), why should it want to listen on the object before it even has access to the object. That's what I cannot understand. Maybe *Svata* can help out with an example of a use case. As soon as I will be able to think the use case, I probably will agree with you. Flags ===== It's not the most important point for me, so I will go with you, if you stick on your meaning about flags. Nevertheless I would not necessarily lead to a growing number of listeners, because users would be allowed to use constructs like LISTEN_ON_ATTRIBUTE_CHANGE | LISTEN_ON_INSTANCE_DELETION to register a listener which listens on both kinds of events. The flag would be similar to a declaration of what this listener is meant for and manual code review would easily determine any errors made by the user. But, is I said, if you want to go forward without flags, I'll go with you. I wish you a nice holiday !
OK, Holger. I think that issue with registering listeners for newly created objects is quite isolated and not really related to your patch - the patch can be integrated now and the issue can be solved later. Although when performing an API change I would like to do it at once (also together with events about transactions). Thus I would prefer waiting for Svata's comments and integrate the patch after I come back from the vacation if you don't mind (the patch will affect our unit tests so we need to make sure that everything is corrected). I guess that your UNDO manager uses hierarchical events anyway, doesn't it? Regarding flags I know that they not always mean adding number of listeners. However it seems to me that in any case (but the really simple one with one flag) you will always need to do some checking in the listener. Anyway, maybe I will change my mind after the vacation. I really need it to clean my head...
Martin, let's do it after your vacation. Maybe others (Svata, Constantine ?) can comment on this issue meanwhile.
Holger, Martin, my use case is as follows: my clients should be able to create source code pieces using JMI calls - once the instance is created and bound to some context (inserted into a JavaSource instance or JavaClass instance), the code generator should reflect all changes into the source text. Fortunately for this case, there always exists a container element, which represents the source text, or the outer class (method, field) for the new element; so when the element is added into the container, my listener (on the container element) has a chance to add instance listener to the new member. It requires tackling with both pre-change and post-change listeners, however. In this case: clz = pck.getJavaClass().createJavaClass(); someOtherClass.getNestedClasses().add(clz); clz.setName("FooBar"); a listener must be attached to `clz' before setName() is invoked, otherwise the corresponding change event would be already in the dispatch queue when the listener is added (e.g. as a response to NestedClasses association post-change). Some data flow between metamodels (symbol usage tracking) may require that an instance is listened to from its inception, but I don't know details yet. There's a chance, that I could use similar approach as the one above (the validity is not determined by new instance's attribute, but by association with its environment). I have another problem w/ event system I also wanted to solve by hierarchical listener on the package extent (although there's quite dense traffic there): Imagine the client code does something like: JavaSource src = <get source model from somewhere>; src.getClasses().get(0).setName("FooBar"); Now - the second line uses JMI only and `name' attribute is not derived. The code generator's code must somehow step in prior to setName() and start listening on the JavaClass instance (and then process the attr change event). Note that in this case, no instance object is created, it is only fetched from the persistent storage transparently to the calling client. I do fear that there will be quite an overhead, since the package extent listener will need to lookup the instance in a hashtable for virtually any change call being made within the package extent... brr. Do you have other solutions for me ?
I've written down my new API proposal which deviates from my patches in the following points: - Removed boolean parameter `recursive' for addListener method - Added int parameter `mask' to addListener and removeListener methods. The meaning is the register resp. unregister a listener for certain types of events. The default for `addListener' is: all events which originate on the object the listener is registered for. The default for `removeListener' is: all events. - Added transaction events. I'll append the Javadoc of the proposed new API and kindly ask for comments. (Actually the new API is already implemented in my local sources so I can provide a patch in near future.)
Created attachment 6618 [details] jar-archive of the javadocs of the proposed new API
See http://mdr.netbeans.org/servlets/BrowseList?listName=dev&by=thread&from=12829 for further discussions on this issue. Hi Martin, I'm going to add two versions of EventNotifier, both implementing bitmasked listeners. Both passed the unit tests. The first is a simple and straightforward solution, which probably requires further optimization. The second is an optimized solution. The optimization in the second versions consists in: a) using tailor-made arrays instead of Collection API (this is done locally in certain inner classes and is hidden from the EventNotifier core, so it should be well maintainable) b) using type-safe data-structures to avoid casts c) reducing the number of hash-table lookups d) reducing the number of comparisions made synchronously to a minimum e) reducing the number of method calls insofar as it can be done without worsening maintainability Running the unit test on both version should no performance difference. (Probably the unit tests are not made to measure performance ;-) I send these two classes instead of a full patch, because, as I already told, it takes some work for me to prepare the patch, so I would like to be sure, that it will be accepted. The EventNotifier class contains the most problematic changes, hence we should decide on it first. My personal preference would be to use the second approach. If you agree I would like to remove the static inner classes containing the data-structure and make them top-level classes (or even better: interfaces & default implementations) in the util package. Currently the only reason I added those new classes as static inner classes of EventNotifier was, that I wanted to have all code to be reviewed within one file. Concerning the performance of the bitmasked-approach as compared to the plain approach without bitmasks: - if the performance of (type && mask) == type counts far heavier than the performance of method calls, then both approaches should be of similar performance - if the performance of method calls counts similar of heavier than the performance of (type && mask) == type then the bitmasked approach should be more performent. - the main advantage of the bitmasked approach seems to be, that it allows tailor-made implementations for certain apps if only the datastructure-classes are made interfaces the implementations of which may be choosen by the app
Created attachment 6710 [details] EventNotifier.java version 1
Created attachment 6711 [details] EventNotifier.java version 2
Performance note: version 1 should be faster for addListener/removeListener version 2 should be faster for event firing version 2 could be made as fast as version 1 concerning addListener/removeListener by adding a HashMap listener=>index for index lookup
Hi Holger, I think I like the EventNotifier. My preference is to have the first one - the second one seems overoptimized to me, but I will have closer look at it later - currently I don't have time. One more comment I have is that I think that the default method addListener without bitmask should register listener for listening for all events. This way it stays consistent with the current state and also makes it obvious that bitmask is used for filtering only (not for adding more events) - as it really is also in the implementation.
Regarding default semantics of addListener: OK Regarding variants of EventNotifier: Please tell me the results, when your review is finished. I will then prepare the patch. Nevertheless what variant you'll choose I will abstract away from their differences such that their is a common interface (of the datastructures) and the implementations can be exchanged easily without touching the rest of EventNotifier.
Holger, I prefer EventNotifier-1 and doing more optimizations only after it becomes an issue. It seems to me that the other EventNotifier is too complicated (implementing whole mechanism of allocating memory for listeners itself) and it is not clear how much performance we gain by adding that complexity. If you still have a strong opinion about including EN-2, I will leave it up to you. Regarding the helper classes I don't think they are that important that they deserve their own API. I would leave them as innerclasses of EventNotifier as they are specific to it. Martin
Hi Martin, I'm going to append my patch. It contains, among others: - the proposed API changes (excluding the changes in EventNotifier.java, please use one of the both variants I already have posted) - changes to the build process, which make it independent of the location of other netbeans files (hardcoded names replaced by properties, which are set by default to the standard values but can be overridden locally) - numerous automatic layout changes (unfortunately caused by using Reformat Code from within the IDE) - many javadoc fixes & structural comment additions - the code which allows to replace the basic handler classes by alternative implementations (the frontend of this code is commented out, so that it cannot be used, the backend is left intact, what means that MDRStorage informs the generator classes about the base classes to be used. This allows later on to make MDRStorage configurable in this point. The out-commented frontend allowed this configuration using system properties resp. other parameters) - small fixes (e.g. keyword synchronized removed from some methods, an inner block of which was made synchronized instead; because the inner block is only called under certain circumstances, the new code should be more performant) If there are any further questions concerning the patch, please come back. I can hardly answer all possible questions in advance ;-) Furthermore I'm going to append the image used in the Javadoc of MDRChangeSource.java. Please add it to the Javadoc (currently MDRChangeSource.java contains in its Javadoc a request to add this image) and change MDRChangeSource.java accordingly. I didn't do this, because it was not transparent enough for me how this should be integrated in the build process. There are several ways, I know, but I do not know what is netbeans.org standard in those cases. Good luck during the review ! Holger
Created attachment 6773 [details] finally: the patch
Created attachment 6774 [details] image for API javadoc
Hi Holger, thanks a lot! Here are my comments: 1) I don't think that replacing handlers by different implementation is useful, (at least not globaly for whole VM) thus I will remove any references to it from the patch to not spoil MDR with unused things. I think instead of enabling replacement of handlers it would be more useful to separate Storables by a layer of SPI. We have it in our future plans. In addition we are already working on the feature I suggested in discussion with Brian Smith on users@mdr.netbeans.org mailing list (thread: What handler code gets called, and how do I call the default?) 2) I think that getByMofId method should return null if the object with a given MOF ID is not found. And this should be documented in the JavaDoc. I will do this change. 3) "Doublechecking" does not work (see e.g. Effective Java from Josh Bloch) and should be avoided. Thus I will remove it from your patches. Simple explanation: if (x == null) { synchronize (this) { if (x == null) { ... } } } When one thread is performing x = new X(), for a short moment x is not null but it contains a reference to an invalid uninitialized object. There are many resources on the internet explaining why doublechecking does not work.... 4) Looking at the transaction events I realized I have a problem with it. It seems to me that the only event that you need is "Transaction" event. This event gets fired when the transaction is started. I think this is sufficient. You will know that rollback happened by receiving changeCancelled event. It seems unnecessary to me to have two more events (rollback and commit). For now I will skip it from the patch. Let's discuss it and add it later.
Hi Martin, 1) My patch did not provide handler per JVM, but handlers per storage. Are there some documents concerning your future plans related for a storable SPI. I ask because it might well be that we need some specific implementations in the near future. (I think I will come back to this issue in near future, probably opening a new issuezilla thread, but it would be nice if you could give some further hints about the planned SPI.) 2) no objection 3) Thank you for your explanation. Actually I learnt to use double-checking because I found it in parts of the Netbeans sources (not MDR). Browsing the internet I found code fragments like: version 1: ---------- private A a; private boolean aInitialized = false; public A getA() { if ( ! aInitialized ) { synchronized(this) { if ( ! aInitialized ) { a = new A(); aInitialized = true; } } } return a; } version 2: ---------- private A a; public A getA() { if ( ! a == null ) { synchronized(this) { if ( ! a == null ) { A a = new A(); this.a = a; } } } return a; } version 3: ---------- private Object tmp; private A a; public A getA() { if ( ! a == null ) { synchronized(this) { if ( ! a == null ) { tmp = new A(); a = (A) tmp; } } } return a; } Not that I want to recommend these versions for the MDR code, but I wonder if these pieces of code would work ? 4) Probably it's technically enough to have only one transaction event type, but it don't think that it's the most user-friendly approach. Think about transaction synchronization MDR <-> database or MDR <-> MDR. Suppose the user want's to use only post-change listeners and the application allows him to do this. In your approach the event sequence would be: TRANSACTION CHANGE CHANGE CHANGE ........ (wait for 2 hours until the next transaction starts) TRANSACTION Hence the transaction to an external database must be opened for 2 hours, until it can be closed.
3) Version 1 should be correct if you make the boolean field volatile. However it is still discouraged to do it because volatile does not really work on some VMs? (or something like that :)) 4) Note that in your approach user also needs to watch preChange events - rollback gets never fired asynchronously, because when a transaction is rolled back, changeCancelled is fired instead of post change.
3) Thanks for further explanation. 4) No! When the transaction rolls back the user will be in the happy situation never to be informed about the transaction (exactly what he wants in this case). Independently of that the argument against your proposal remain. The user only will be informed about transaction commit when a new transaction starts. But when no new transaction starts, he will not be informed at all. Very problematic in cases when some external resources have to be locked until the transaction commits/rolls back.
OK, I see. But I still feel it is kind of strange that user first receives planned rollback and then rollback cancelled although the rollback happened. So let's remove the rollback event and put only transactin start/end. Cancelled end means rollback, postChange end means commit.
I understand you this way: For every commit: pre-change TRANSACTION_END + post-change TRANSACTION_END For every rollback: pre-change TRANSACTION_END + cancelled TRANSACTION_END That looks nice !
Yes, that's what I meant. Because currently you had: preChange: COMMIT changed: COMMIT or preChange: ROLLBACK cancelled: ROLLBACK Which I think is ugly and was the motivation of my problem with transaction events as in your patch. So it seems that we have agreement on it. I will update the patch accordingly.
Created attachment 6810 [details] image adapted to API changes
fixed