Bugzilla – Full Text Bug Listing
| Summary: | [evaluator] OCL change impact analysis, e.g., for incremental re-validation, should be provided | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | [Modeling] OCL | Reporter: | Axel Uhl <eclipse> | ||||||
| Component: | Core | Assignee: | OCL Inbox <mdt-ocl-inbox> | ||||||
| Status: | CLOSED FIXED | QA Contact: | |||||||
| Severity: | enhancement | ||||||||
| Priority: | P3 | CC: | eclipse, ed, manuel.holzleitner, martin.hanysz, philipp.berger, zechow-tobi | ||||||
| Version: | unspecified | ||||||||
| Target Milestone: | 3.1.0 | ||||||||
| Hardware: | PC | ||||||||
| OS: | Windows XP | ||||||||
| URL: | http://www.axel-uhl.de/ocl_impact_analyzer_20100820.zip | ||||||||
| Whiteboard: | |||||||||
| Bug Depends on: | 329167, 329336, 333082 | ||||||||
| Bug Blocks: | |||||||||
| Attachments: |
|
||||||||
|
Description
Axel Uhl
Find an implementation at http://www.axel-uhl.de/ocl_impact_analyzer_20100820.zip Currently, the implementation still uses an oclToAst bundle which pre-compiles OCL expressions and stores them in the contents of annotations. As we haven't patched the standard OCL invocation/setting/validating delegates, we introduced our own delegates which pull the ASTs from the annotation contents. This required us to use a proprietary source URI for the OCL annotations. This, however, could (and should) easily be changed by extending the default delegates such that they would look for an AST stored in the annotation's contents. Hi Axel Thanks. I haven't reviewed the contribution, but your description makes it clear that it's something that's good to have. I hope that we can exploit this as part of a general evaluation state adapter that keeps track of cached results and cached Java generated from OCL AST generated from OCL text. This could occur late-ish in the Indigo release cycle. Hi Ed, we haven't yet looked into caching evaluation results. This may create another scalability issue when you think about a large number of expressions over many context instances in various resources. We rather chose to do four things: 1) from each OCL expression derive an event filter to use with the event manager, so notifications only need to be handled if they affect an expression's value at all 2) from the notification details, try to prove that the sub-expression immediately affected can't have changed. Example: If we receive a notification that shows that the value of EAttribute X.s went from 'a' to 'b' and we have an expression for context X: self.s = 'abc' then we can conclude that the expression can't have changed based on the change described by the Notification, as for both, old and new feature value the sub-expression evaluates to false. We use a partial evaluator for this which can inject the Notification details into the evaluation stack. 3) If we can't entirely compute old and new value of a sub-expression, we may still be able to propagate deltas through monotonic expressions. For example, if elements are added to the value of a ->select's source expression then the result of the ->select expression either remains constant or contains one or more of the elements added to its source. More such monotony relations exist. We exploit them to push deltas through an expression tree, always conservatively thinking in supersets of the actual change. If a delta propagates into an empty set then we know that no change can have been implied by the original Notification. 4) If 2) and 3) were unable to prove constant results for pre/post state, we trace back from the Notification to self. This sometimes requires navigating references in reverse which creates the dependency on the hiddenopposites stuff and the OppositeEndFinder interface. We do this also for operation calls, including recursive calls. Nasty, obviously, are uses of allInstances() because they don't let us trace back to a single context element for self. We are currently working on a further improvement because with the trace-back to self we still do too much in some cases. As an example, take an IfExp where a change affects the then-part. From the Notification and the then-expression we trace back to the possible context instances for self for which the then-expression may have changed its value based on the Notification. However, knowing the possible values for self it may be possible to evaluate the IfExp condition expression. For those cases where it evaluates to false, any change in the then-expression doesn't affect the IfExp's value. This is a particularly useful improvement in case the "self" variable occurs inside an operation body deep down in a call hierarchy. If we find out the IfExp hasn't changed, we save the bubble-up process that takes us through the call hierarchy to the outermost context for self. We hope to be able to provide this as another contribution or an update to this contribution in a few weeks. Best, -- Axel I see another really useful use case. The text values returned by EditItemProviders have limited ability to determine deep dependencies, so in the Sample Ecore Editor you can easily have a wrong display of base classes. If a) genmodel supported entry of an OCL expression to evaluate the 'name' b) OCL generates reasonable Java for that OCL expression c) your impact analysis recomputes appropriately we get high level edit item definition deep updates I would be delighted if you are able to provide at least the impact analysis/resolution as a complete contribution. Since the utility extends to minimal standalone modeling, I am reluctant to add any further dependencies to MDT/OCL Core. There are two ongoing changes that make integration difficult at present. The move to an Ecore/UML neutral pivot model means that IfExp<'Ecore'>/IfExp<'UML'> will change to perhaps IfExp<Pivot> but more likely to just IfExp. More exact OMG structure and spelling without Ecore/UML confusion. The move to a model driven standard library means that there will be exactly one class per distinct library operation. For consistent extensible evaluation, perhaps even 'if' is evaluated by the corresponding configuration of an If class. This should allow your impact analysis to hang off an e.g. hasImpact() API if you need it. A preview of this structure can be found in the ...library... packages in the MDT_OCL_3_0_0_ReflectiveLibrary branch. Making all 'operations' uniformly callable eliminates the quite significant visitor and type/mode dispatch overheads. You might find that the more uniform structure suits you much better and so if you want to pick up the evaluation you are very welcome. I'm not sure how much resource/enthusiasm you have to commit to MDT/OCL. Continuing the Eclipse Summit Europe discussion after reviewing the PowerPoint slide back for the Impact Analysis and the Event Handler. This looks really cool, so I'd like to take it further. The 'guard' conditions for the notification handlers can of course be expressed in OCL, so the impact analysis can both drive and use the enhanced notifications, and if ALL relevant operations are fully defined in OCL, the 'guard's may have substantial hierarchical complexity. Perhaps OCLinEcore could provide a first class syntax for the underlying EAnnotations. It would be nice if some kind of CompleteOCL / OCLinEcore hybrid could define additional OCL functionality to be injected into generated classes. (In reply to comment #5) Ed, > The 'guard' conditions for the notification handlers can of course be expressed > in OCL, so the impact analysis can both drive and use the enhanced if you talk generally about the event handler, it is currently restricted to a limited set of filters and logical combinations (AND/OR/NOT) thereof. Were you to allow general OCL expressions for event filters, I think you would 1) map EMF Notification objects to classes whose instances can be handled by OCL (Notification is not currently an EObject subclass) 2) need to come up with a way to map arbitrary OCL conditions to the event handler conditions permitted; I don't think this is generally possible because in particular the event manager by design does not allow for recursion and iteration (which OCL does). This would break the assumptions based on which the event manager achieves its performance gains. Besides, may I ask what kind of use cases you have in mind for Notification filter conditions expressed in OCL? > notifications, and if ALL relevant operations are fully defined in OCL, the > 'guard's may have substantial hierarchical complexity. > > Perhaps OCLinEcore could provide a first class syntax for the underlying > EAnnotations. It would be nice if some kind of CompleteOCL / OCLinEcore hybrid > could define additional OCL functionality to be injected into generated > classes. Sorry, I don't understand this last paragraph. Which annotations are you referring to? What do you mean by injecting OCL functionality? Anything beyond defining invariante, derived properties and operation bodies using OCL? Best, -- Axel I'm considering the use case of an EditItemProvider similar to that for the Sample Ecore Editor outline, for which MyClass might compute a 'displayText' of
MyClass -> ocl::cst::ExpCS
When 'ocl' is renamed, I want all affected 'displayText' to update.
So I want to 'inject' a 'displayName' feature that computes
parent.name + '::' + self.name
and a 'displayText' that computes
displayName + superClasses.displayText->splice(' -> ', ', ', '')
[splice(prefix, infix, suffix) concatenates a String OrderedCollection]
These principles are recursive declarations but affect a tree of objects, so that the data-dependent analysis can establish a non-recursive data dependency tree, so that any change identifies what tree values change and/or what trees need rebuilding.
In order to use OCL as a definition language for EditItemProviders, I was envisaging some kind of Complete OCL syntax such as
dynamic context MyClass::displayText : String {
in MyClassEditItemProvider
body : displayName + superClasses.displayText->splice(' -> ', ', ', '');
}
dynamic context MyClass::displayName : String {
in MyClassEditItemProvider
body : container().displayName->splice('', '', '::') + name;
}
'dynamic' signals that Impact Analysis should be hooked up.
'in' somehow, no idea how, indicates where the generated code should be 'injected'/generated.
Perhaps a self.displayText.oclChanged() query could interrogate the prevailing analysis state.
Created attachment 182569 [details] Initial implementation proposal for an OCL Impact Analysis This implementation is partitioned into two key bundles: *.impactAnalyzer and *.impactAnalyzerWithHiddenOpposites. The latter extends the impact analyzer such that it understands the hidden opposites OCL extension proposed by https://bugs.eclipse.org/bugs/show_bug.cgi?id=251621. It is based on an event manager which has also been attached to https://bugs.eclipse.org/bugs/show_bug.cgi?id=329336. In order to progress this we need to start taking on the bureaucratic requirements to make the code ready for IP approval and project integration. Here are some initial comments based on a one-hour review. I'm sorry but bureaucracy is one of the delights of a mature Eclipse project. The ZIP includes the Bug 251621 plugins, which makes review easier for me, but complicates IP approval. After taking on board the comments please ensure that Bug 251621 is self-standing, and that this controbution layers on top of it. All source files (java, properties, and non-trivial xml) must have EPL copyrights ... "SAP and others" with an identified initial API author. If the source file is a clone of another the original copyright attribution should remain with an additional contributing author and a comment indicating the rationale for the copy and subsequent change. All copied code must be reviewed; SyntaxHelperWithHiddenOpposites identifies a contribution by EObject.D.Willink (sic). Cloned files are good for independent development, but need to be revised as patches, where this can be done without causing API problems. All plugins should be 3.1.0, Java SE 1.5, with about.html, build.properties, plugin.properties; check the existing plugins for style. ...editor is the conventional name for auto-generated EMF editor plugins. Other UI contributions are usually ...ui. A patch to the examples-feature is needed to integrate your new plugins. The following are missing: de.hpi.sam.bp2009.company, de.hpi.sam.bp2009.ngpm, de.hpi.sam.bp2009.solution.oclToAst, de.hpi.sam.bp2009.solution.testutils, com.sap.ocl.oppositefinder.query2, de.hpi.sam.bp2009.solution.queryContextScopeProvider, The following are currently not OCL dependencies. I would prefer to avoid them, since there have been cyclic dependency issues. org.eclipse.emf.query.index.ui, org.eclipse.emf.query.index.update Are the eventManager plugins a contribution to EMF or OCL? I suggest plugins, package names based on org.eclipse.ocl.examples.impact Is a separate plugin for impact with opposites necessary? Perhaps it is sufficient to just modularize in packages. Once this code matures in MDT/OCL 4.0 there will be an integrated opposites solution. The impactAnalyzer tests introduce 5 new libraries. How many of these are in Orbit? Are those that are not in Orbit really necessary? Please ensure that the code can be reviewed by: a) Install Eclipse 3.6.1 Modeling EPP. b) Import ocl.psf (from the Project Page) c) Import tests.psf, test-tools.psf (from org.eclipse.ocl/releng/psfs) d) Apply your patches e) Import your ZIPs Bug 251621 has now been submitted for IP approval. Please update this patch to accommodate and include 251621. a) a ZIP of org.eclipse.ocl.examples.impact... plugins b) if necessary, a patch to org.eclipse.ocl... temporarily including Bug 251621. (In reply to comment #10) > Bug 251621 has now been submitted for IP approval. > > Please update this patch to accommodate and include 251621. > > a) a ZIP of org.eclipse.ocl.examples.impact... plugins > b) if necessary, a patch to org.eclipse.ocl... temporarily including Bug > 251621. There's a minor dependency on https://bugs.eclipse.org/bugs/show_bug.cgi?id=329167 regarding the way the OCL ASTs are fetched from the Ecore model for analysis. It would be slightly easier to first have 329167 included, then assemble this one. What do you think? There is much more here than I have time to review, but since it's going into examples I don't need to. If I get to try to exploit this to optimize CST to ASG updates in the editor, I may start to comment more constructively. It seems that the original 428 file contribution has expanded to 1085 files. Is this entirely due to the extra ...ngpm suite of test models. "ngpm" is not an obvious name and could be combined with "company". I think the use of test models could be usefully considered project wide. On the one hand sharred test models are useful, on the otherhand test models are sometimes deliberately stupid and this may be lost in sharing. The test models are presumably 100% unedited, so perhaps CVS need have just the *.ecore and *.genmodel and we have a build step that autogenerates the models into e.g. /gen-tst-src Overall I think this could be quite close: Copyrights - every sample file I looked at was good. But some easy fixes: Completeness: there are 33 references to "de.hpi" some causing build failures. there are 26 references to "com.sap" - all in comments. Java: the manifests refer to Java 1.6 not 1.5. Fixing this gives about 7 'spurious' @override errors. There are about 20 unused imports to remove, and about 30 unchecked casts to resolve; I prefer an @SuppressWarnings on a cast assignment to a local variable rather than on the entire function. SapClass doesn't seem like a very appropriate name. Libraries: IP contributions cannot contain jars. Any not in Orbit will need separate CQs; it seems all bar cglib are in Orbit. Fortunately they're only declared by test plugins and.... Deleting cglib.jar causes no errors; presumably redundant. Deleting asm-all.jar causes no errors; presumably redundant. Deleting objenesis.jar causes no errors; presumably redundant. Deleting easymock*.jar causes no errors; presumably redundant. org.eclipse.emf.query.index is imported by package rather than bundle and not available from emf.query - easily fixed by deleting them since they are not used. Misc: pom.xml is not needed within Eclipse. about.html's are missing. ---- The attachment size can probably be fixed by checking the large file checkbox. Attaching in 2 parts: eventmanager/impact test, or as non-test/test. Minor name changes for the non-test contribution. o.e.o.e.eventmanager o.e.o.e.impactanalyzer o.e.o.e.impactanalyzer.ui (.editor is normally an EMF editor) For the test contribution: 5 test plugins seems excessive. I would prefer to see a self-standing o.e.o.e.tests.eventmanager and a o.e.o.e.tests.impactanalyzer that perhaps depends on o.e.o.e.tests.eventmanager. [I think o.e.o.e.xtext.tests was a mistake, o.e.o.e.tests.xtext would be better.] Perhaps an o.e.o.e.tests.impactanalyzer.benchmark is justified. I haven't studied it. What is the basis of benchmark? an industry standard; if not then a write up defining the benchmark as a prelude to a stnadrda one day could be useful. Try to avoid package names that do not obviously extend the bundle name e.g. benchmark. ---- Some documentation would be good. Perhaps a tutorial, but documentation is traditionally written after the M7 freeze, so no panic. Hi Ed, thanks for the review. Here are my comments. > It seems that the original 428 file contribution has expanded to 1085 files. As you observed, the main reason is that I now attached our core test metamodel which contains many OCL expressions, some of them seriously large and complex. Never mind its contents and names (such as SapClass etc.). Changing it without good OCL/Ecore refactoring support would be a pain in the neck and IMHO wouldn't help much in this context. BTW: ngpm stands for "next-generation programming model", a little experiment we conducted with our language workbench. The many OCL expressions in it may also be of good use for other use cases. > The test models are presumably 100% unedited, so perhaps CVS need have just the > *.ecore and *.genmodel and we have a build step that autogenerates the models > into e.g. /gen-tst-src True. However that works. Personally, I'd rather prefer easier setup over larger test bundles. > But some easy fixes: > > Completeness: > there are 33 references to "de.hpi" some causing build failures. Removed. There was one particular use of the oclToAst plugin which I've replaced by ValidationBehavior.getInvariant and SettingBehavior.getFeatureBody. This makes this functionality depend on https://bugs.eclipse.org/bugs/show_bug.cgi?id=329167. I hope this is ok. The bundles should compile now when first applying the latest 329167 patch. > there are 26 references to "com.sap" - all in comments. Either removed because they were outdated anyway or updated to new classnames / package names. > Java: the manifests refer to Java 1.6 not 1.5. Fixing this gives about 7 > 'spurious' @override errors. > There are about 20 unused imports to remove, and about 30 unchecked casts to > resolve; I prefer an @SuppressWarnings on a cast assignment to a local variable > rather than on the entire function. I did the 1.5 downgrade and removed the offending @Override tags. Although I have my Eclipse set to warn me of unchecked generics operations, I can't see any related warnings. Do you have more details on this? > SapClass doesn't seem like a very appropriate name. See above. Shouldn't matter, I hope. > Libraries: > IP contributions cannot contain jars. Any not in Orbit will need separate CQs; > it seems all bar cglib are in Orbit. Fortunately they're only declared by test > plugins and.... > Deleting cglib.jar causes no errors; presumably redundant. > Deleting asm-all.jar causes no errors; presumably redundant. > Deleting objenesis.jar causes no errors; presumably redundant. > Deleting easymock*.jar causes no errors; presumably redundant. Removed. Some remnants from earlier tests. Good catch :-) > org.eclipse.emf.query.index is imported by package rather than bundle and not > available from emf.query - easily fixed by deleting them since they are not > used. Removed. Good catch too. > Misc: > pom.xml is not needed within Eclipse. Removed pom.xml files. > about.html's are missing. To be delivered later. > The attachment size can probably be fixed by checking the large file checkbox. > > Attaching in 2 parts: eventmanager/impact test, or as non-test/test. > > > Minor name changes for the non-test contribution. > o.e.o.e.eventmanager > o.e.o.e.impactanalyzer > o.e.o.e.impactanalyzer.ui (.editor is normally an EMF editor) > > For the test contribution: > 5 test plugins seems excessive. We have more stuff that benefits a lot from this structure. Updated plugin uploaded to http://www.axel-uhl.de/impactanalyzer.zip > > I would prefer to see a self-standing o.e.o.e.tests.eventmanager and a > o.e.o.e.tests.impactanalyzer that perhaps depends on > o.e.o.e.tests.eventmanager. [I think o.e.o.e.xtext.tests was a mistake, > o.e.o.e.tests.xtext would be better.] > > Perhaps an o.e.o.e.tests.impactanalyzer.benchmark is justified. I haven't > studied it. What is the basis of benchmark? an industry standard; if not then a > write up defining the benchmark as a prelude to a stnadrda one day could be > useful. > > Try to avoid package names that do not obviously extend the bundle name e.g. > benchmark. > > ---- > > Some documentation would be good. Perhaps a tutorial, but documentation is > traditionally written after the M7 freeze, so no panic. Created attachment 185492 [details] Impact Analyzer with adjusted package names Trying to upload this in addition to http://www.axel-uhl.de/impactanalyzer.zip (In reply to comment #12) > I would prefer to see a self-standing o.e.o.e.tests.eventmanager and a > o.e.o.e.tests.impactanalyzer that perhaps depends on > o.e.o.e.tests.eventmanager. [I think o.e.o.e.xtext.tests was a mistake, > o.e.o.e.tests.xtext would be better.] > > Perhaps an o.e.o.e.tests.impactanalyzer.benchmark is justified. I haven't > studied it. What is the basis of benchmark? an industry standard; if not then a > write up defining the benchmark as a prelude to a stnadrda one day could be > useful. Question on naming of test bundles: so far I was under the impression that test bundles are typically named after <name-of-bundle-to-test>.tests? (the trailing "s" sometimes being omitted). Examples: - org.eclipse.ocl.tests - org.eclipse.ocl.uml.tests - org.eclipse.ocl.ecore.tests So I'd like to stick to this convention and have - org.eclipse.ocl.examples.eventmanager.tests - org.eclipse.ocl.examples.impactanalyzer.tests Ok? Event manager bundles committed to HEAD. In the remaining bundles there is a dependency on https://bugs.eclipse.org/bugs/show_bug.cgi?id=329167 and https://bugs.eclipse.org/bugs/show_bug.cgi?id=333082. In particular, it would really be helpful to be able to use DefaultOppositeEndFinder.getInstance(Registry) which is currently still under debate. Also, being able to use ValidationBehavior.INSTANCE.getInvariant to obtain the AST in a well-defined form, exploiting commont caching facilities, is desirable. Here is my IP statement 1. I authored 100% of the content I am contributing 2. I have the rights to contribute the content to Eclipse 3. I contribute the content under the Eclipse Public License Cheers, Phil My IP statement: I authored 100% of the content I am contributing. I have the rights to contribute the content to Eclipse. I contribute the content under the Eclipse Public License. Regards, Manuel My IP statement: 1. I authored 100% of the content I am contributing 2. I have the rights to contribute the content to Eclipse 3. I contribute the content under the Eclipse Public License Best regards, Martin My IP statement: 1. I authored 100% of the content I am contributing 2. I have the rights to contribute the content to Eclipse 3. I contribute the content under the Eclipse Public License Best regards, Tobias Committed to CVS based on approval for CQ 4850. Closing all bugs resolved in Indigo. |