Community
Participate
Working Groups
See bug 402781 comment #30 The createModelElement(*) method can be extended to support any model classes provided in the org.eclipse.emf.ecore.generated_package extension point. https://git.eclipse.org/r/#/c/17326/ PW
Paul, could you please give this a review as well? PW
Hmm, there's an easier way to do this using EMF directly, since it processes those extensions already: Set<String> keys = new HashSet<String>(EPackage.Registry.INSTANCE.keySet()); for(String nsUri : keys) { EPackage epackage = EPackage.Registry.INSTANCE.getEPackage(nsUri); EList<EClassifier> list = epackage.getEClassifiers(); for(EClassifier classifier : list) { if(classifier instanceof EClass && classifier.getInstanceClass() == type){ return type.cast(epackage.getEFactoryInstance().create((EClass)classifier)); } } } where "type" is a java.lang.Class. This approach shields us from knowing of EMF's extension points, and may also support EMF's dynamically-created types (I think).
I thought about this idea too, but if you take into account that the EPackage.Registry.INSTANCE will contain all EMF models in the environment (means also the models from other bundles) than the sequential search could be slow. That's the reason why I decided to use an approach which can eliminate wrong EPackages earlier. If the developer doesn't register the dynamically created EPackage inside the EPackage.Registry.INSTANCE it will also not be found. BTW, if a developer created the EMF model dynamically why should he use our service to create an instance of these model elements?
(In reply to René Brandstetter from comment #3) > I thought about this idea too, but if you take into account that the > EPackage.Registry.INSTANCE will contain all EMF models in the environment > (means also the models from other bundles) than the sequential search could > be slow. That's the reason why I decided to use an approach which can > eliminate wrong EPackages earlier. Creation isn't done very frequently, so I don't think the performance hit would be of paramount concern. Can you explain the implications of the comment on naming conventions: But if the EMF genmodel was modified and the EMF Metadata will not be generated (as in all UI related elements) the EPackage-Implementation will be referenced instead of the EPackage-Interface. To check this we relay on the EMF naming convention which causes the concrete implemenation to be inside of an "impl" package. CAUTION: If somebody also changes the EMF genmodel attribute "Package Suffixes/Implementation" this method will not be able to extract the package-name and the entire mechanism will not work anymore. Does this mean that we're *relying* on contributors to follow the standard naming conventions, or is it an optimization? Will things break if someone doesn't follow this naming convention? > If the developer doesn't register the dynamically created EPackage inside > the EPackage.Registry.INSTANCE it will also not be found. BTW, if a > developer created the EMF model dynamically why should he use our service to > create an instance of these model elements? Good question — I have no clue :-) nor a usecase, at the moment.
(In reply to Brian de Alwis from comment #4) > (In reply to René Brandstetter from comment #3) > > I thought about this idea too, but if you take into account that the > > EPackage.Registry.INSTANCE will contain all EMF models in the environment > > (means also the models from other bundles) than the sequential search could > > be slow. That's the reason why I decided to use an approach which can > > eliminate wrong EPackages earlier. > > Creation isn't done very frequently, so I don't think the performance hit > would be of paramount concern. Are you sure about creation isn't done frequently? Because I thought you would like to have all creation-calls of the application model to be done via this Service and if the application becomes larger and larger (open views (+editor views), different perspectives, …) this can be called very often isn't it? - But if more people thing that the extension point idea isn't worth it I will remove it. > > Can you explain the implications of the comment on naming conventions: > > But if the EMF genmodel was modified and the EMF Metadata will > not be generated (as in all UI related elements) the > EPackage-Implementation will be referenced instead of the > EPackage-Interface. To check this we relay on the > EMF naming convention which causes the concrete implemenation > to be inside of an "impl" package. > > CAUTION: If somebody also changes the EMF genmodel attribute > "Package Suffixes/Implementation" this method will not be able > to extract the package-name and the entire mechanism will not > work anymore. > > Does this mean that we're *relying* on contributors to follow the standard > naming conventions, or is it an optimization? Will things break if someone > doesn't follow this naming convention? This means it will work in a lot of EMF cases (known to me :-) ), because it covers the scenarios: * normal EMF model without any gen-model modifications * EMF models which do not want to display their EMF nature in there interface definitions What it doesn't cover is that if somebody changes the default package-prefix of the implementation package it will not be able to map the correct package-name to nsURI. But I think a lot of people use the “impl” as the package name which contains all of the implementations. This will of course cause some conventions to which the contributer has adhere to but there are already some if you want to extend the Application-Model with your custom Model-Attributes (e.g.: the custom model element has to extend the MApplicationModel to be generated via the EModelService). - If this is a problem than I think, after a decision was made about removing or keeping the extension point idea, we can either create our own extension point which can map the package to nsURI in a more fine-grained way or we can get rid of it completely. > > > If the developer doesn't register the dynamically created EPackage inside > > the EPackage.Registry.INSTANCE it will also not be found. BTW, if a > > developer created the EMF model dynamically why should he use our service to > > create an instance of these model elements? > > Good question — I have no clue :-) nor a usecase, at the moment.
> all creation-calls of the application model to be done > via this Service and if the application becomes larger and larger (open > views (+editor views), different perspectives, …) this can be called very > often isn't it? I think the target is to use the model service for all object creation and avoid using the the EMF factories. For example in https://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=9db76a062b421f3bc3540e3a9f0bc113e194f861 I used the model service in the PartService to create objects.
Rene: A few comments: 1) It is not a good idea to throw an exception on a deprecated EClass - this would lead to API breakage. A future deprecation of an EClass would break any code that calls createModelElement() with that EClass. 2) In comment 4 and comment 5, you and Brian discuss performance. Here are a few numbers. Launching a new workbench/workspace (in my dev env) leads to 690 calls, with an average time of about 5ms/call (on my machine). New code has an average time of about 30ms/call. Interestingly, createModelElement isn't called at all on restarting the workbench. 3) The most significant concern, to me, is whether it is a good idea to extend createModelElement to include any Ecore model. I added createModelElement() in an effort to isolate the E4 APIs from the Ecore APIs. The idea being that we would be able to switch out the implementation of the model interfaces without affecting client code. But, if clients start using createModelElement to instantiate any class from any Ecore model whether or not it is related to the E4 APIs, it seems to me that we are making a bigger dependency on EMF, not a smaller one. Thoughts?
(In reply to Paul Elder from comment #7) > Rene: > > A few comments: > > 1) It is not a good idea to throw an exception on a deprecated EClass - this > would lead to API breakage. A future deprecation of an EClass would break > any code that calls createModelElement() with that EClass. I know and as you have seen the ModelServiceImpl#checkDeprecation() method I've implemented there is just a POC with many TODO's. (As you can see in the TODO I also prefer a log-output, but there was no logger at the time of writing and I thought if you don't want to have this check why should I add an additional dependency into the code :-) ) > > 2) In comment 4 and comment 5, you and Brian discuss performance. Here are a > few numbers. Launching a new workbench/workspace (in my dev env) leads to > 690 calls, with an average time of about 5ms/call (on my machine). New code > has an average time of about 30ms/call. Interestingly, createModelElement > isn't called at all on restarting the workbench. I will check this by the end of the week and have a look where the implementation loses the time (but I think I can't beat the if-else-if-else... block) and maybe I switch to Brian's suggestion. > > 3) The most significant concern, to me, is whether it is a good idea to > extend createModelElement to include any Ecore model. I added > createModelElement() in an effort to isolate the E4 APIs from the Ecore > APIs. The idea being that we would be able to switch out the implementation > of the model interfaces without affecting client code. But, if clients start > using createModelElement to instantiate any class from any Ecore model > whether or not it is related to the E4 APIs, it seems to me that we are > making a bigger dependency on EMF, not a smaller one. Thoughts? As the method definition of EModelService#createModelElement(Class<T>) shows: public final <T extends MApplicationElement> T createModelElement(Class<T> elementType); It is impossible to return any other EMF object except those which implement the MApplicationElement but the EClassProvider is able to return you any EMF related class. So maybe we move the EClassProvider and its implementation out to a new bundle and/or make an ExtensionPoint for a better mapping between package and EPackage nsURI (as I suggested in comment #5).
(In reply to René Brandstetter from comment #8) > (In reply to Paul Elder from comment #7) > > Rene: > > > > A few comments: > > > > 1) It is not a good idea to throw an exception on a deprecated EClass - this > > would lead to API breakage. A future deprecation of an EClass would break > > any code that calls createModelElement() with that EClass. > > I know and as you have seen the ModelServiceImpl#checkDeprecation() method > I've implemented there is just a POC with many TODO's. (As you can see in > the TODO I also prefer a log-output, but there was no logger at the time of > writing and I thought if you don't want to have this check why should I add > an additional dependency into the code :-) ) > > > > > 2) In comment 4 and comment 5, you and Brian discuss performance. Here are a > > few numbers. Launching a new workbench/workspace (in my dev env) leads to > > 690 calls, with an average time of about 5ms/call (on my machine). New code > > has an average time of about 30ms/call. Interestingly, createModelElement > > isn't called at all on restarting the workbench. > > I will check this by the end of the week and have a look where the > implementation loses the time (but I think I can't beat the > if-else-if-else... block) and maybe I switch to Brian's suggestion. > > > > > 3) The most significant concern, to me, is whether it is a good idea to > > extend createModelElement to include any Ecore model. I added > > createModelElement() in an effort to isolate the E4 APIs from the Ecore > > APIs. The idea being that we would be able to switch out the implementation > > of the model interfaces without affecting client code. But, if clients start > > using createModelElement to instantiate any class from any Ecore model > > whether or not it is related to the E4 APIs, it seems to me that we are > > making a bigger dependency on EMF, not a smaller one. Thoughts? > > As the method definition of EModelService#createModelElement(Class<T>) shows: > > public final <T extends MApplicationElement> T createModelElement(Class<T> > elementType); > It is impossible (not impossible but you know what I meant) to return any other EMF object except those which implement the MApplicationElement but the EClassProvider is able to return you any EMF related class. So maybe we move the EClassProvider and its implementation out to a new bundle and/or make an ExtensionPoint for a better mapping between package and EPackage nsURI (as I suggested in comment #5).
If I can't find a faster solution for finding the EClass maybe we can leave the generated if-else-if-else...Block and move the generic way in front of the IllegalArgumentException as a kind of: Let's try something else and search for a user specific model element. This could bring back the 5ms/call in the normal case. Thoughts?
(In reply to René Brandstetter from comment #10) > If I can't find a faster solution for finding the EClass maybe we can leave > the generated if-else-if-else...Block and move the generic way in front of > the IllegalArgumentException as a kind of: Let's try something else and > search for a user specific model element. > > This could bring back the 5ms/call in the normal case. I was going to propose the same thing. This would give us speed for the "normal" case but flexibility for model extensions.
(In reply to René Brandstetter from comment #9) > ...(snip)... > It is impossible (not impossible but you know what I meant) to return any > other EMF object except those which implement > the MApplicationElement ... I stand corrected. I think your observation completely eliminates my concern. As for performance, I'm not too concerned (yet). The upfront cost of scanning the extension points is probably significant, but I'd be surprised if performance didn't get significantly better once a class was cached. I'm going to look a bit more carefully at the performance characteristics over time.
(In reply to René Brandstetter from comment #8) > (In reply to Paul Elder from comment #7) > > Rene: > > > > A few comments: > > > > 1) It is not a good idea to throw an exception on a deprecated EClass - this > > would lead to API breakage. A future deprecation of an EClass would break > > any code that calls createModelElement() with that EClass. > > I know and as you have seen the ModelServiceImpl#checkDeprecation() method > I've implemented there is just a POC with many TODO's. (As you can see in > the TODO I also prefer a log-output, but there was no logger at the time of > writing and I thought if you don't want to have this check why should I add > an additional dependency into the code :-) ) Logging a warning would probably better, but I'm not sure even that is necessary. Presumably if we've marked the EClass as deprecated, the corresponding Java interfaces will also be marked, so a developer will see the warning directly in his/her code. That seems much better than having to correlate log file warnings code back to source code.
Created attachment 236908 [details] Model-Object-Creation performance statistic I've take a closer look at the “generic object creation time consumption” stuff and all the concerns you have and here are my thoughts and the way to a new faster implementation. 1. I implemented the iteration suggestion from Brian de Alwis and noticed that it needs nearly twice the time as my generic way required, because of all the useless iterations over the time. 2. I realized that a lot of time was lost to find the concrete EClass because all of the standard M*-Application model elements are not resolved via the EPackage#getEClassifier(String) method. This caused an iteration over all EClasses in the found EPackage. 3. I started with some experimental implementations which used custom ExtensionPoints to get rid of the EMF related ExtensionPoint and so on. Finally I've took a (small) look about the concern that the developers shouldn't use our service to create their EMF model objects. At the end I combined some of your suggestions and created something that performs nearly as fast as the if-else-if-else block. (see the graphic I've added, sorry I'm not good at statistics) The solution is simple it still listens to changes of the EMF-related ExtensionPoint, because adding a new one causes some additional configuration which can be forgotten or even worst be an additional point of fail configuration. But the listener now doesn't build a simple map of package-name to EMF Package nsURI it uses the idea from Brian de Alwis and builds up a map of Class to EClass which can directly be used to retrieve the EClass. This solution removes the previously needed EMF naming-convention code and the EPackage#getEClassifier(String) lookup. To keep the Map as small as possible (not all found EClasses in a system) I've added a filter to keep only EClasses which extend the MApplicationElement-EClass, because those are the ones the EModelService will create. The simplified/specific lookup made the EClassProivderService useless and so I was able to remove it in favor of an internally used ObjectFactory class. As you can see I've tried to combine your suggestions and worries ;-) into a new solution which is available under a new gerrit commit: https://git.eclipse.org/r/#/c/17769/ P.S.: To still have the performance of the if-else-if-else generated code I would like to put it in front of the new generic way. But I wasn't able to find the templates nor the call of the generator Paul used and so I couldn't add the generic call to it. Paul if you still find this a good idea just leaf some hints where to find that part and I will add it.
Published the https://git.eclipse.org/r/#/c/17769/ patch to master as https://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=4ba99c68eee2c4b938ae2d0057d041fd94d5e604 Rene: Can you abandon the previous change (https://git.eclipse.org/r/#/c/17326/), as we aren't going forward with it at this time. Thanks.
Hi Paul, I've abandon the 17326 Gerrit review. Maybe we can talk about the TODO's I've added and remove them before this bug is fixed? open TODOs: * Should we integrate the "if-else-if-else... block" generator? (If yes, where can I find the generator and its call?) * Should I remove the checkDeprecation() call which now writes a log? (If the method is kept, where should I put the constants for it?) Regards René (In reply to Paul Elder from comment #15) > Published the https://git.eclipse.org/r/#/c/17769/ patch to master as > > https://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/ > ?id=4ba99c68eee2c4b938ae2d0057d041fd94d5e604 > > Rene: Can you abandon the previous change > (https://git.eclipse.org/r/#/c/17326/), as we aren't going forward with it > at this time. Thanks.
Rene: thanks for abandoning 17326. Is there anything else to add, or can I resolve this one now?
Hi Paul, if all of my todos in the code are obsoulete, then just remove them and remove the corresponding code. If you want I can remove them and check it in again.
Rene: Let's not worry about the if/else/if generated code unless someone proves to us that the performance difference is significant. As for the logging, I think its worth keeping. But, if an app uses a deprecated type, we should only make one log entry per session, otherwise, we could fill a user's log with such warnings. We use such a technique for duplicate key bindings (and I'm sure for other things, too).
Hi Paul, two small questions left: 1. Should I create a new Gerrit-Commit for the final cleanup (remove of my TODOs and "reduced" log entries) or re-use the existing one? (Because you've already merged it.) 2. Where should I put the String constants of "GenericMApplicationElementFactoryImpl#checkDeprecation(EClass)" and should we also keep the "since" detail? (In reply to Paul Elder from comment #19) > Rene: Let's not worry about the if/else/if generated code unless someone > proves to us that the performance difference is significant. As for the > logging, I think its worth keeping. But, if an app uses a deprecated type, > we should only make one log entry per session, otherwise, we could fill a > user's log with such warnings. We use such a technique for duplicate key > bindings (and I'm sure for other things, too).
(In reply to comment #20) > Hi Paul, > > two small questions left: > > 1. Should I create a new Gerrit-Commit for the final cleanup (remove of my TODOs > and "reduced" log entries) or re-use the existing one? (Because you've already > merged it.) I don't think you can re-use the existing Gerrit task (because I've merged it). Just create a new one. > 2. Where should I put the String constants of > "GenericMApplicationElementFactoryImpl#checkDeprecation(EClass)" and should we > also keep the "since" detail? > Hmmm. I guess I hadn't looked at this too carefully. What would be really nice would be to detect the @deprecated javadoc tag in the standard EMF doc annotation, but I just tested that - it's not there are runtime. Have you tested with your custom annotation? I'm less sure about a custom annotation. It's hard to remember when editing the .ecore model (although we could rely on copy and paste). I feel it would be very likely that a future deprecation would not be properly marked. Overall, the deprecation check is nice, but not essential. A plug-in developer who uses a deprecated element will know because of compiler warnings anyhow.
(In reply to Paul Elder from comment #21) > (In reply to comment #20) > > Hi Paul, > > > > two small questions left: > > > > 1. Should I create a new Gerrit-Commit for the final cleanup (remove of my TODOs > > and "reduced" log entries) or re-use the existing one? (Because you've already > > merged it.) > > I don't think you can re-use the existing Gerrit task (because I've merged > it). Just create a new one. > > > 2. Where should I put the String constants of > > "GenericMApplicationElementFactoryImpl#checkDeprecation(EClass)" and should we > > also keep the "since" detail? > > > > Hmmm. I guess I hadn't looked at this too carefully. What would be really > nice would be to detect the @deprecated javadoc tag in the standard EMF doc > annotation, but I just tested that - it's not there are runtime. Have you > tested with your custom annotation? I'm less sure about a custom annotation. > It's hard to remember when editing the .ecore model (although we could rely > on copy and paste). I feel it would be very likely that a future deprecation > would not be properly marked. Overall, the deprecation check is nice, but > not essential. A plug-in developer who uses a deprecated element will know > because of compiler warnings anyhow. Yeah the byte code doesn't care about the JavaDoc but it knows about the @java.lang.Deprecated because this has a RetentionPolicy of type RUNTIME. But to have this inside the generated models, we have to change/adapt the JetTemplates of the EMF-GenModel to write this annotation. This would be no problem because EMF allows this, but if somebody forgets to mention the template in its own GenModel than the annotation will not be added. So I can think of 3 solutions for this problem: 1. create our own JetTemplate which will also write the “@java.lang.Deprecated” to the corresponding elements 2. create (look for) a Bug in the EMF component which allows the the model to be enriched with Java-Annotations (e.g.: @java.lang.Deprecated), because I think this would be needed by other developers too 3. remove the entire deprecated check from the GenericMApplicationElementFactoryImpl I think we should make a combination of 2 and 3, because I think your are right the developer should see the deprecated method with a compile warning and so our check is only a performance loss. The 2nd point is not only useful for the @java.lang.Deprecated annotation if you think about all the other frameworks (e.g.: hibernate, ...) which use there own annotations and the code-analyzes tools like PMD, FindBugs, … All those code-analyze tools for example complain about the “==” check instead of the “equals” check, but in EMF it's fairly OK to do such a comparison and to turn off the check in a particular case they normally provide an annotation.
I already found the EMF Bugs which is responsible for Java-Annotation generation in EMF: Bug 217220 and Bug 176726
I've fixed the todos in the Gerrit-Commit: https://git.eclipse.org/r/18690
(In reply to René Brandstetter from comment #24) > I've fixed the todos in the Gerrit-Commit: https://git.eclipse.org/r/18690 Release to master as: https://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=d0d5df142ca0087f00b5fbd88ab8d91973428b10 Thanks René
In 4.4.0.I20131209-2000, I noticed we need to remove the @generated tag from the method's doc comment. I'll do that in the M4 bug fix day (Dec 11)
(In reply to Paul Elder from comment #26) > In 4.4.0.I20131209-2000, I noticed we need to remove the @generated tag from > the method's doc comment. I'll do that in the M4 bug fix day (Dec 11) I think we should also update the JavaDoc of the org.eclipse.e4.ui.workbench.modeling.EModelService.createModelElement(Class<T>) method because it can now create much more than the mentioned model elements.
(In reply to René Brandstetter from comment #27) > (In reply to Paul Elder from comment #26) > > In 4.4.0.I20131209-2000, I noticed we need to remove the @generated tag from > > the method's doc comment. I'll do that in the M4 bug fix day (Dec 11) > > I think we should also update the JavaDoc of the > org.eclipse.e4.ui.workbench.modeling.EModelService. > createModelElement(Class<T>) method because it can now create much more than > the mentioned model elements. I'll do that, too. Thanks!
Java doc changes pushed to gerrit as: https://git.eclipse.org/r/19649
Verified in 4.4.0.I20131211-2000
(In reply to Paul Elder from comment #12) > As for performance, I'm not too concerned (yet). The upfront cost of > scanning the extension points is probably significant, but I'd be surprised > if performance didn't get significantly better once a class was cached. I'm > going to look a bit more carefully at the performance characteristics over > time. Bug 427790 demonstrates that the fix needs retraction since all plugin code not just extension points are loaded.
The fix to bug 427790 has address the performance concerns. Starting the EModelService now only activates models that are explicitly registered as model extensions. By default, the registered models are only the core E4 UI model and the fragments model.
Marking VERIFIED as per Paul's last comment...in 4.4.0.I20140528-2000.
