Bugzilla – Full Text Bug Listing
|
Description
Axel Uhl
Created attachment 185702 [details] Patch adding per-registry cache Additionally, as Adolfo had suggested in https://bugs.eclipse.org/bugs/show_bug.cgi?id=333032 this makes the constructor protected to force clients to use the getInstance(...) operations. The class shouldn't be made final because specializations may benefit from the finding of the opposite ends in the metamodel while implementing their own strategies for navigating them in the instance models. Axel, It looks good to me. I'll create a new patch in the other bug avoiding to change the creation of a DefaultOppositeFinder. P.S: Thanks for the clarification of the utility of extending the default oppositeEndFinder. Cheers, Adolfo. Axel, Perhaps, you may want to do your debut as MDT/OCL committer with this bug ;). As general rule: - You need at least one +1, and no -1, from other committers prior to do any committ to MDT/OCL Core functionality. - This rule doesn't strictly apply to examples stuff, although is desirable that a committer have a chance to review any code anyway. You should also update the wiki's new and noteworthy (http://wiki.eclipse.org/MDT/OCL/New_and_Noteworthy) items for our releases: our next simultaneous release (Indigo) , and/or our on going service releases (Helios) P.S: I'll try to update our wiki with this (similar) information, which concerns our development's team "must know" (http://wiki.eclipse.org/MDT/OCL/Dev/Setup) , which I think you already know P.S2: "It looks good to me" in comment 2 = +1 !!! Cheers, Adolfo. (In reply to comment #3) > Perhaps, you may want to do your debut as MDT/OCL committer with this bug ;). Haven't received my CVS credentials yet. I guess the admin folks don't work these days ;-) > (http://wiki.eclipse.org/MDT/OCL/Dev/Setup) , which I think you already know > P.S2: "It looks good to me" in comment 2 = +1 !!! Then why don't you go about committing it? I assume my current status to be "committer who hasn't received his CVS credentials yet" so I would guess my implicit +1 should count :-) My "debut" then has to wait for the impact analyzer stuff which is to go into the examples section. -1 See Bug 333241 that demonstrates that this is solving the wrong problem. (Much less should be cached, which makes the use of EPackage.Registry as a key invalid.) Propose WONTFIX. (In reply to comment #5) > -1 See Bug 333241 that demonstrates that this is solving the wrong problem. > (Much less should be cached, which makes the use of EPackage.Registry as a key > invalid.) Are you proposing to store the "hidden" opposite references in an adapter that attaches to the class that is the type of the "real" reference? Created attachment 185866 [details]
Cache in adapters on Ecore elements
According to Ed's proposal, this patch uses adapters on the EClass elements to cache hidden opposites. EPackage elements for which contained hidden opposites were already cached are marked by an according adapter.
Can packages including their EClass elements get unloaded while EPackage elements containing references to such classes remain loaded? If not, this should work fine. If yes, the marker adapter on such a package may need to be invalidated as the referenced classes get unloaded. I assume that Ecore may not support proper unloading of an EClass that is referenced by an EReference in some other EPackage where this reference is resolved. It would need to turn into a proxy. Do we have to assume that this happens?
> (In reply to comment #6) > > -1 See Bug 333241 that demonstrates that this is solving the wrong problem. > > (Much less should be cached, which makes the use of EPackage.Registry as a key > > invalid.) > > Are you proposing to store the "hidden" opposite references in an adapter that > attaches to the class that is the type of the "real" reference? Axel: Your comments confuse me. Are you confusing bug 329167 where we are introducing EAnnotation adapters with bug 333241 which is where I propose performance enhancement of OppositeEndFinder; no adapters at all. (In reply to comment #7) > According to Ed's proposal, this patch uses adapters on the EClass elements to > cache hidden opposites. EPackage elements for which contained hidden opposites > were already cached are marked by an according adapter. I don't understand this. Certainly not Ed W's intention. Has Ed M made a suggestion? > Can packages including their EClass elements get unloaded while EPackage > elements containing references to such classes remain loaded? If not, this > should work fine. If yes, the marker adapter on such a package may need to be > invalidated as the referenced classes get unloaded. I assume that Ecore may not > support proper unloading of an EClass that is referenced by an EReference in > some other EPackage where this reference is resolved. It would need to turn > into a proxy. Do we have to assume that this happens? The Ecore philosphy is that the eClass() relationship is special. It is assumed that meta-models are invariant, so any external change results in all eClass()es going to a proxy that is never resolved. Allowing dynamic evolaution of meta-models is something that I would like the pivot model to support one day. (In reply to comment #8) > Axel: Your comments confuse me. Are you confusing bug 329167 where we are > introducing EAnnotation adapters with bug 333241 which is where I propose > performance enhancement of OppositeEndFinder; no adapters at all. Yes, I confused the bug references. Yet, I still find it a useful idea to consider caching the hidden opposites in adapters of the classes on which they can be resolved. This also makes things independent of the EPackage.Registry. It obviously becomes affordable to create a new DefaultOppositeEndFinder each time one is needed as the opposites are no longer cached in the DefaultOppositeEndFinder instance. The remaining challenge: how to identify new EPackage objects that got resolved into the EPackage.Registry? I remember https://bugs.eclipse.org/bugs/show_bug.cgi?id=273989 that asks for events raised by the EPackage.Registry for this case, but it seems to have stalled for some reason. Generally, this default implementation uses the entire registry as scope for finding hidden opposite references. I agree that this is not ideal for all cases, particularly since it will resolve all package descriptors in the registry, once trying to find an opposite. However, most of that seems to be due to the nature of its specification. I see one possible improvement without changing its spec: when a hidden opposite is to be looked up, the lookup can at least first check the already loaded packages before trying to resolve any yet unresolved descriptors. This would avoid some unnecessary package resolving. Different OppositeEndFinder implementations are possible, of course, with different specs. Our Query2OppositeEndFinder may fire a query2 request looking for hidden opposite references and hence wouldn't need to load EPackages. Its scope is identified as a regular query2 scope. A trivial OppositeEndFinder implementation could be defined as not finding any opposite ends at all. That's blazingly fast and predictable ;-). Slightly more usefully, an implementation could be provided that looks for hidden opposites only in those EPackages already resolved in a given Registry. That makes things about as predictable as using the ECrossReferenceAdapter: purely random. > (In reply to comment #7) > > According to Ed's proposal, this patch uses adapters on the EClass elements to > > cache hidden opposites. EPackage elements for which contained hidden opposites > > were already cached are marked by an according adapter. > > I don't understand this. Certainly not Ed W's intention. Has Ed M made a > suggestion? See above, I confused the bug #s you referred to. Yet, an interesting idea to cache the hidden opposites in an adapter. > > Can packages including their EClass elements get unloaded while EPackage > > elements containing references to such classes remain loaded? If not, this > > should work fine. If yes, the marker adapter on such a package may need to be > > invalidated as the referenced classes get unloaded. I assume that Ecore may not > > support proper unloading of an EClass that is referenced by an EReference in > > some other EPackage where this reference is resolved. It would need to turn > > into a proxy. Do we have to assume that this happens? > > The Ecore philosphy is that the eClass() relationship is special. It is assumed > that meta-models are invariant, so any external change results in all > eClass()es going to a proxy that is never resolved. I wasn't referring to the eClass() relationship but to the eType reference from an EReference to its EClassifier. Those may cross package boundaries and therefore may be proxyfied. This is not related to metamodel evolution but the load/resolve state of EPackages. A package that contains EReference elements can be loaded and those EReferences' eType references may be proxies referring to other yet unloaded/unresolved package contents (EClass elements). I would assume that when these get navigated, the packages references get loaded/resolved. Or maybe all dependencies get loaded together with the dependents? Anyway, the question was about whether such dependency packages could be unloaded/unresolved during a registry's life time. 3.1.0 target so that Mylyn shows this in the 'active' Bugzilla query. (In reply to comment #9) > Yet, I still find it a useful idea to > consider caching the hidden opposites in adapters of the classes on which they > can be resolved. This also makes things independent of the EPackage.Registry. Adapters are certainly possible (and I prefer them to WeakHash...), but we need to be sure that they are safe and necessary. Remember that the Ecore meta-model is provided in 'pre-compiled' Java class form in a singleton plugin that is shared by all modeling applications, therefore any change by application A is shared by application B. Failure to honour this lies at the root of strange bugs with ATL/QVTo/Acceleo/Modisco that go away when you restart Eclipse. Now, if both A and B are OCL-based, we must be sure that the A-B sharing is appropriate. If sharing is based on the total global registry (and updated when the global registry updates) then sharing probably is valid, subject to suitable synchronization mutexes. But, anything based on the total global registry is unscalable and so probably unacceptable as default behabiour. And, the global registry has no update notification mechanism and no synchronization mutexes. So I conclude that shareable adapters are unsafe. > Generally, this default implementation uses the entire registry as scope for > finding hidden opposite references. I agree that this is not ideal for all > cases, particularly since it will resolve all package descriptors in the > registry, once trying to find an opposite. However, most of that seems to be > due to the nature of its specification. > > I see one possible improvement without changing its spec: when a hidden > opposite is to be looked up, the lookup can at least first check the already > loaded packages before trying to resolve any yet unresolved descriptors. This > would avoid some unnecessary package resolving. I'm not clear what the specification is. It seems to me that if my OCL contains let a : Pkg::A in a.b then it is mandatory for Pkg to be in the 'imported' set of meta-model packages of the OCL environment. This requires Pkg to be in a application-specific EPackage.Registry, unless this is part of a richer OCL context that support an 'import Pkg' capability. If Pkg::A::b exists then 'a.b' is easily resolved. However if 'b' is a hidden opposite, where is it? You are 'generously' searching everywhere you can think of. I consider that the user is obliged to provide appropriate 'import's, so I think that it is a user responsibility to add the appropriate package to resolve 'b' to the application-specific EPackage.Registry. cf. JDT only sees declarations for imported packages, and JDT quickfixes sometimes only offer suggestions from required bundles. Since opposite functionality is only interesting when used in OCL expressions, the required functionality is fully discovered at compile time. At run time, it does not matter if the actual models use instances from a larger set of meta-models with many more potential opposites. These extra opposites are not used by the OCL so can (and desirably should) be ignored. The underlying problem here is that OMG OCL's import capability is unspecified, and Eclipse OCL's import capability is limited (and actually different between UML and Ecore bindings). I would like to partition this problem to avoid the confusion. Let the opposite support assume that the user has adequately populated an application-specific EPackage.Registry, perhaps as part of a MetaModelManager. Let separate work on import functionality enable this adequate population to be achieved. (In reply to comment #11) Yes, if multiple differently-configured OppositeEndFinder are in use, sharing caches is not possible. Therefore I agree that the cache should at best be shared for equal configurations / scopes. An EPackage.Registry may be one way to scope an OppositeEndFinder. We may go as far as specifying that---in order to avoid the obvious scalability issue that comes with loading all registered EPackages---packages not yet resolved are not considered part of the scope. This only means that subsequent resolving of those EPackages means an on-the-fly scope change for an existing OppositeEndFinder using that registry. Therefore it may be preferable to specify a DefaultOppositeEndFinder's scope as a set of resolved EPackage objects. We can additionally provide a simple way of extracting just the resolved EPackages from an EPackage.Registry. This avoids implicitly triggering the loading of an EPackage's plugin. Regarding the caching, hidden opposites for DefaultOppositeEndFinders using equal sets of EPackages as scope can be stored in a cache shared by these DefaultOppositeEndFinders. > cf. JDT only sees declarations for imported packages, and JDT quickfixes > sometimes only offer suggestions from required bundles. True for forward references declared in the scope of some class. Not so straightforward for a "Find references" query or for a "Find Type." Those queries are either traversing scopes/imports backwards or even apply to the entire workspace. > Let the opposite support assume that the user has adequately populated an > application-specific EPackage.Registry, perhaps as part of a MetaModelManager. See above. I think a simple Set<EPackage> should do, shouldn't it? > Let separate work on import functionality enable this adequate population to be > achieved. Yes, that corresponds with the idea I brought forward above regarding the population of a Set<EPackage> from the already resolved EPackages from an existing EPackage.Registry. If you agree, I will rework the DefaultOppositeEndFinder API correspondingly. Created attachment 185930 [details]
No adapters, no implicit loading of EPackage bundles
I partly reverted the changes of the previous patch 185866, no longer using adapters for caching. Instead, a DefaultOppositeEndFinder maintains its own cache of opposite references. To avoid excessive creation of such caches, I went back to using a WeakHashMap, keyed by the registry. After all, it's not that bad. For all I know, a WeakHashMap removes its entries upon access when they are no longer strongly referenced. That's pretty much what we want here.
This implementation no longer implicitly loads EPackage bundles. Instead, it requires clients to ensure that before a request all EPackages required for the request have been resolved in the registry originally passed to the opposite end loader.
The remaining cost added to a no-opposites implementation is that whenever the analyzer has to assume that something may be defined by a hidden opposite the registry's set of resolved packages is compared to the finder's package cache, and new packages are cached on demand. Given that this only happens during compile time and only if references are not found as forward references, this seems acceptable to me.
Created attachment 185931 [details]
No adapters, no implicit loading of EPackage bundles
Same as 185930, only as workspace and not as git patch
-1 There is no point optimising a cache that caches the wrong information. Bug 333241 must be addressed first. (In reply to comment #15) > -1 > > There is no point optimising a cache that caches the wrong information. Bug > 333241 must be addressed first. The two bugs are closely related. Please look at the attachment and the attachment's comment. It should address the concern voiced by 333241. What is cached and how it's cached and what acts as a cache key should be fixed by one patch. https://bugs.eclipse.org/bugs/attachment.cgi?id=185931 should be that. It would be helpful if you reviewed it, also against the background of 333241 (which came after this, so I guess it would be fair to first resolve this one and then see if this also addresses 333241 to your satisfaction). Sorry; I missed the subtlety that although getLoadedPackages still traverses the entire registry it leaves unloaded packages unloaded. This is much better; it avoids MDT/OCL committing the crime of loading the entire registry, but still leaves MDT/OCL susceptible to excess work if someone else has done so. But, is it guaranteed that all required packages have been loaded, and why do we need to cache so much? Surely it is sufficient to start cache population using the EClassifier argument of either findOppositeEnds or getAllOppositeEnds. There is no need for a DefaultOppositeEndFinder to have a package registry at all. I'm hoping for an initial algorithm such as: eClassifier argument provides an initial ePackage repeat ePackage cross reference analysis to find more ePackages till no more ePackages found then for each referenced ePackage create a cache of hidden opposites. And an update algorithm (if really needed) such as: if eClassifier argument's ePackage in cache do nothing else repeat above incrementally for new packages The above will perhaps impose the cost of about two packages of analysis on typical applications; much less than the 10 for a minimally loaded EMF or the hundred for a significantly loaded modeling enivironment. If the cache is very little to do with the EPackage.Registry, there is little point associating it with the registry. If you have per-package caches, then these could be realised as adapters on the EPackage, since as a context-independent information item it can be shared across applications. In particular, an empty cache for Ecore will save many applications re-scanning Ecore for opposites. (In reply to comment #17) > But, is it guaranteed that all required packages have been loaded, and why do > we need to cache so much? Not guaranteed; as the Javadoc states, this then defines the behavior of the DefaultOppositeEndFinder; clients who want packages to be considered need to ensure they are resolved in the registry used by the finder. > Surely it is sufficient to start cache population using the EClassifier > argument of either findOppositeEnds or getAllOppositeEnds. There is no need for > a DefaultOppositeEndFinder to have a package registry at all. > > I'm hoping for an initial algorithm such as: > > eClassifier argument provides an initial ePackage > repeat ePackage cross reference analysis to find more ePackages till no more > ePackages found How would you do this cross reference analysis for EPackage usage? Of course, if that's possible then what you propose is very useful. I'm just not aware of a good, efficient way to find out about (reverse) package usage relations. > The above will perhaps impose the cost of about two packages of analysis on > typical applications; much less than the 10 for a minimally loaded EMF or the > hundred for a significantly loaded modeling enivironment. Agreed, under the prerequisite above (efficient cross reference analysis for EPackages). Let me know how to do this and I'll update the patch accordingly. > If the cache is very little to do with the EPackage.Registry, there is little > point associating it with the registry. If you have per-package caches, then We need some sort of Collection<EPackage> at least in order to define some search scope where hidden opposites shall be looked for. A Registry and its resolved EPackages serve as a useful default that in particular has minimal impact on existing APIs. If an explicit package list were to be assembled, it would somehow need to be passed into the DefaultOppositeEndFinder and I would wonder where it would come from. That's why I think that for the *Default*OppositeEndFinder an EPackage.Registry makes for the most useful package collection provider and therefore also for the best cache key. > these could be realised as adapters on the EPackage, since as a > context-independent information item it can be shared across applications. In > particular, an empty cache for Ecore will save many applications re-scanning > Ecore for opposites. This could lead to a two-level cache population strategy. First, check if all packages in scope have been cached. For those that haven't, check if the hidden opposites they define have already been extracted and cached in an adapter on the EPackage. If so, use. If not, extract hidden opposites and cache in adapter on EPackage; then update cache in DefaultOppositeEndFinder with this info. Created attachment 186106 [details] DefaultOppositeEndFinder as MetaModelManager (In reply to comment #18) > How would you do this cross reference analysis for EPackage usage? Of course, > if that's possible then what you propose is very useful. I'm just not aware of > a good, efficient way to find out about (reverse) package usage relations. Mmm. Telepathically? The primary problem is that there are old behaviours involving default construction and use of the global registry. Inadequate delegation in EcoreEnvironment.findPackage makes this usage the only one that supports package name paths well. Analysis of the global registry for opposites has undesirable corollaries, so opposite behaviour must be suppressed for the global registry. It is arguable that the overheads of opposites should also be suppressed for existing users. A cache for opposite analysis is desirable and difficult to locate. The attached patch solves all these problems. OppositeEndFinder is no longer a facility of an EcoreEnvironment, rather an EcoreEnvironment uses a 'MetaModelManager' (which for API compatibility reuses the EPackage.Registry interface). A traditional EPackageRegistryImpl may therefore be used as a 'MetaModelManager'. EcoreEnvironment tests the 'MetaModelManager' for the OppositeEndFinder interface wherever opposite functionality needs consideration. DefaultOppositeEndFinder now implements EPackage.Registry to satisfy the existing interface and delegates to an actual registry probably shared with a ResourceSet via resourceSet.setPackageRegistry(new DOEF(resourceSet.getPackageRegistry())). The user may therefore create a 'MetaModelManager' and reuse it as many times as required by passing it as the 'registry' construction argument. The new patch eliminates one new class, an API filter and many of the EcoreEnvironment changes. The API is substantially less changed for existing usages. The new opposite functionality is only available if a 'MetaModelManager' is explicitly created (see the JUnit tests). Since the 'MetaModelManager' is reusable it is its own cache. Since DefaultOppositeEndFinder is only used when requested, we can optimise it for opposites without needing to compromise with traditional functionality. (In reply to comment #19) > Created attachment 186106 [details] > DefaultOppositeEndFinder as MetaModelManager This means it doesn't suffice to implement the OppositeEndFinder interface to use it. Instead, if I read your patch correctly, clients have to implement their own class that implements Registry, probably delegates to one, and additionally implements OppositeEndFinder, then uses this---as you call it---MetamodelManager as the Registry to pass to the EcoreEnvironmentFactory from where it ultimately gets passed to the EcoreEnvironment. Combined with the necessity to cast a Registry to an OppositeEndFinder this seems complicated and less clean. It obfuscates the additional functionality optionally to be provided by the registry. The patch rolls back my previous change where in updateOppositeCache() I ensured that no packages would get resolved just for finding an opposite. Since the registry to which the DefaultOppositeEndFinder delegates with your latest patch would have to be identical to the registry used by the EcoreEnvironment, there would be no way to have packages considered by the regular OCL functionality (e.g., for forward navigation with on-demand package bundle loading) but not for eagerly caching opposite ends. I suspect this is not what we want. About the elimination of which new class are you talking? Created attachment 186152 [details] Improved to-OppositeEndFinder locality (In reply to comment #20) > This means it doesn't suffice to implement the OppositeEndFinder interface to > use it. Instead, if I read your patch correctly, clients have to implement > their own class that implements Registry, probably delegates to one, and > additionally implements OppositeEndFinder, then uses this---as you call > it---MetamodelManager as the Registry to pass to the EcoreEnvironmentFactory > from where it ultimately gets passed to the EcoreEnvironment. Yes. This enables us to offer good performance to all three types of clients. a) old users of the global package registry incur the cost of an 'instanceof' during construction and some failing 'if (oppositeEndFinder != null)' tests for property lookups. Very little proportional cost. b) old users of a custom registry incur the same very small costs. c) new users requiring hidden opposite support must explicitly populate a DefaultOppositeEndFinder to request this additional functionality. Please remember that AFAIK there are no tools planned for Indigo that provide hidden opposite EAnnotations, so in practice you are the only user in c). > > Combined with the necessity to cast a Registry to an OppositeEndFinder this > seems complicated and less clean. It obfuscates the additional functionality > optionally to be provided by the registry. The attached patch restores the EcoreEnvironment.oppositeEndFinder which migrates the instanceof tests to the constructor making the subsequent usage much clearer. (I also made registry final; it was private; how could it change?) > > The patch rolls back my previous change where in updateOppositeCache() I > ensured that no packages would get resolved just for finding an opposite. This was intentional. Since the DefaultOppositeEndFinder is explicitly requested we can expect users (you) to configure it appropriately. There are indeed two sets of packages to consider. One that must be analyzed for hidden opposite EAnnotations, and another (overlapping) that provides packages in which names may be looked up. Having identified the larger MetaModelManager role, it is certainly possible to introduce a MetaModelManager interface, a manadatory AbstractMetaModelManager minimal implementation of which DefaultOppositeEndFinder is just one possible realisation. MetaModelManager could provide additional API to define the packages for hidden opposite analysis, distinct from the inherited EPackage.Registry API for lookup scope. I see this patch as a major step forward that rectifies the bad effects of the original Bug 251621 commit on old users and allows you to evolve the new API independently. However since this API is liable to be obsoleted by the pivot model, I am not convinced that it is actually worth introducing explicit MetaModelManager classes whose integrity is undermined by API compatibility. > About the elimination of which new class are you talking? EcoreEnvironmentFactoryInterface. (In reply to comment #21) > Created attachment 186152 [details] > Improved to-OppositeEndFinder locality Let me try to summarize my understanding of the cumulative patch 186152. EcoreEnvironment has Registry and OppositeEndFinder members. Instead of making OppositeEndFinder construction explicit in createOppositeEndFinder() where it's also re-defineable in subclasses, you mangle it into the Registry passed to the environment and extract it from there. You removed the OppositeEndFinder member from EcoreEnvironmentFactory and EcoreEvaluationEnvironment. Instead, EcoreEnvironmentFactory adapts to OppositeEndFinder, and adaptation works by checking if the Registry implements OppositeEndFinder. DefaultOppositeEndFinder therefore has to implement Registry. The packages in this registry are the ones in which hidden opposites are looked up. The way you changed things, this at the same time defines the set of packages in which OCL looks up names. You mentioned yourself that these scopes may overlap but generally are distinct. The DefaultOppositeEndFinder loads all packages of its registry upon the first hidden opposite request. Since with this, DefaultOppositeEndFinder has to provide all packages in its registry that shall be available for name lookup, all of those will be loaded upon the first hidden opposite lookup. I suppose this is not what you want. I can't see the benefit of eliminating the OppositeEndFinder members. It cleanly encoded the difference between the registry used for name lookup and the scope for looking up hidden opposites. It also lets these two things vary independently which I think is important. Let's get back to your core point: don't load large amounts of packages for hidden opposite lookup. I agree. And my last patch provides that, keeping the two scopes for name lookup and opposite end lookup different variation points. It avoids adaptation based on instanceof as well as casts. Additionally, you claim performance issues for existing users because DefaultOppositeEndFinder checks for necessary cache updates. I would find it appropriate if you backed your claim and the corresponding change request on some data. We have quite extensive OCL tests and numerous quite complicated expressions in our Impact Analyzer setup. The recent changes in this area haven't noticeably impacted our stuff at all. Is it possible that there may be no noticeable performance penalty at all when measured together with an entire parser run? The key point, though, is that we have to make a choice whether to support hidden opposites by default or not. If we do, we additionally have to decide for which default scope we want to support them. With your proposal, hidden opposites by default won't be supported because the default Registry used doesn't implement the OppositeEndFinder interface. If someone annotates their Ecore metamodels with Property.oppositeRoleName, this won't have any effect on a default configuration of MDT/OCL. Before we make this decision, I'd really like to understand the performance implications better, quantitatively. My preference is to support hidden opposites by default, accepting that there is a (as I assume: miniscule) performance "penalty" to be paid, mostly for the first lookup of some feature, as this is when caches are filled. With default usage, only the default Registry will be used, so caching happens once only, and as long as the default Registry survives the DefaultOppositeEndFinder will, too (by effect of the WeakHashMap per-Registry caching). No additional package loading takes place implicitly, so scalability is no longer the issue. The per-lookup "penalty" is to scan registry keys for resolved, yet un-cached packages. If (hopefully after some quantitative analysis) we still come to the conclusion that this is too much an overhead, the next best option in my opinion is to decide that by default hidden opposites are not supported. However, my preference for implementing this default is to have createOppositeEndFinder() assign null to the oppositeEndFinder member. This way, subclasses can easily and cleanly adjust exactly the hidden opposite lookup behavior. I performed some rough micro-benchmarking of the DefaultOppositeEndFinder updateOppositeCache functionality, so we have an idea of what we're talking about. I ran the ecore OCL tests (standalone) on a T510 (~2.5GHz core speed). Aggregating over all test cases, the following numbers resulted: Calls: 4075 Max: 2416320ns Min: 11751ns Avg: 51344ns Populating the cache takes 2.4ms, average lookups add cache updating overhead of 51us on average. For the plugin test execution mode I get: Calls: 3948 Max: 840829ns Min: 13777ns Avg: 41266ns I was surprised to see that performance is even *better* for plugin mode where I had expected to see more Ecore models in the registry. I'll also run this through a profiler to see the overall share of execution time that we're discussion here. Created attachment 186238 [details]
Screenshot of performance benchmark in profiler
The attached profiler screenshot shows that more than 80% of a lookupProperty call are spent for things other than opposite end lookup (280ms vs. 46.8ms). Additionally, I'll attach a second screenshot that shows that under oclExpressionCS 1.18s are consumed altogether. So, for parsing the expressions occurring in the test cases, we're talking about 4% that the opposite lookup consumes. I argue that this is so little that it's outweighed by the benefit of having a default implementation for hidden opposites that's always available and doesn't need special configuration. Also, according to the profiler, a total of 1.49s are spent under AbstractEvaluationVisitor.visitExpression(...), and the tests presumably evaluate most expressions only once after they have been compiled. In non-test scenarios, evaluation time would exceed compilation time more significantly even. But even for once-only execution, compilation and evaluation add up to 2.67s out of which the 47ms lookup time are ~1.8%.
Created attachment 186239 [details] Screenshot of performance benchmark overall parsing See comment of attachment 186238 [details] Axel: You fail to appreciate the relevance of Ecore opposites. a) it is out of specification for Ecore functionality b) it is out of specification for (EMOF) OCL functionality c) it is not supported by any Eclipse modeling tools it is therefore really of interest only to one business unit at SAP. d) the integration of the proposed implementation will be obsoleted by the migration to the Pivot model In https://bugs.eclipse.org/bugs/show_bug.cgi?id=251621#c29 this contribution was left as an available unsupported functionality. At ESE I misunderstood that relevant core functionality was to be incorporated into EMF and so agreed that we would endeavour to get the opposite support in provided this had minimal time and cost impact. Both of these provisions have totally failed, I consider that this activity has delayed my other OCL work by at least three weeks, and the current committed code has significant risks of incurring costs for existing users. To resolve Bug 333241 I want to see a way to minimise the cost impact with minimal further time commitments. Yes, these are only risks, and while your benchmarks show limited cost, they represent your well-configured application usage. My concern is that users with hundreds, possibly thousands of models in the registry may suddenly encounter surprises. The best way to avoid these surprises is to avoid the imposed changes. I do not have time to proceed with further iterations. Please address only what is unuseable with attachment 186152 [details], since I consider it very satisfactorily minimises cost for existing users, ensuring that the cost of configuring and using the DefaultOppositeEndFinder is incurred by its knowing users so that the support you want is available. [The package registry functionality is not 'mangled' into DefaultOppositeEndFinder. A new MetaModelManager is defined that provides package functionality and opposite functionality. If this code was to persist into 4.0 we could change the EPackage.Registry API to reflect the larger usage, which would make it clearer that the role of model management was associated with a single object that could be shared by many root environments. For the current single user the package registry functionality is merged into DefaultOppositeEndFinder; as I commented in #21 making DefaultOppositeEndFinder just for you "allows you to evolve the new API independently"] (In reply to comment #26) Ed, > Axel: You fail to appreciate the relevance of Ecore opposites. > > a) it is out of specification for Ecore functionality > b) it is out of specification for (EMOF) OCL functionality > c) it is not supported by any Eclipse modeling tools > > it is therefore really of interest only to one business unit at SAP. sorry, but I can't follow this line of reasoning. Regarding a), Ecore neither specifies OCL. We certainly don't take this as an argument to kill the OCL project. Regarding b), this is IMHO not fully correct. The property specifying the opposite role name has been adopted by the OMG. Issue 12800 to my knowledge has been resolved. Pete Rivett confirmed to me that the resolution provided through David Frankel has been adopted. With this, the tag Property.oppositeRoleName is officially in EMOF and hence should be supported by the MDT/OCL implementation. Regarding c), other aspects such as how OCL delegates and their expressions are to be specified are similarly based on annotations that have to be filled using the default Ecore tools. Besides, how can you say that only one business unit at SAP is interested? It's just the only one you know of. So please don't generalize based on vague assumptions and remember that you once brought this whole point forward yourself. > d) the integration of the proposed implementation will be obsoleted by the > migration to the Pivot model We know that and discussed that at length, also with you. We all agreed that hidden opposites are a solution that works for the current OCL implementation. If the Pivot model works and gets adopted, many other things may need deprecation as well, not just hidden opposites. And until we have the working Pivot model OCL implementation in place we can easily use hidden opposites today. > In https://bugs.eclipse.org/bugs/show_bug.cgi?id=251621#c29 this contribution > was left as an available unsupported functionality. > > At ESE I misunderstood that relevant core functionality was to be incorporated > into EMF and so agreed that we would endeavour to get the opposite support in > provided this had minimal time and cost impact. > > Both of these provisions have totally failed, I consider that this activity I can't tell what you misunderstood at ESE. I think we always talked about two alternative implementations of this functionality. One would require an Ecore extension, the other an OCL extension. After I prototyped both approaches we all agreed that it would be better to have the cost be where the key benefit is: OCL. I'm sorry that reviewing our contribution has delayed your work. > delayed my other OCL work by at least three weeks, and the current committed > code has significant risks of incurring costs for existing users. To resolve Please prove. I find it inacceptable to hold back my proposed improvement by a vague statement about possible risks. I've provided benchmark results that you haven't even seriously considered nor discussed. I claim they disprove your suspicion. > Bug 333241 I want to see a way to minimise the cost impact with minimal further > time commitments. Well, then just +1 my patch :-) > Yes, these are only risks, and while your benchmarks show limited cost, they > represent your well-configured application usage. My concern is that users If you try to imply that this is a best case analysis I have to say that this is not the case. I've made it clear that to the contrary we're only considering one-time evaluation which is far worse than more realistic scenarios where expressions---once compiled---are typically evaluated many times. Please be more concrete and demonstrate *why* you think that this is a best-case (or even better-than-average-case) analysis. > hundreds, possibly thousands of models in the registry may suddenly encounter > surprises. The best way to avoid these surprises is to avoid the imposed > changes. Sorry, I don't follow this because if generalized it probably means we can't improve OCL anymore because any change may imply a risk. The hidden opposites feature has significant benefit, discussed many times. We've always striven to keep the risk low. Where there was doubt, we benchmarked. I've shown the cost for scanning for hidden opposites. Please use these numbers to explain why you think this is an unbearable risk. > I do not have time to proceed with further iterations. Please address only I propose a patch. You don't like it for reasons we're still disagreeing about. You propose a patch which I don't like for reasons we're still disagreeing about. You're asking me to move forward with your patch and tell me you don't have time for further iterations. I don't think it works this way. It would really help more if we could argue the technical aspects. > is unuseable with attachment 186152 [details], since I consider it very satisfactorily > minimises cost for existing users, ensuring that the cost of configuring and > using the DefaultOppositeEndFinder is incurred by its knowing users so that the > support you want is available. I understand your main concern to be that existing OCL users may be impacted by the (IMHO and according to my benchmarks insignificant) performance penalty of scanning already loaded registry packages for hidden opposites. You'd rather make hidden opposites a non-default feature of the OCL implementation (which I regret as it's an adopted OMG standard). I'll create a variant of patch 185931 which by default leaves oppositeEndFinder==null and therefore doesn't perform any registry package scans at all. In addition, I'll provide a specialized EcoreEnvironmentFactory which, if used for the construction of an OCL instance, uses a DefaultOppositeEndFinder or a client-provided opposite end finder for lookups. I'll put that into the o.e.o.e.opposites package so people can find and use it. This should accomodate your concerns, I hope. I plan on providing the patch later today. Best, -- Axel Created attachment 187129 [details]
Making hidden opposites optional
This patch accomodates Ed's concerns about potential performance implications of hidden opposites lookups. With it, clients have to explicitly use a specialized EcoreEnvironmentFactory, such as the EcoreEnvironmentFactoryWithHiddenOpposites now provided in the o.e.o.e.opposites package. Otherwise, oppositeEndFinder fields in factory and environment remain null and no lookups will occur.
Tests and documentation adjusted accordingly.
Note that patch 187129 also addresses https://bugs.eclipse.org/bugs/show_bug.cgi?id=333241 in that it makes hidden opposites optional unless an environment factory is used that returns a non-null OppositeEndFinder. Yes this addresses 333082 by making the opposite end finder optional, but: Minor issues ------------ EcoreEvaluationEnvironment still has a default DefaultOppositeEndFinder; easily set to null. EcoreEvaluationEnvironment.navigateOppositeProperty can be streamlined, by initialising result with null. An NPE risk on oppositeEndFinder should be eliminated. More significant issue ---------------------- The new patch fails to establish the coherence of the Package Registry and OppositeEndFinder as part of a single object that takes responsibility for providing meta-model related services for one registry and multiple environments. Because you do not have object coherence you have to have a WeakHashMap entry instead, which has needlessly vague garbage collection. My patch had a coherent object and I requested you to address what was unuseable about that patch. One alternative to my composite PackageRegistry+OppositeEndFinder could be to use the EnvironmentFactory as the container for both eliminating the WeakHashMap to associate them. Agreed about the extra use of DefaultOpppsiteEndFinder and the streamlining by initializing result to null. I'm not sure why you think it may constitute an inconsistency if the DefaultOppositeEndFinder uses some other registry for opposite lookup. Besides, I had pointed out other potential inconsistencies with your patch on which I haven't seen a response from you. Given that other OpppsiteEndFinder implementations may choose entirely different strategies for lookup, I'm not convinced we should design it such that the DefaultOppositeEndFinder's registry has to be the same as the one used for package name lookup. I even think I remember one of your comments where you agrees that these loops (opposites vs. package names) may well ufse different scopes. Suggestion: I ensure that if the EcoreEnvironmentFactoryWithHiddenOpposites has a registry, it will use the same for opposite lookup. OK? Created attachment 187391 [details] No WeakHashMap, factory tying registry+OEF together You propose: > One alternative to my composite PackageRegistry+OppositeEndFinder could be to > use the EnvironmentFactory as the container for both eliminating the > WeakHashMap to associate them. EcoreEnvironmentFactoryWithHiddenOpposites is pretty close to this, I think. The trouble with not remembering a DefaultOppositeEndFinder instance is that when it needs to be re-created, the already built-up opposite cache goes to waste. Particularly for the straightforward case of using the EPackage.Registry.INSTANCE this would be annoying since default use of the hidden opposites implementation would probably usually work with this registry. How about removing the WeakHashMap that bothers you from DefaultOppositeEndFinder and introduce an INSTANCE field in DefaultOppositeEndFinder that keeps a single DefaultOppositeEndFinder for the default registry, and an INSTANCE field in EcoreEnvironmentFactoryWithHiddenOpposites that keeps the result of a default constructor invocation of that class? Furthermore, I removed the constructor allowing clients to only pass an OppositeEndFinder without a Registry. This way, as you asked for, the factory plays the role of binding registry and opposite end finder together. (In reply to comment #32) > > One alternative to my composite PackageRegistry+OppositeEndFinder could be to > > use the EnvironmentFactory as the container for both eliminating the > > WeakHashMap to associate them. > > EcoreEnvironmentFactoryWithHiddenOpposites is pretty close to this, I think. I think we've converged on a solution. After writing my final comment I realised that EnvironmentFactory is not the stateless re-useable factory I had been considering: one Factory for many MetaModelManagers each for many Environments. With a stateful Factory there is clearly just one {EnvironmentFactory+EPackage.Registry+OppositeEndFinder} (NB not DefaultOppositeEndFinder) for many Environments. So we can indeed treat the EnvironmentFactory[WithHiddenOpposites] the primary 'MetaModelManager' supervising its EPackage.Registry [and a OppositeEndFinder]. ------ We can clarify the API to construct a root Environment as new Environment(factory) rather than new Environment(factory.getRegistry()) environment.setFactory(factory) [so that Environment.factory is final in MDT/OCL 4.0]. If you like this, I am happy to have this new constructor, and perhaps deprecate the old ones and setFactory(). (Subject to empirical discovery that some test/code demonstrates that it is stupid.) (In reply to comment #33) > I think we've converged on a solution. Very cool. > We can clarify the API to construct a root Environment as > > new Environment(factory) > > rather than > > new Environment(factory.getRegistry()) > environment.setFactory(factory) > > [so that Environment.factory is final in MDT/OCL 4.0]. > > If you like this, I am happy to have this new constructor, and perhaps > deprecate the old ones and setFactory(). (Subject to empirical discovery that > some test/code demonstrates that it is stupid.) Happy to create such a patch. However, be aware that it's probably more than one constructor. EcoreEnvironment currently has three constructors, taking the following argument lists: - Registry - Registry, Resource - Environment The constructor taking the parent environment is currently used in EcoreEnvironmentFactory.createEnvironment(parent) where a subsequent call to setFactory happens. The way this is designed it would generally be possible to construct parent and child environment using different factories, therefore also using different registries and opposite end finder pairs. To remain compatible with the existing functionality, we should add a constructor taking (EcoreEnvironmentFactory, Environment) and initialize the child environment's registry and opposite end finder from the environment factory instead of copying the opposite end finder from the parent environment. In addition to the new constructor taking (EcoreEnvironmentFactory) we would then also need (EcoreEnvironmentFactory, Resource). Patch to follow, unless you stop me short because you think three new constructors with three ones deprecated is too big a change. Created attachment 187420 [details]
Add EcoreEnvironment constructors taking factory, deprecate others and setFactory
This patch does the deprecations discussed and introduces the new EcoreEnvironment constructors taking EcoreEnvironmentFactory arguments for consistent registry / oppositeEndFinder initialization. setFactory() is now deprecated. I adjusted all tests that used it accordingly. All tests green.
(In reply to comment #34) > Happy to create such a patch. However, be aware that it's probably more than > one constructor. EcoreEnvironment currently has three constructors, taking the > following argument lists: > - Registry > - Registry, Resource > - Environment [Need to get branch promoted for M5 asap so not had time to review the patch.] Instinmctively, I think there should be just two constructors. The root: either EcoreEnvironment(factory) or EcoreEnvironment(factory, resource) the child: EcoreEnvironment(EcoreEnvironment) with everything else deprecated. I would need to understand the utility of the other constructors to not deprecate. It seems to me that we have evolved from discomfort with the Registry constructor to apprecate the need for the better Factory constructor that hopefully obsoletes old workarounds/misunsderstnadings. Ok, this would only mean that a child environment "inherits" the parent's factory which seems reasonable to me although up to now it was possible to create parent and child through different factories. I'll update the patch later tonight. Created attachment 187457 [details]
Only deprecating two constructors
As discussed, the EcoreEnvironment constructor for child environments now again only takes the parent environment as argument. The child's factory is set to the parent's factory. The two remaining old constructors are deprecated in favor of the two new constructors which take an EcoreEnvironmentFactory as additional argument.
Created attachment 187501 [details]
Another depredcated EcoreEnvironment constructor
Great. We solved the real problem. It's always nice when a change makes things cleaner.
Attached cleans up a number of derived test EcoreEnvironment classes with spurious old arguments and deprecates a further EcoreEnvironment constructor so that we just have the two preferred remnants.
Now that we've identified EnvironmentFactory as the master for the Factory+Registry+OppositeEndFinder trio, I recommend revising DefaultOppositeEndFinder to take an EnvironmentFactory rather than Registry argument.
Then, if you're happy, just go for it. +1.
(In reply to comment #39) > Great. We solved the real problem. It's always nice when a change makes things > cleaner. Cool. > Attached cleans up a number of derived test EcoreEnvironment classes with > spurious old arguments and deprecates a further EcoreEnvironment constructor so > that we just have the two preferred remnants. Good. > Now that we've identified EnvironmentFactory as the master for the > Factory+Registry+OppositeEndFinder trio, I recommend revising > DefaultOppositeEndFinder to take an EnvironmentFactory rather than Registry > argument. That, IMHO, would lead to a cycle. An EnvironmentFactory is supposed to manage the oppositeEndFinder. EcoreEnvironmentFactoryWithHiddenOpposites has it as a final attribute. While the factory manages the relation between both (registry + oppositeEndFinder), the default opposite end finder knows its registry but shouldn't have to know its managing factory. Another issue then would be that the parameterless getInstance() which currently uses the default registry would have to obtain a factory from somewhere which it nor many of its clients don't necessarily have access to. > Then, if you're happy, just go for it. +1. Will commit your latest patch. If you think we need to discuss the parameterization of the DefaultOppositeEndFinder factory methods, I suggest we have another bug on that. Committed Ed's patch https://bugs.eclipse.org/bugs/attachment.cgi?id=187501 plus the documentation changes explaining how to activate hidden opposites. (In reply to comment #40) > > Now that we've identified EnvironmentFactory as the master for the > > Factory+Registry+OppositeEndFinder trio, I recommend revising > > DefaultOppositeEndFinder to take an EnvironmentFactory rather than Registry > > argument. > > That, IMHO, would lead to a cycle. An EnvironmentFactory is supposed to manage > the oppositeEndFinder. EcoreEnvironmentFactoryWithHiddenOpposites has it as a > final attribute. While the factory manages the relation between both (registry > + oppositeEndFinder), the default opposite end finder knows its registry but > shouldn't have to know its managing factory. Another issue then would be that > the parameterless getInstance() which currently uses the default registry would > have to obtain a factory from somewhere which it nor many of its clients don't > necessarily have access to. The EnvironmentFactory is an extensible (via getAdapter) provider of services to the Environment. It was the absence of a factory that made the Environment difficult many releases ago, and the evolution via use of setFactory() rather than constructors that caused us grief in the last few months. The DefaultOppositeEndFinder needs to know the registry, so yes a Registry is the minimum construction argument, but it is not extensible because a registry does not know its factory. If instead a DefaultOppositeEndFinder takes a factory argument it can deduce the registry accordingly and in the future can access further services via the factory. Factory-less operation is awkward, so perhaps a default factory instance should be available to support default oppsite end finders. This might also have enabled me to remove a deprecation on an Environment constructor in OCLUnParser. Closing |