Community
Participate
Working Groups
Build Identifier: 20100917-0705 When executing an operation that has an OCL-specified body, or when computing a derived property whose derivation expression was given using OCL, and also when performing a validation, any compiled version of an OCLExpression that is found in the contents of the standard EAnnotation used to specify the expression should be considered. This is partly the case for the delegates using those expressions that are compiled during a session and therefore attached in this way to the EAnnotation's contents. The OCL MDT evaluation should consistently make use of these compiled expressions. This saves redundant re-compilation and will allow for the storage of pre-compiled OCL expressions even in .ecore metamodel resources, avoiding repeated OCL compilation altogether. It will also help moving towards true refactorings across Ecore models that include the OCL expressions as they are then (at least also) stored as models rather than just strings. We attach a proposed patch for review. Reproducible: Always
Created attachment 182124 [details] Patch for consistently using compiled OCL in EAnnotations
There seem to be two changes and no tests. a) support for multiple delegate domains and consequently opposites This is a useful functionality that needs careful analysis identifying the problems and solutions. I'm not happy seeing it woven into this patch. b) support for compiled OCL This is also useful but there are problems. How are synthesized types such as Sequence(String) persisted to avoid dangling references? I suggest an EPackage EAnnotation to define them. Similar problems may exist for tuple types and everything else for which the environment maintains extra packages. Alternatively Bug 297394 needs to be implemented to provide the persistence and type equality resolved within the OCL implementation. How is the relative validity of String and AST representations determined? The AST has excellent chances of surviving simple refactorings. So we have the dilemma that currently the String when present is always treated as valid; however if an AST is also present, and a refactoring has been performed, the String is broken although the AST is correct. For pedantic compatibility it is necessary to first try the String and compare it with the AST, accepting the AST only when the String is invalid. It would be good to establish a protocol for marking the String as invalid. Is this going to need an annotationDelegate to install an adapter to watch its AST or is it guaranteed that the refactoring will warm up the package delegate registrations? This problem might be mitigated if the AST was a child rather than sibling annotation of the detail containing the OCL String, since this is clearly new API; assuming that no one else ever uses the annotation contents is dangerous. To be investigated: does Ecore validation validate ASTs within EAnnotations? What is necessary to ensure that it does? ----------------------- I'm not keen to expand the Ecore-only support in MDT/OCL, and this change will conflict with introduction of neutral Ecore/UML support and migration of the OMG AST to one that is UML-aligned (e.g supports iterators, Complete OCL, OclAny, ...). I would therefore prefer to see this happen in an alternative perhaps OCL_DELEGATE_DOMAIN_URI+"/Ecore" domain, and to address any API issues that make extension/re-use of the existing OCL_DELEGATE_DOMAIN_URI hard. This will allow the OCL_DELEGATE_DOMAIN_URI to support UML-aligned OMG ASTs once that AST is defined.
Hi Ed, agreed on a). That was a mistake on our part. Of course, InvocationBehavior is already specific to the default OCL delegate domain, so no other domains need to be considered here. I've removed the loop from the patch. Regarding b), I understand your concerns. If both AST and string are remembered there is danger of both running out of sync. We had assumed that the redundant storage in EAnnotation contents already happens in the Environment.defineOperation and Environment.defineAttribute methods. However, a closer look reveals that in those cases the string doesn't originate from the annotation. One may argue thatthe string that leads to the creation of the Constraint passed to defineOperation/defineAttribute may as well run out of sync with the AST stored in the contents. If I understand the existing code correctly, there is also caching taking place, e.g., in OCLValidationDelegate.invariantOperationMap where compiled invariants are cached: if ((query == null) & !invariantOperationMap.containsKey(invariant)) { try { query = createQuery(expression); } finally { invariantOperationMap.put(invariant, query); } } I assume this will also run out of sync if OCL expression strings on an Ecore model change dynamically. Therefore, I don't think it is currently a supported use case, and so we should focus on what happens if OCL strings change between separate sessions where the Ecore model is freshly loaded, clearing any caches and starting with empty annotation contents. The AST will usually be added transiently to the annotation. It will be parsed again when the Ecore model is loaded again. Hence, the patch should support the same set of use cases as the existing code. Additionally, you raise the valid question how elements created by the parser and so far stored in the transient ocl:///oclenv.ecore resource will be referenced. This becomes an issue when persistently storing the AST in the Ecore resource which the patch doesn't imply so far. We have implemented a solution to this problem which clones the elements referenced from the ocl:///oclenv.ecore resource to the referencing Ecore resource and updates the references. As you already suggest, we use an EAnnotation with source "oclTypes" on each EPackage in the contents of which we then store the elements cloned from ocl:///oclenv.ecore. With this the OCL expressions stored in the .ecore resources are "self-contained." Of course, with this approach, as you describe, it becomes necessary to update or remove the AST after changing the string representation. Since we have no convincing solution for this issue yet other than telling users of our String2AST-converter to run the converter after changing the strings, we haven't proposed this as an extension yet. Of course, if anyone is interested, we can publish this String2AST converter here too. Updated patch follows. Best, -- Axel
Created attachment 182131 [details] Patch for consistently using compiled OCL in EAnnotations This version of the patch relies on InvocationBehavior applying only to the default OCL delegate domain.
(In reply to comment #3) > I assume this will also run out of sync if OCL expression strings on an Ecore > model change dynamically. Therefore, I don't think it is currently a supported > use case.... It is certainly the case that EMF forbids change of a meta-model while its model is loaded, so thereis nothing to break here. > The AST will usually be added transiently to the annotation. This solves most of the long-term API concerns. However, how do you make an Annotation transient without introducing a new EClass whose child OCLExpression is transient? If the Annotation is unsaveable, I'm not sure what the point of this patch is. --- Philosophical question. Should an Ecore meta-model persist the OCL Concrete or Abstract syntax? I'm inclined towards a compact CS, since anyone concerned with efficiency will genmodel and that is where the AS should be provided (as inline Java). If Ecore has the compact OCL CS, then we need a refactoring hook to ensure that the CS updates.
Sorry for not being clear about the use of the word "transient." What I meant was that as long as we add an EObject to the contents of an EAnnotation attached to a loaded Ecore model according to what we know it won't affect the persistent representation of this Ecore model unless someone chose to try to store this Ecore model again from its loaded/registered state into some persistent .ecore resources. This to us didn't seem a practical use case which is why we consider these annotations transient. Regarding the difference that the patch makes, we particularly assume that for derived properties specified in OCL and for operations with a body specified in OCL the OCL evaluation visitor will be able to evaluate much faster if the ASTs are cached in the annotation contents after having been parsed for the first time. Today, as far as we understand the code and its execution, property and operation body definitions not performed with define...(...) calls but by attaching the OCL string as EAnnotation will cause EMF reflection to happen which then invokes the OCL delegates. Only there will the parsing / caching / cache lookup take place. So even without an ultimate solution to the synchronization problem between string and AST representation should this result in a noticeable performance gain. I suggest that we have a separate discussion on synchronizing persistently-stored ASTs with persistently-stored OCL strings. As a quick note on that problem, I think that reasonable textual editing capabilities that operate directly on the AST can be provided so that eventually the use of the text string may serve for documentation purposes or similar.
The attached 'patch' cannot be applied. Please use Create Patch to create a patch of your code with respect to CVS HEAD.
Created attachment 182780 [details] Current and Proposed Evaluation Sequences A set of sequence charts and summary explaining our understanding of the current procedures during evaluating OCL-defined features, together with a proposal for how to change it as (more or less; updates to be submitted soon) proposed by the patch.
(In reply to comment #7) > The attached 'patch' cannot be applied. > > Please use Create Patch to create a patch of your code with respect to CVS > HEAD. Sorry, that was a Git patch. I'll produce an updated CVS patch soon.
I'm just now starting to wonder what the semantics of the two different annotation source URIs is: Environment.OCL_NAMESPACE_URI ("http://www.eclipse.org/ocl/1.1.0/OCL") OCLDelegateDomain.OCL_DELEGATE_URI ("http://www.eclipse.org/emf/2002/Ecore/OCL") where the latter is constructed as org.eclipse.emf.ecore.EcorePackage.eNS_URI + "/OCL". The Environment.OCL_NAMESPACE_URI is currently used by EcoreEnvironment to store and look up compiled versions of OCL expressions in annotations with this source URI. If we choose to compile OCL expressions provided as strings in the delegate's annotation, the contents of which annotation would be the better place to cache the compilation results: Environment.OCL_NAMESPACE_URI or OCLDelegateDomain.OCL_DELEGATE_URI? Environment.OCL_NAMESPACE_URI would come with the benefit that EcoreEnvironment.getDefinition(...) implementation wouldn't need to change. However, it would spread the delegate-specific stuff across two annotations. Suggestions?
Created attachment 182806 [details] Current and Proposed Evaluation Sequences updated version describing the evaluation sequences for operation invocations through the EMF interface and operation call expression evaluations coming from the OCL evaluation visitor, both for current and proposed solution.
Created attachment 182809 [details] Patch for consistently using compiled OCL in EAnnotations Updated patch, as an Eclipse workspace patch; slightly refactored and in accordance with the evaluation sequences described in attachment 182806 [details]. Tested to apply properly on HEAD from 2010-11-10; tests all green.
Environment.OCL_NAMESPACE_URI ("http://www.eclipse.org/ocl/1.1.0/OCL"), which I was not properly aware of, is the basis for the Eclipse 3.3 vintage attempt to integrate OCL and Ecore validation. It is not officially deprecated; it's just few people including the MDT/OCL committers know how it works. OCLDelegateDomain.OCL_DELEGATE_URI("http://www.eclipse.org/emf/2002/Ecore/OCL") was chosen for the Eclipse 3.6 integration. I suggest ignoring Environment.OCL_NAMESPACE_URI. Bug 329928 raised to start work on deprecating it officially.
(In reply to comment #13) > Environment.OCL_NAMESPACE_URI ("http://www.eclipse.org/ocl/1.1.0/OCL"), which I > was not properly aware of, is the basis for the Eclipse 3.3 vintage attempt to > integrate OCL and Ecore validation. It is not officially deprecated; it's just > few people including the MDT/OCL committers know how it works. > > OCLDelegateDomain.OCL_DELEGATE_URI("http://www.eclipse.org/emf/2002/Ecore/OCL") > was chosen for the Eclipse 3.6 integration. > > I suggest ignoring Environment.OCL_NAMESPACE_URI. Bug 329928 raised to start > work on deprecating it officially. Ok. Then how to proceed? My suggestion: integrate the patch as proposed, then let's work on refactoring all usages of Environment.OCL_NAMESPACE_URI into usages of OCLDelegateDomain.OCL_DELEGATE_URI. Alternatively, I can offer to produce a patch that changes all usages of Environment.OCL_NAMESPACE_URI into OCLDelegateDomain.OCL_DELEGATE_URI immediately. Let me know what you prefer.
Tests all pass. Good. No new tests. Perhaps some scope for a couple more; perhaps not; it's all the same. The changes are very simple and so seemingly problem free. However why isn't OCLValidationDelegate.constraintNameMap/invariantOperationMap obsolete? Your cache is better so why do we now have alternatives? OCLValidationDelegate.constraintNameMap is not API, so I think we can clean up. After some pain last year we realised that it is helpful to a reviewer to minimize textual changes, so please avoid reformatting. If reformatting is appropriate (a separate undiscussed issue) the time to do it is between successful review and commit. getCachedExpression/cacheExpression might be snappier than getExpressionFromAnnotationsOf/saveExpressionInAnnotation. Eliminate all new use of Environment.OCL_NAMESPACE_URI and so simplify a little. SettingBehavior.getFeatureBody/InvocationBehavior.getOperationBody ------------------------------ Looks like modelElement.getEAnnotation(OCLDelegateDomain.OCL_DELEGATE_URI); should be factored out to avoid three repeats. saveExpressionInAnnotation(structuralFeature, constraint); could usefully be invoked to save an InvalidLiteralExp if the parser fails and so avoid reparses. ValidationBehavior ------------------------------ Indentation is adrift. AbstractDelegatedBehavior ------------------------------ My personal preference is to organize class contents by static/no-static, then by fields/constructors/operations, then by alphabetically. However Christian seemed to use some kind of chronological/arbitrary order. We perhaps need a project discussion to see if we care. This was my code so it was in my order. AbstractDelegatedBehavior.getExpressionFromAnnotationsOf ------------------------------ The multi-cache relies on details being ordered, which they are. However I think it would be quicker to just maintain the list of EAnnotation.eContents so that you just do a linear search for a) instanceof Constraint b) Constraint.getStereotype() = constraintKey which in practice will fail immediatedly for old-style usage and nearly always pass at the first test for new-style. a.setEModelElement(modelElement); Not seen this before. Maybe because it only works when there is an unambiguous parent type. I think modelElement.getEAnnotations().add(a); is preferred. OCLValidationDelegate..getExpressionFromAnnotationsOf ------------------------------ This duplicates AbstractDelegatedBehavior.getExpressionFromAnnotationsOf. Make AbstractDelegatedBehavior.getExpressionFromAnnotationsOf public static.
Created attachment 182988 [details] Patch for consistently using compiled OCL in EAnnotations Updated patch with consistent use of OCLDelegateDomain.OCL_DELEGATE_URI, removed obsolete caches and additional test cases that verify successful use of cached expressions. Also, ordering of methods was adjusted to project conventions.
(In reply to comment #15) Ed, > The changes are very simple and so seemingly problem free. However why isn't > OCLValidationDelegate.constraintNameMap/invariantOperationMap obsolete? Your > cache is better so why do we now have alternatives? > OCLValidationDelegate.constraintNameMap is not API, so I think we can clean up. I now removed those obsoleted caches in the updated patch. This forced me to change one test case which now receives a subtly-changed error message when a non-Boolean constraint is being parsed. > getCachedExpression/cacheExpression might be snappier than > getExpressionFromAnnotationsOf/saveExpressionInAnnotation. Changed. > Eliminate all new use of Environment.OCL_NAMESPACE_URI and so simplify a > little. Done. > SettingBehavior.getFeatureBody/InvocationBehavior.getOperationBody > ------------------------------ > > Looks like modelElement.getEAnnotation(OCLDelegateDomain.OCL_DELEGATE_URI); > should be factored out to avoid three repeats. Eliminated one repetition. > saveExpressionInAnnotation(structuralFeature, constraint); could usefully be > invoked to save an InvalidLiteralExp if the parser fails and so avoid reparses. Suggestion: let's do that as a follow-up bugzilla if we find it's worthwhile. > ValidationBehavior > ------------------------------ > > Indentation is adrift. Better now? > AbstractDelegatedBehavior > ------------------------------ > > My personal preference is to organize class contents by static/no-static, then > by fields/constructors/operations, then by alphabetically. However Christian > seemed to use some kind of chronological/arbitrary order. We perhaps need a > project discussion to see if we care. This was my code so it was in my order. Done. (Personally, I use the Eclipse outline view to order things alphabetically if I want to and otherwise try to keep logically-related code close together to minimize scrolling; but even that is increasingly giving way to F3 and Search/Hierarchy view. Minimalistically, I find it useful to have field declarations at the top, then constructors, then methods.) > AbstractDelegatedBehavior.getExpressionFromAnnotationsOf > ------------------------------ > > The multi-cache relies on details being ordered, which they are. However I > think it would be quicker to just maintain the list of EAnnotation.eContents so > that you just do a linear search for > a) instanceof Constraint > b) Constraint.getStereotype() = constraintKey > which in practice will fail immediatedly for old-style usage and nearly always > pass at the first test for new-style. Won't work for multiple constraints on the same class because there the stereotype is "invariant" for all Constraint elements. I now enforce same positions in contents and details by padding contents with a DUMMY_CONSTRAINT if I have to (null is not permissible in EList). I assert this with an additional test case for which I added a second invariant to Employee. > a.setEModelElement(modelElement); > > Not seen this before. Maybe because it only works when there is an unambiguous > parent type. I think > > modelElement.getEAnnotations().add(a); > > is preferred. Done. > OCLValidationDelegate..getExpressionFromAnnotationsOf > ------------------------------ > > This duplicates AbstractDelegatedBehavior.getExpressionFromAnnotationsOf. Make > AbstractDelegatedBehavior.getExpressionFromAnnotationsOf public static. Done.
We did some measurements to analyze how much performance differs depending on whether an expression is found in the getDefinition() or getBody(...) call, where getDefinition() comes first and fails if the OCL expression is cached as body. See the results below. It indicates that in those cases where performance matters (many calls), the penalty for the extra getDefinition() call approaches zero. 10 runs with one call each: Time using stereotype 'body' : 107788.0 ns Time using stereotype 'definition': 3647.0 ns Using getDefinition() is 29.555250891143405times faster Time using stereotype 'body' : 4458.0 ns Time using stereotype 'definition': 2431.0 ns Using getDefinition() is 1.8338132455779514times faster Time using stereotype 'body' : 3647.0 ns Time using stereotype 'definition': 2432.0 ns Using getDefinition() is 1.4995888157894737times faster Time using stereotype 'body' : 3647.0 ns Time using stereotype 'definition': 2431.0 ns Using getDefinition() is 1.5002056766762648times faster Time using stereotype 'body' : 9319.0 ns Time using stereotype 'definition': 2432.0 ns Using getDefinition() is 3.8318256578947367times faster Time using stereotype 'body' : 3647.0 ns Time using stereotype 'definition': 2431.0 ns Using getDefinition() is 1.5002056766762648times faster Time using stereotype 'body' : 3647.0 ns Time using stereotype 'definition': 2431.0 ns Using getDefinition() is 1.5002056766762648times faster Time using stereotype 'body' : 3647.0 ns Time using stereotype 'definition': 2431.0 ns Using getDefinition() is 1.5002056766762648times faster Time using stereotype 'body' : 4052.0 ns Time using stereotype 'definition': 2431.0 ns Using getDefinition() is 1.6668037844508432times faster Time using stereotype 'body' : 3647.0 ns Time using stereotype 'definition': 2432.0 ns Using getDefinition() is 1.4995888157894737times faster 10 runs with 10 calls each: Time using stereotype 'body' : 123185.0 ns Time using stereotype 'definition': 10536.0 ns Using getDefinition() is 11.6918185269552times faster Time using stereotype 'body' : 11346.0 ns Time using stereotype 'definition': 8915.0 ns Using getDefinition() is 1.2726864834548515times faster Time using stereotype 'body' : 10536.0 ns Time using stereotype 'definition': 8915.0 ns Using getDefinition() is 1.1818283791362871times faster Time using stereotype 'body' : 10536.0 ns Time using stereotype 'definition': 8915.0 ns Using getDefinition() is 1.1818283791362871times faster Time using stereotype 'body' : 11347.0 ns Time using stereotype 'definition': 9319.0 ns Using getDefinition() is 1.2176199163000323times faster Time using stereotype 'body' : 10535.0 ns Time using stereotype 'definition': 8915.0 ns Using getDefinition() is 1.1817162086371285times faster Time using stereotype 'body' : 10535.0 ns Time using stereotype 'definition': 9320.0 ns Using getDefinition() is 1.1303648068669527times faster Time using stereotype 'body' : 17829.0 ns Time using stereotype 'definition': 9320.0 ns Using getDefinition() is 1.9129828326180258times faster Time using stereotype 'body' : 11347.0 ns Time using stereotype 'definition': 9320.0 ns Using getDefinition() is 1.217489270386266times faster Time using stereotype 'body' : 10536.0 ns Time using stereotype 'definition': 9320.0 ns Using getDefinition() is 1.130472103004292times faster 10 runs with 20 calls each: Time using stereotype 'body' : 130075.0 ns Time using stereotype 'definition': 17424.0 ns Using getDefinition() is 7.465277777777778times faster Time using stereotype 'body' : 19855.0 ns Time using stereotype 'definition': 16614.0 ns Using getDefinition() is 1.1950764415553148times faster Time using stereotype 'body' : 17830.0 ns Time using stereotype 'definition': 16614.0 ns Using getDefinition() is 1.07319128445889times faster Time using stereotype 'body' : 18235.0 ns Time using stereotype 'definition': 16208.0 ns Using getDefinition() is 1.1250616979269497times faster Time using stereotype 'body' : 19045.0 ns Time using stereotype 'definition': 16614.0 ns Using getDefinition() is 1.146322378716745times faster Time using stereotype 'body' : 17830.0 ns Time using stereotype 'definition': 16209.0 ns Using getDefinition() is 1.100006169412055times faster Time using stereotype 'body' : 17424.0 ns Time using stereotype 'definition': 16209.0 ns Using getDefinition() is 1.0749583564686285times faster Time using stereotype 'body' : 17830.0 ns Time using stereotype 'definition': 16614.0 ns Using getDefinition() is 1.07319128445889times faster Time using stereotype 'body' : 76991.0 ns Time using stereotype 'definition': 20666.0 ns Using getDefinition() is 3.725491144875641times faster Time using stereotype 'body' : 17830.0 ns Time using stereotype 'definition': 16614.0 ns Using getDefinition() is 1.07319128445889times faster 10 runs with 100 calls each: Time using stereotype 'body' : 295402.0 ns Time using stereotype 'definition': 76586.0 ns Using getDefinition() is 3.8571279346094585times faster Time using stereotype 'body' : 84690.0 ns Time using stereotype 'definition': 74965.0 ns Using getDefinition() is 1.1297272060294805times faster Time using stereotype 'body' : 75776.0 ns Time using stereotype 'definition': 75370.0 ns Using getDefinition() is 1.0053867586572907times faster Time using stereotype 'body' : 76991.0 ns Time using stereotype 'definition': 85096.0 ns Using getDefinition() is 0.9047546300648679times faster Time using stereotype 'body' : 98873.0 ns Time using stereotype 'definition': 91984.0 ns Using getDefinition() is 1.0748934597321274times faster Time using stereotype 'body' : 94415.0 ns Time using stereotype 'definition': 91984.0 ns Using getDefinition() is 1.0264285093059662times faster Time using stereotype 'body' : 92795.0 ns Time using stereotype 'definition': 92390.0 ns Using getDefinition() is 1.0043835912977594times faster Time using stereotype 'body' : 100088.0 ns Time using stereotype 'definition': 93605.0 ns Using getDefinition() is 1.069259120773463times faster Time using stereotype 'body' : 94415.0 ns Time using stereotype 'definition': 90363.0 ns Using getDefinition() is 1.044841362061906times faster Time using stereotype 'body' : 91985.0 ns Time using stereotype 'definition': 90363.0 ns Using getDefinition() is 1.0179498245963503times faster 10 runs with 1000 calls each: Time using stereotype 'body' : 2034587.0 ns Time using stereotype 'definition': 895526.0 ns Using getDefinition() is 2.27194631981651times faster Time using stereotype 'body' : 393059.0 ns Time using stereotype 'definition': 320526.0 ns Using getDefinition() is 1.2262936548049144times faster Time using stereotype 'body' : 385360.0 ns Time using stereotype 'definition': 306748.0 ns Using getDefinition() is 1.2562755095387745times faster Time using stereotype 'body' : 322146.0 ns Time using stereotype 'definition': 292160.0 ns Using getDefinition() is 1.1026355421686747times faster Time using stereotype 'body' : 258122.0 ns Time using stereotype 'definition': 260149.0 ns Using getDefinition() is 0.9922083113907799times faster Time using stereotype 'body' : 257312.0 ns Time using stereotype 'definition': 256502.0 ns Using getDefinition() is 1.003157870114073times faster Time using stereotype 'body' : 258122.0 ns Time using stereotype 'definition': 262580.0 ns Using getDefinition() is 0.9830223170081499times faster Time using stereotype 'body' : 259743.0 ns Time using stereotype 'definition': 258933.0 ns Using getDefinition() is 1.0031282223586797times faster Time using stereotype 'body' : 259743.0 ns Time using stereotype 'definition': 258122.0 ns Using getDefinition() is 1.0062799761353158times faster Time using stereotype 'body' : 258122.0 ns Time using stereotype 'definition': 256502.0 ns Using getDefinition() is 1.0063157402281464times faster
Created attachment 185032 [details] Regenerated patch against current CVS Head
(In reply to comment #19) > Created an attachment (id=185032) [details] > Regenerated patch against current CVS Head Thanks for updating the patch. One more thing (the other minor thing we noticed I'll raise as a separate bug 332400). In EcoreEnvironment we found that getDefinition() should only return constraints with stereotype "definition" and getBodyCondition() should only return constraints with stereotype "body". This results in the following additional modification which I'll attach as a new cumulative patch next to 185032. From our perspective it can deprecate 185032. Your view? diff --git org.eclipse.ocl.ecore/src/org/eclipse/ocl/ecore/EcoreEnvironment.java org.eclipse.ocl.ecore/src/org/eclipse/ocl/ecore/EcoreEnvironment.java index 805adc4..f7f7f04 100644 --- org.eclipse.ocl.ecore/src/org/eclipse/ocl/ecore/EcoreEnvironment.java +++ org.eclipse.ocl.ecore/src/org/eclipse/ocl/ecore/EcoreEnvironment.java @@ -54,6 +54,7 @@ import org.eclipse.ocl.ecore.internal.UMLReflectionImpl; import org.eclipse.ocl.expressions.ExpressionsPackage; import org.eclipse.ocl.expressions.Variable; import org.eclipse.ocl.expressions.impl.ExpressionsPackageImpl; +import org.eclipse.ocl.helper.ConstraintKind; import org.eclipse.ocl.types.OCLStandardLibrary; import org.eclipse.ocl.types.TypesPackage; import org.eclipse.ocl.utilities.OCLFactory; @@ -542,26 +543,27 @@ public class EcoreEnvironment } } } else { - String expr = EcoreUtil.getAnnotation(typedFeature, OCLDelegateDomain.OCL_DELEGATE_URI, InvocationBehavior.BODY_CONSTRAINT_KEY); + String expr = ann.getDetails().get(UMLReflection.DEFINITION); if (expr == null) { return null; } try { Helper helper = OCL.newInstance().createOCLHelper(); - if (feature instanceof EOperation) { - EOperation op = (EOperation) feature; - helper.setOperationContext((EClassifier) ((EOperation) feature).eContainer(), op); - } else if (feature instanceof EStructuralFeature) { + if (feature instanceof EStructuralFeature) { helper.setContext((EClassifier) ((EStructuralFeature) feature).eContainer()); + } else if (feature instanceof EOperation) { + EOperation op = (EOperation) feature; + helper.setOperationContext((EClassifier) op.eContainer(), op); + } else { + return result; } - result = helper.createBodyCondition(expr); + result = helper.createConstraint(ConstraintKind.DEFINITION, expr); ann.getContents().add(result); } catch (ParserException e) { throw new OCLDelegateException(e.getLocalizedMessage(), e); } } } - return result; } @@ -576,16 +578,26 @@ public class EcoreEnvironment } ETypedElement typedFeature = feature; EAnnotation ann = typedFeature.getEAnnotation(OCLDelegateDomain.OCL_DELEGATE_URI); - if (ann == null) { - // expects getOperationBody(...) to store the AST in the OCL annotation's contents - InvocationBehavior.INSTANCE.getOperationBody(OCL.newInstance(), feature); - ann = typedFeature.getEAnnotation(OCLDelegateDomain.OCL_DELEGATE_URI); - } - if ((ann != null) && !ann.getContents().isEmpty()) { - for (EObject o : ann.getContents()) { - if ((o instanceof Constraint)&& UMLReflection.BODY.equals(((Constraint) o).getStereotype())) { - result = (Constraint) o; - break; + if (ann != null) { + if (!ann.getContents().isEmpty()) { + for (EObject o : ann.getContents()) { + if ((o instanceof Constraint)&& UMLReflection.BODY.equals(((Constraint) o).getStereotype())) { + result = (Constraint) o; + break; + } + } + } else { + String expr = ann.getDetails().get(InvocationBehavior.BODY_CONSTRAINT_KEY); + if (expr == null) { + return null; + } + try { + Helper helper = OCL.newInstance().createOCLHelper(); + helper.setOperationContext((EClassifier) feature.eContainer(), feature); + result = helper.createBodyCondition(expr); + ann.getContents().add(result); + } catch (ParserException e) { + throw new OCLDelegateException(e.getLocalizedMessage(), e); } } }
Created attachment 185034 [details] Cumulative patch considering stereotypes Relating to my last comment https://bugs.eclipse.org/bugs/show_bug.cgi?id=329167#c20 this cumulative patch adds checks for proper stereotypes in getDefinition and getBodyCondition.
Created attachment 185383 [details] Cumulative patch considering stereotypes, after 251621 Re-produced the patch after updating the 251621 patch from CVS
Created attachment 185460 [details] Simplified cache not affecting additional features There seems to be confusion between Environment.OCL_NAMESPACE_URI which is used to cache Complete OCL additional features and OCLDelegateDomain.OCL_DELEGATE_URI for the new cache. I've elimated all the spurious changes to EcoreEnvironment which avoids the need for a change to one of the DelegatesTests. It may be that the EcoreEnvironment.getBodyExpression override is needed but no test demonstrates it. I've changed the cache to an OCLExpression cache just as the name indicates, and supported and exploited caching of a null OCLExpression as an InvalidLiteralExp to allow a parse failure to be cached avoiding a reparse refailure. Use of an OCLExpression cache eliminates the need for the new createConstraint methods allowing createQuery to be used as before. Your createConstraint had distinct classifier/operation contexts that suggests that you might have fixed a bug. A few casts and alternate control flow parths simplified. SettingBehavior.getFeatureBody now only caches the found key, with derived preferred as per the OCL spec. EvaluationSequences.mdl excluded.
I've applied your modified patch and have run our impact analyzer tests and a few other things against it. Without looking at the details, it seems your changes broke a few handful of our tests in add-on modules. I have to find out what exactly went wrong and which of our assumptions may have been flawed.
Areas to pay particular attention to: Any use of Complete OCL defined features for which the two distinct EAnnotation source namespaces were confused. Any caching of null 'Constraint's, which is now supported as a cached cannot resolve. A re-resolve of a constraint that throws an exception fiorst time, does not throw an exception second time, just returns an invalid literal. Any late provision of constraints after the cache has already been partially set up. In particular, I think that declaring the keys "a b" with only constraint for "b" then "a" used to ignore the subsequent "a".
(In reply to comment #25) > Areas to pay particular attention to: > > Any use of Complete OCL defined features for which the two distinct EAnnotation > source namespaces were confused. and, much the most likely, the EcoreEnvironment.getBodyExpression override which I deleted since it was not needed by any test.
(In reply to comment #26) I think I found the problem. If no body is found for an operation, e.g., for a stdlib operation such as oclAsType(...), you store null in the cache. Upon the next lookup you interpret this as an InvalidLiteral. This, I think, is wrong. You have to return null in those cases again. I'll try to produce and test an updated patch.
Created attachment 185483 [details] Fixes bug for non-OCL-specified operations (e.g. stdlib) The previous patch Ed presented had the problem that when calling InvocationBehavior.getOperationBody(...) or SettingBehavior.getFeatureBody(...) more than once for a feature that doesn't have an OCL definition, the first call returns null, subsequent calls return an "invalid" literal. This causes trouble and is inconsistent. The patch above fixes this by distinguishing between cases where exceptions occur during parsing the expression in which case an "invalid" literal is cached, and cases where null results in which case null will be returned consistently. I added two test cases asserting this behavior.
Created attachment 185489 [details] Tests caching of failure and fixes multiple nulls in cache Yes. The cache should only be populated if an attempt has been made to parse something to be cached. The attached achieves this more easily by opening the try block later. Adding a test for this revealed a new bug. The annotation contents list may not have duplicates, (multiple NULL_CONSTRAINTs were reduced to a single one). Fixed by using distinct instances of a distinct class.
Ed, I tested https://bugs.eclipse.org/bugs/attachment.cgi?id=185489 in our environment and it works well. Is IP review necessary? Best, -- Axel
Adolfo: I'm happy that this contribution is largely patch or copy and paste and so less than 200 new lines so we don't need IP approval. Are you happy to commit it (after I tidy up a couple of commented out functions)? Axel: Can you please state that the contribution was entirely written by you (or me) and that SAP makes it available for EPL? Detecting patch differences is a pain. I've taken to taking a copy of e.g. o.e.o.ecore from the old patch before reverting to CVS and applying the new patch. A diff between o.e.o.ecore's is then quite easy. Your comments suggest you missed my comment #29 where I fixed a bug too. In comment #18 you produced performance results demonstrating the benefits of a change to EcoreEnvironment. This change is no longer present. I don't understand what you were measuring, so I don't know whether your enhancement has been lost or not. If it's still worth having, please raise a new bug for it with a patch once this one has gone through.
(In reply to comment #31) > Axel: Can you please state that the contribution was entirely written by you > (or me) and that SAP makes it available for EPL? I hereby state that the work on this patch is original work by Tobias Hoppe and myself. SAP generally grants permission to publish this code under EPL. Tobias has agreed, too. He's on this bug's Cc list. I'll repeat Tobias' e-mail he sent with his consent: "I confirm, that the code I contributed is my original work and hasn't been copied from code under other licenses. I agree to make my contributions available under the EPL. Tobias Hoppe" > In comment #18 you produced performance results demonstrating the benefits of a > change to EcoreEnvironment. This change is no longer present. I don't > understand what you were measuring, so I don't know whether your enhancement > has been lost or not. If it's still worth having, please raise a new bug for it > with a patch once this one has gone through. We'll re-measure and report back.
(In reply to comment #31) Oh, wait: I just see that by removing the EcoreEnvironment changes a core piece of what we wanted to achieve doesn't work. It goes to show that our tests weren't sufficient, so we have to work on this. The key idea was of course (and I think you agreed with this) to align the caching of expression ASTs and the use of these cached ASTs across usage by delegates and by the evaluation visitor. By reverting our changes in EcoreEnvironment the evaluation visitor won't be able to use the same cache. Instead, it's back to using EAnnotation ann = typedFeature.getEAnnotation(Environment.OCL_NAMESPACE_URI); which I think is not what we want. We have to come up with tests asserting this. And I think we need the EcoreEnvironment patch then too to achieve the desired behavior.
(In reply to comment #33) Here's a failing test case that documents the problem: /** * Caches an operation AST in the annotation used by the {@link InvocationBehavior} implementation * and ensures that it's used by the delegate. * @throws ParserException * @throws InvocationTargetException */ public void test_operationUsedFromCache() throws ParserException, InvocationTargetException { initModel(COMPANY_XMI); EObject manager = companyFactory.create(employeeClass); EObject employee = companyFactory.create(employeeClass); employee.eSet(employeeClass.getEStructuralFeature("manager"), manager); OCL ocl = OCL.newInstance(); Helper helper = ocl.createOCLHelper(); helper.setContext(employeeClass); OCLExpression expr = helper.createQuery("self.reportsTo(self.manager)"); assertTrue((Boolean) ocl.evaluate(employee, expr)); // by the default impl, employee reports to manager // similarly, the delegate should answer with "true" EOperation reportsToOp = employeeClass.getEOperation(CompanyPackage.EMPLOYEE___REPORTS_TO__EMPLOYEE); assertTrue((Boolean) employee.eInvoke(reportsToOp, new BasicEList<EObject>(Collections.singletonList(manager)))); // Now cache a BooleanLiteralExp with the "false" literal as the implementation for reportsTo: BooleanLiteralExp falseLiteralExp = EcoreFactory.eINSTANCE.createBooleanLiteralExp(); falseLiteralExp.setBooleanSymbol(false); EAnnotation reportsToAnn = reportsToOp.getEAnnotation(OCLDelegateDomain.OCL_DELEGATE_URI); assertTrue(reportsToAnn.getDetails().containsKey(InvocationBehavior.BODY_CONSTRAINT_KEY)); EObject oldFirstContents = reportsToAnn.getContents().get(0); try { assertFalse((Boolean) ocl.evaluate(employee, expr)); assertFalse((Boolean) employee.eInvoke(reportsToOp, new BasicEList<EObject>(Collections.singletonList(manager)))); } finally { reportsToAnn.getContents().set(0, oldFirstContents); } }
I see two possible approaches: 1) We *do* redefine getBodyCondition(...) and/or getDefinition(...) in EcoreEnvironment and let them check InvocationBehavior.getOperationBody(...) or SettingBehavior.getFeatureBody(...), respectively. This would ensure that EvaluationVisitor finds the cached expressions. 2) We enhance AbstractEvaluationVisitor.getOperationBody so that *it* checks InvocationBehavior.getOperationBody(...), and we enhance AbstractEvaluationVisitor.getPropertyBody(...) so that *it* checks SettingBehavior.getFeatureBody(...). From your reverting our proposed EcoreEnvironment change I take it that a change to AbstractEvaluationVisitor may be the more appropriate place to change. I'll go for that and send a patch soon. Stay tuned.
Created attachment 185497 [details] Asserting that evaluation visitor uses cache + fix Instead of patching EcoreEnvironment.getDefinition|getBodyCondition, this patch adds redefinitions of getOperationBody | getPropertyBody to the new o.e.o.e.EvaluationVisitorImpl. They first try to fetch cached ASTs using InvocationBehavior.getOperationBody | SettingBehavior.getFeatureBody, respectively, before delegating to the base class implementations if no cached AST is found. I added two test cases that make sure that the evaluation visitor uses the cached AST copies.
The attachment was not built against HEAD, so it is very difficult to apply. Please re-attach. I don't have a problem with overriding EcoreEnvironment getBody, if that's the appropriate solution. I just removed it because it wasn't needed by any tests and appeared to be possibly the result of an EAnnotation namespace misunderstanding. With 2 authors, I think there will be no alternative to IP approval. Hopefully with your statement already in place and the small size of the patch, Sharon can approve while triaging again.
(In reply to comment #37) > The attachment was not built against HEAD, so it is very difficult to apply. > Please re-attach. Strange. I did a CVS update and didn't get any, so I wonder against which CVS state it would have been built otherwise. Anyway, I re-created the patch and will attach it any moment. > With 2 authors, I think there will be no alternative to IP approval. Hopefully > with your statement already in place and the small size of the patch, Sharon > can approve while triaging again. Ok, let me know if there's anything else I need to do.
Created attachment 185498 [details] Asserting that evaluation visitor uses cache + fix Re-created patch as Ed said 185497 didn't apply property to CVS head.
185497 was the wrong file; one from bug 252621 activity. 185498 applies fine. The change is ok, but it is a shame to have to create a new OCL instance when none is needed for a cache hit. If you're expecting a cache hit, perhaps e.g. getOperationBody could be called twice, firstly with a null ocl to exploit a hit, and then again for a miss with a non-null ocl. Maybe your benchmarks might show a difference. [It's not actually necessary to construct an ocl or a helper in your tests since the test harness already provides them, perhaps needing a downcast.]
(In reply to comment #40) > 185497 was the wrong file; one from bug 252621 activity. Ah, that explains it :-) Sorry. > 185498 applies fine. The change is ok, but it is a shame to have to create a > new OCL instance when none is needed for a cache hit. True. > If you're expecting a cache hit, perhaps e.g. getOperationBody could be called > twice, firstly with a null ocl to exploit a hit, and then again for a miss with > a non-null ocl. Maybe your benchmarks might show a difference. I was first struggling with how getFeatureBody/getOperationBody would signal the difference between "found a textual expression in an annotation but failed to compile because a null OCL was given" and "didn't find a textual expression, so no need to try to pass a valid OCL in order to compile". One way may be to just let it hit the NullPointerException, but that's quite a bit ugly and I had supposed that exception handling would not perform so well. Instead, I created a "LazyOCL" in o.e.o.e.EvaluationVisitorImpl which works off a static AbstractEnvironmentFactory that quickly initializes the base class (o.e.o.e.OCL) fields with null values. All OCL operations are redefined in LazyOCL, in particular createOCLHelper, so as to use the EvaluationVisitorImpl's EcoreEnvironmentFactory to construct a valid OCL delegate. As you expected, avoiding elaborate OCL construction, particular if it happens for each cache lookup, saves time. Biggest surprise last. Here are the numbers: Without using cache in EvaluationVisitorImpl, using delegates only: Executing self.reportsTo(self.manager) 1000000 times took 15571ms Executing self.reportsTo(self.manager) 1000000 times took 14404ms Executing self.reportsTo(self.manager) 1000000 times took 14399ms Using cache in EvaluationVisitorImpl With regular OCL instance: Executing self.reportsTo(self.manager) 1000000 times took 12462ms Executing self.reportsTo(self.manager) 1000000 times took 11346ms Executing self.reportsTo(self.manager) 1000000 times took 11290ms With LazyOCL: Executing self.reportsTo(self.manager) 1000000 times took 8127ms Executing self.reportsTo(self.manager) 1000000 times took 7299ms Executing self.reportsTo(self.manager) 1000000 times took 7523ms With single LazyOCL per EvaluationVisitorImpl: Executing self.reportsTo(self.manager) 1000000 times took 7288ms Executing self.reportsTo(self.manager) 1000000 times took 6613ms Executing self.reportsTo(self.manager) 1000000 times took 6533ms With IllegalArgumentException for null OCL ref: Executing self.reportsTo(self.manager) 1000000 times took 7206ms Executing self.reportsTo(self.manager) 1000000 times took 6024ms Executing self.reportsTo(self.manager) 1000000 times took 6057ms As reference, I've included the times with no immediate cache use in EvaluationVisitorImpl (commented getOperationBody/getPropertyBody methods). You can see that the LazyOCL approach in its current form saves ~42% evaluation time. Surprisingly (at least to me), the exception handling-based approach saves ~45% evaluation time and is a lot easier to implement. I will create a patch now that implements the exception handling-based approach and, FYI, still contains the commented LazyOCL stuff (to explain how the above numbers were produced; additionally the probably to-be-removed-again test case DelegatesTest.test_performanceOfCacheRetrieval). You may hence review both approaches and easily do your own benchmarks with both options. The difference seems not very significant, so the discussion would probably rather have to consider things like clarity, brevity, maintainability, robustness. I can live with both options. > [It's not actually necessary to construct an ocl or a helper in your tests > since the test harness already provides them, perhaps needing a downcast.] Fixed. Thanks for the hint.
Created attachment 185528 [details] Exception-based approach for avoiding OCL construction For clarity, I decided to split into two patches, one for each option. This one shows the slightly faster approach using IllegalArgumentExceptions. To measure and compare performance, increase counters in DelegatesTest.test_performanceOfCacheRetrieval().
Created attachment 185529 [details] LazyOCL-based approach for avoiding OCL construction Alternatively to 185528. Avoids IllegalArgumentException and its handling for what should rather be a simple case distinction. However, needs to introduce quite some boilerplate code to match the OCL and EnvironmentFactory contracts and delegate to a lazily-constructed real OCL instance. Slightly slower than the exception approach (~3%, measured by DelegatesTest.test_performanceOfCacheRetrieval()).
Created attachment 185736 [details] Peek approach to avoid redundant OCL creation You obviously weren't fully happy with either of your approaches. I think this gives the best of both worlds by allowing a peek without an OCL to bypass the OCL creation. No LazyOCL, no IllegalArgumentException. Why doesn't getInvariant need the same? Why isn't getInvariant used at all? This also fixes a duplicate filtering problem for multiple cached invalids. NB cacheInvalidExpression wasn't necessary since a Java null caches invalid. cacheInvalidExpression wasn't actually used.
(In reply to comment #44) > Created an attachment (id=185736) [details] > Peek approach to avoid redundant OCL creation Looks like a good idea. > You obviously weren't fully happy with either of your approaches. True. > I think this gives the best of both worlds by allowing a peek without an OCL to > bypass the OCL creation. No LazyOCL, no IllegalArgumentException. Seems so. > Why doesn't getInvariant need the same? Why isn't getInvariant used at all? It's not needed at evaluation time. However, we use it intensely in our code, e.g., for fetching all invariants from all classes of a given EPackage, in order to feed those into the ImpactAnalyzer. Having a solid API that hides but uses the caching implementation is very useful to us. > This also fixes a duplicate filtering problem for multiple cached invalids. NB > cacheInvalidExpression wasn't necessary since a Java null caches invalid. > cacheInvalidExpression wasn't actually used. Had I forgotten to remove it in the recent patches? You're right, it wasn't needed in either. I've repeated the benchmark with your patch: Executing self.reportsTo(self.manager) 1000000 times took 11709ms Executing self.reportsTo(self.manager) 1000000 times took 10959ms Executing self.reportsTo(self.manager) 1000000 times took 10699ms This is surprisingly quite slower than the IllegalArgumentException approach which was: With IllegalArgumentException for null OCL ref: Executing self.reportsTo(self.manager) 1000000 times took 7206ms Executing self.reportsTo(self.manager) 1000000 times took 6024ms Executing self.reportsTo(self.manager) 1000000 times took 6057ms Do you have an idea why that is? If we can't clarify or improve this, I suggest we go with IllegalArgumentException because this is a significant difference. I'll repeat the benchmark also for IllegalArgumentException and will report back. Minor typo in Javadoc: "sertting" should be "setting" and the two ".." after "definition" in the Javadoc of InvocationBehavior.getOperationBody(OCL ocl, EOperation operation) is one too many.
(In reply to comment #45) It's repeatable. I re-measured the IllegalArgumentException approach: Executing self.reportsTo(self.manager) 1000000 times took 7009ms Executing self.reportsTo(self.manager) 1000000 times took 5981ms Executing self.reportsTo(self.manager) 1000000 times took 6104ms *Much* faster. I'm still to find out why.
(In reply to comment #46) > (In reply to comment #45) > It's repeatable. I re-measured the IllegalArgumentException approach: > > Executing self.reportsTo(self.manager) 1000000 times took 7009ms > Executing self.reportsTo(self.manager) 1000000 times took 5981ms > Executing self.reportsTo(self.manager) 1000000 times took 6104ms > > > *Much* faster. I'm still to find out why. I think I found the problem. Your patch takes a null return as indication that an OCL object was missing. However, null also results when there is no annotation at all. For those cases, creating an OCL instance and passing it to a call to getOperationBody(OCL, EOperation) again returns null. With the exception-based cache this second call doesn't happen because no exception is thrown but null is returned silently, meaning there is no body expression (e.g., for stdlib operations). I'll try to restructure your "peek" approach to benefit the same way. Let's see...
Created attachment 185741 [details] Performance improvement by adding probes for text annotation Added two methods public boolean hasUncompiledOperationBody(EOperation operation) and public boolean hasUncompiledFeatureBody(EStructuralFeature structuralFeature) to InvocationBehavior and SettingBehavior, respectively. This brings performance back to good: Executing self.reportsTo(self.manager) 1000000 times took 6717ms Executing self.reportsTo(self.manager) 1000000 times took 5638ms Executing self.reportsTo(self.manager) 1000000 times took 5660ms when executed as the single test case and even Executing self.reportsTo(self.manager) 1000000 times took 5132ms Executing self.reportsTo(self.manager) 1000000 times took 3701ms Executing self.reportsTo(self.manager) 1000000 times took 3818ms when running in the standalone o.e.o.ecore suite (probably a warmed-up VM...)
I'm puzzled. The actual problem with my first cache enhancement was that I failed to cache all the possible post cache enquiry states distinctly. Subsequent changes of control flow have worked around the problem that the cache 'line' once populated does not fully capture all cacheable state. So I added an UncompiledExpression to cache the string-valued EAnnotation access. Oops. 12 tests now fail because they expect fruitPackage to be unmodified, but the test harness framework detects that populating the cache modifies its containers. Since my improvement doesn't work, perhaps we could just accept that your last patch is pretty good. However I'm worried that this isModified problem has only just shown up. I think its because the partial cacheing only affected unshared resources that the test harness didn't check. Full cacheing affects all resources an anomally is detected. So we have a philosphical problem. Using intra-model EAnnotation.eContents as a cache introduces a behavioural change by marking models as modified, whereas the old extra-model caches did not. Since the EAnnotation.eContents is private protocol, by virtue of the reserved source URI, I think we should suppress the EMF change notifications on eContents. However this makes me very worried about the validity of this patch. EMF users can expect a tree traversal to find 'normal' objects. But by re-using EAnnotation.eContents as a cache we have created some less 'normal' objects. Will global model algorithms such as cross reference caches get confused if some objects that are not marked as volatile or transient do not participate in notification mechanisms? I think we should use a derived EAnnotation with an additional non-EMF cache of String-to-OclExpression avoiding the need for synchronized lists that cannot contain duplicates. We will still get a modification from addition of the cache EAnnotation, but that will be for a well-behaved object. All subsequent changes will be hidden. This may break in a future EMF release since there is a vague intention that the EPackage.freeze that occurs at the end of EPackage initialization should be enforced. Any better ideas?
An OCL-specific example of the problem: Can if IfExp.allInstances()->isEmpty() then true else false endif ever be false? since with IfExp cached within the containment hierarchy, it is possible that some allInstances algorithm could find it. Clearly OCL meta-model instances must not be findable, otherwise we would have to proactively compile everything to avoid allInstances() returns changing as caches populate. I think that for now we need a derived CachingEAnnotation.eVolatiles, and we need to request that EMF provide a backdoor EAnnotation.eVolatiles so that we don't need an eAnnotations change to install it after an EPackage freeze.
(In reply to comment #50) Ed, I see your point regarding the modification of Ecore resources. Let me quickly reiterate our original design goals. 1) We want to ensure that evaluation of operation and feature bodies not called through a delegate explicitly but during evaluating calling expressions work fast. This suggested that these calls should ideally not get routed through the delegates but instead consumed the OCL bodies directly. 2) We want to be able to store the OCL ASTs, not solely for caching/performance reasons, but at least as much towards enabling common refactoring together with the Ecore models they annotate. Caching in the annotation contents solves 1) and 2) but creates the issue you mention, particularly when the cache is dynamically populated. 2) intentionally wants an IfExp.allInstances() to return any IfExp in which it is embedded because tooling on top of the Ecore/OCL combination will be able to work like a charm once we have that, I'm sure. If we agree that 1) and 2) are desirable goals then a solution can look like this: We don't silently update the EAnnotation contents during caching. We do read them, though, in order to find any previously and perhaps persistently cached AST. We can copy those into a transient cache, similar to what the getDefinition()/getBodyCondition()/... methods and the delegates already do. If compilation from a string is necessary, this wouldn't update the EAnnotation contents but only the transient cache. At the same time this establishes a contract by which tools can save some compilation time (by pre-compiling the expressions and storing them in the Ecore resources) and that allows tools to manipulate the ASTs directly which then would be defined to take precedence over the string version. Let me know what you think, and I can work on a patch implementing this idea.
I don't understand what you're proposing. I agree with 1) and 2) but I think you're going a meta-level confusion beyond 2). If an OCL or any other AST analysis is to be performed on the embedded OCL then that AST must exist. It would seem to be a prerequisite to fully load the meta-model and compile all its OCL expressions. Perhaps we need a cacheAll() helper function. Perhaps this is part of a special ExtentMap that can see the hidden AST caches. But for normal usage MyClass with myInvariant should enable me to evaluate OCL over MyClass instances, but allInstances should not see the OCL artefacts. Having distinct transient caches seems like a lot of extra work that goes back to where we started and loses all the good features we've been tuning. I'm not sure what's wrong with my CachedEAnnotation idea that just simplifies the current eContents in an eVolatiles.
(In reply to comment #52) > I don't understand what you're proposing. I agree with 1) and 2) That's good :-) > but I think > you're going a meta-level confusion beyond 2). Not sure. > If an OCL or any other AST analysis is to be performed on the embedded OCL then > that AST must exist. True. It just means that *if* someone wants to come up with such tool support, then they have to make sure the ASTs are compiled and attached. > It would seem to be a prerequisite to fully load the > meta-model and compile all its OCL expressions. Perhaps we need a cacheAll() > helper function. Perhaps this is part of a special ExtentMap that can see the This can be provided at some later point. We've been prototyping this, and of course it comes with a few complications on its own. In particular, those elements that the analyzer creates in the ocl://... resource need to be copied into the .ecore resource with the references to them updated. We don't have that fully under control yet but are working on it. I don't think that introducing the reading part should be blocked by that. > hidden AST caches. But for normal usage MyClass with myInvariant should enable > me to evaluate OCL over MyClass instances, but allInstances should not see the > OCL artefacts. Why is it a problem if persistently cached OCL expressions are returned by allInstances()? This would already happen, I presume, if someone used defineAttribute(...) or defineOperation(...) because it will add a Constraint object (which contains the OCLExpression) to the contents of the EAnnotation. A properly working allInstances() therefore would have to return these elements already. The only difference being that the patch proposed here would also change .ecore resources for those not using defineAttribute or defineOperation. > Having distinct transient caches seems like a lot of extra work that goes back > to where we started and loses all the good features we've been tuning. Not really. So far there is getDefinition(...) which tries to read a Constraint (not an OCLExpression, BTW) from the Environment.OCL_NAMESPACE_URI annotation (BTW, remember you proposed to unify this with the OCLDelegateDomain.OCL_DELEGATE_URI? What happened to that thought?). getBodyCondition(...) reads a transient cache from the Environment which will be gone if a new EnvironmentFactory / new OCL object is created / used. It also caches a Constraint, not an OCLExpression. If changing the .ecore metamodel resources is not an option then the cache read by getDefinition(...) can't be changed during evaluation for the same reason we can't put the "naked" OCLExpression into the annotation contents. And the getBodyCondition/Environment cache is not an option either because it has to be re-constructed for each new Environment. I would imagine a WeakHashMap-based cache that is attached to the bundle activator (OCLEcorePlugin) and uses the EOperation/EStructuralFeature objects as key and has OCLExpression as value. We could even go as far as using it as first lookup source, only to be invalidated by a define[Attribute|Operation]. If nothing's there, the usual cascade of lookups (getDefinition/getBodyCondition) takes place and caches any lookup/compilation results. > I'm not > sure what's wrong with my CachedEAnnotation idea that just simplifies the > current eContents in an eVolatiles. The problem I see with this is that when loading a standard Ecore resource with its OCL annotations that don't contain ASTs but only the textual representation, the default Ecore deserializer and any other existing way of loading Ecore models will not use the CachedEAnnotation class. Upon caching we would have to replace the annotation which again modifies the resource which I take to be the root cause of your valid concern. Therefore I think we wouldn't gain anything from this approach.
Created attachment 185799 [details] Don't modify ecore resources when caching; cache in plugin instance Here are the new performance numbers: Executing self.reportsTo(self.manager) 1000000 times took 5641ms Executing self.reportsTo(self.manager) 1000000 times took 4647ms Executing self.reportsTo(self.manager) 1000000 times took 4662ms That looks quite ok. This patch no longer updates caches in the ecore resources. Instead, I installed three WeakHashMap caches in the OCLEcorePlugin instance (operations, properties, invariants). I also made the validation part consistent with the invocation/setting part as far as possible. For now I removed the special caching you had introduced regarding uncompilable expressions where you suggested to cache an InvalidLiteralExp. It made handling quite more complex, I found. Besides, I'm not convinced that answering with an "invalid" literal is semantically collect. Someone could try to evaluate an operation call expression to an operation with an uncompilable body. If that calling expression appended an ->oclIsInvalid() this would come out without exception as a valid response which alters the original, uncached behavior which would have to throw an exception all the way up, perhaps only at the very latest moment getting turned in an OclInvalid. Last but not least, uncompilable expressions should really be the exception, should get fixed, and suboptimal performance in those cases should be tolerable.
(In reply to comment #53) > Why is it a problem if persistently cached OCL expressions are returned by > allInstances()? This would already happen, I presume, if someone used > defineAttribute(...) or defineOperation(...) because it will add a Constraint > object (which contains the OCLExpression) to the contents of the EAnnotation. A > properly working allInstances() therefore would have to return these elements > already. The only difference being that the patch proposed here would also > change .ecore resources for those not using defineAttribute or defineOperation. It's not a problem, but it may be an unwelcome surprise to some users. A user interested in their models, may expect the extent to contain their models. They will be confused if tooling artefacts show up, and also a little iritated about wasted resources that support the tooling artefacts. A user interested in the tooling for their models will indeed want to see the tooling artefacts. I think these users should specifically request the larger extent. > I would imagine a WeakHashMap-based cache that is attached to the bundle > activator (OCLEcorePlugin) and uses the EOperation/EStructuralFeature objects > as key and has OCLExpression as value. We could even go as far as using it as > first lookup source, only to be invalidated by a define[Attribute|Operation]. > If nothing's there, the usual cascade of lookups > (getDefinition/getBodyCondition) takes place and caches any lookup/compilation > results. I'm not fond of WeakHashMap; it's an admission of loss of control. I was composing an EMF newsgroup message for help on a non-modifying cache and realised how to solve the problem. We must use a mechanism that is false for Notification.isTouch(). This appears to allow us to add/remove adapters, so we can just move the synchronized index no-duplicates EAnnotation.eContents to a simpler Map of Key-OclExpression in an EAnnotation.eAdapters. The EAnnotation should not be changing very much so there should be minimal overhead from gratuitous notifications to the extra adapter. > > I'm not > > sure what's wrong with my CachedEAnnotation idea that just simplifies the > > current eContents in an eVolatiles. > > The problem I see with this is that when loading a standard Ecore resource with > its OCL annotations that don't contain ASTs but only the textual > representation, the default Ecore deserializer and any other existing way of > loading Ecore models will not use the CachedEAnnotation class. Upon caching we > would have to replace the annotation which again modifies the resource which I > take to be the root cause of your valid concern. Therefore I think we wouldn't > gain anything from this approach. Yes.I was prepared to live with one modification at 'start up', but eAdapters above solves the problem. (In reply to comment #54) > For now I removed the special caching you had introduced regarding uncompilable > expressions where you suggested to cache an InvalidLiteralExp. It made ... and suboptimal > performance in those cases should be tolerable. No problem.
Using an Adapter also resolves my Save concern since adapters are not saved.
(In reply to comment #56) > Using an Adapter also resolves my Save concern since adapters are not saved. Interesting idea to use adapters. I'll give it a shot and see if I can produce a corresponding patch.
Created attachment 185865 [details] Using eAdapters() to cache ASTs I compiled a patch that attaches the cached expressions as adapters. Benchmark results: Executing self.reportsTo(self.manager) 1000000 times took 7227ms Executing self.reportsTo(self.manager) 1000000 times took 6235ms Executing self.reportsTo(self.manager) 1000000 times took 6191ms About in the same ball park. Looking forward to your review.
You have added, rather than replaced, an adapter cache. I tried to revise this, but something went wrong, possibly just that the tests are testing the wrong thing. I've now run out of time. You've perhaps not used adapters before. EcoreUtil.getAdapters uses typically e.g. @Override public boolean isAdapterForType(Object type) { return type == OCLExpressionCacheAdapter.class; } (In reply to comment #55) > We must use a mechanism that is false for > Notification.isTouch(). This appears to allow us to add/remove adapters, so we > can just move the synchronized index no-duplicates EAnnotation.eContents to a > simpler Map of Key-OclExpression in an EAnnotation.eAdapters. The EAnnotation > should not be changing very much so there should be minimal overhead from > gratuitous notifications to the extra adapter. I would like to see the use of eContents eliminated, and only one adapter per EModelElement (class/operation/property) which is located in OCL_DELEGATE_URI EAnnotation.eAdapters. Therefore, the initial search for the OCL_DELEGATE_URI annotation also indicates whether there is no cache. Performance was not considered when the delegates were introduced for Helios. I suspect that >90% usage is on standard library features so a fast null return is very important for these. The miss on the OCL_DELEGATE_URI annotation achieves this directly. Once an OCL_DELEGATE_URI annotation hit occurs, the cache can be located and if found it should return a) a compiled OCLExpression b) an uncompiled OCLExpression (I suggest a derived StringLiteralExp with name=constraintKey and stringValue=expression text. c) null b) is returned to caller if there is no OCL to compile with, ie.e in getCachedXXX. b) is compiled to a) on success, or left as b) throwing an exception on failure in getXXX. You've added some internal classes for the caches. I suspect that they're small enough to be nested. You've plausibly used different cache classes for invariants where there might be 100 bodies, and others where there is most one or two. I think three different caches are actually appropriate: map of N class invariants; two possible setting bodies; one possible operation body (obviously we ignore pre/postconditions). There is a probably trivial bug in the setting functionality. helper.createDerivedValueExpression rather than helper.createInitialValueExpression is used for an INITIAL constraint. ----- The revised adapter design has a number of benefits: a) no apparent resource modification b) no adverse effect on save c) expressions can be shared in multiple caches and minor consequences a) eResource() is null for cached expressions b) cached expressions are not seen by an allInstances containment traversal. [It would be better to cache ExpressionInOCL rather than OCLExpression since ExpressionInOcl is where the additional environment such as bariables and synthesized types can be persisted. Unfortunately the createQuery API is wrong; fixable in the pivot model.] ----- (In reply to comment #53) > (not an OCLExpression, BTW) from the Environment.OCL_NAMESPACE_URI annotation > (BTW, remember you proposed to unify this with the > OCLDelegateDomain.OCL_DELEGATE_URI? What happened to that thought?). Working on this bug enabled me to discover (comment #23) that Environment.OCL_NAMESPACE_URI is used to annotate Complete OCL defined contributions which have significantly different semantics (genuine additions) to OCL_DELEGATE_URI which should make no semantic change; just a perforkmance boost. I was therefore pleased by one of my revisions that eliminated all code modifying Environment.OCL_NAMESPACE_URI. Again the pivot model should avoid the need for annotations here.
(In reply to comment #59) > You have added, rather than replaced, an adapter cache. I replaced the cache in the OCLEcorePlugin. > You've perhaps not used adapters before. EcoreUtil.getAdapters uses typically > e.g. > > @Override > public boolean isAdapterForType(Object type) { > return type == OCLExpressionCacheAdapter.class; > } Thanks for the hint. I'll change the patch correspondingly. > I would like to see the use of eContents eliminated, and only one adapter per > EModelElement (class/operation/property) which is located in OCL_DELEGATE_URI > EAnnotation.eAdapters. Therefore, the initial search for the OCL_DELEGATE_URI > annotation also indicates whether there is no cache. That would be ok if there weren't the agreed-upon desire to be able to store the compiled ASTs durably in the Ecore resources. According to my understanding changes to the eAdapters reference are not stored in the resource holding the element to which the adapters attach. If this assumption is valid, durably storing the AST won't be possible in an adapter. Or am I missing something? This would mean that we'll need two optional locations for the compiled AST: 1) in an adapter that transiently attaches to the EAnnotation 2) in the contents of the EAnnotation for durable storage, refactoring capabilities, allInstances support, etc. 2) is mostly addressed by the way getDefinition looks up stuff, only it would be good if it used the same annotation source URI as the delegate stuff to be able to unify these lookups. 1) was what I attempted in the last patch, more or less, only that instead of attaching the adapter to the EAnnotation I attached it directly to the element to which the EAnnotation is attached. Changing that should be easy, but we first need to have agreement on the above, I think.
(In reply to comment #60) Some quick comments. > I replaced the cache in the OCLEcorePlugin. Good. You need to revert to CVS since you're now patcghing some whitesapce only. > > > You've perhaps not used adapters before. EcoreUtil.getAdapters uses typically > > e.g. > > > > @Override > > public boolean isAdapterForType(Object type) { > > return type == OCLExpressionCacheAdapter.class; > > } > > Thanks for the hint. I'll change the patch correspondingly. Did you overlook this? This method is not overridden in the new caches. > That would be ok if there weren't the agreed-upon desire to be able to store > the compiled ASTs durably in the Ecore resources. I think we have a misunderstanding here. I thought that you calmed my Save and AST/String validity concerns by saying that the AST was solely for transient use. This is supported nicely by the adapters without using EAnnotation.eContents. Persisting the AST may be useful, but is a separate issue. Please eliminate it from this Bugzilla and raise a separate one. I cannot promise to deal with that rapidly or positively since my time prior to M6 is limited and I'm not sure that I want to introduce a mechanism to persist a non-standard AST that will hopefully be deprecated by the standard AST of the pivot model.
(In reply to comment #61) > (In reply to comment #60) > Some quick comments. > > > I replaced the cache in the OCLEcorePlugin. > > Good. You need to revert to CVS since you're now patcghing some whitesapce > only. That's easy :-) > > > You've perhaps not used adapters before. EcoreUtil.getAdapters uses typically > > > e.g. > > > > > > @Override > > > public boolean isAdapterForType(Object type) { > > > return type == OCLExpressionCacheAdapter.class; > > > } > > > > Thanks for the hint. I'll change the patch correspondingly. > > Did you overlook this? This method is not overridden in the new caches. You may have overlooked that I didn't produce an updated patch with my comment. Still to come. I first would like to ensure we agree upon the general direction to avoid rework, extra work or duplication of work. > > That would be ok if there weren't the agreed-upon desire to be able to store > > the compiled ASTs durably in the Ecore resources. > > I think we have a misunderstanding here. I thought that you calmed my Save and > AST/String validity concerns by saying that the AST was solely for transient > use. This is supported nicely by the adapters without using > EAnnotation.eContents. Quick recap: Originally I had hoped that we can align transient and persistent cache. That led to the correct observation that transient caching would change Ecore resources which may have undesirable side effects. Caching transiently in adapters is one way to resolve this but requires splitting this off from the clean-up of the persistent caching capabilities. We already have basic support for persistent caching by means of the lookup performed by getDefinition(). However, it's so far not consistent with the delegate logic which looks up the expressions in different annotations. It would be rather uncool to have the AST in an EAnnotation other than the one providing the textual representation, don't you think? We can split this bug into two: - provide transient caching used during evaluation which is consistent with the delegate mechanism, such that expressions introduced by the delegate logic can be cached by this mechanism as well - clean up persistent caching and unify between delegates and getDefinition(), deprecating the old getDefinition() annotation source, probably removing it with 4.0.0. What do you think?
It is amazing how difficult this superficially simple, sensible change has proved to be; partly the consequence of API comaptibility; partly the consequence of under-specified inflexible structure. The problems should vanish completely in 4.0, since UML and so the Pivot model supports Constraints via NamedElement.ownedRule, and ExpressionInOcl via Constraint.specification. OMG OCL 2.x has yet to align ExpressionInOcl with UML 2.0's change to ValueSpecification/OpaqueExpression so there is discretion to get either the OMG specification or the Eclipse realisation to have appropriate fields for string/ast values. EAnnotations should not be necessary in the pivot model, merely for full persistence of the pivot model in inadequate traditional formats (e.g. opposites in Ecore, nsPrefix in UML). We are therefore struggling to find a temporary solution for Eclipse OCL 3.1 that is helpful to some (particularly you) without being harmful to others. I think/hope we're in agreement.
(In reply to comment #63) > It is amazing how difficult this superficially simple, sensible change has > proved to be; partly the consequence of API comaptibility; partly the > consequence of under-specified inflexible structure. > > The problems should vanish completely in 4.0, since UML and so the Pivot model > supports Constraints via NamedElement.ownedRule, and ExpressionInOcl via > Constraint.specification. OMG OCL 2.x has yet to align ExpressionInOcl with UML > 2.0's change to ValueSpecification/OpaqueExpression so there is discretion to > get either the OMG specification or the Eclipse realisation to have appropriate > fields for string/ast values. EAnnotations should not be necessary in the pivot > model, merely for full persistence of the pivot model in inadequate traditional > formats (e.g. opposites in Ecore, nsPrefix in UML). > > We are therefore struggling to find a temporary solution for Eclipse OCL 3.1 > that is helpful to some (particularly you) without being harmful to others. > > I think/hope we're in agreement. Sorry, I don't understand what this is supposed to mean with regard to this bug and 3.1. Are you suggesting I proceeded as I described (splitting etc.)?
(In reply to comment #64) > Sorry, I don't understand what this is supposed to mean with regard to this bug > and 3.1. Are you suggesting I proceeded as I described (splitting etc.)? Trying to unblock confusion/misunderstanding. If you want to introduce persistence of ASTs, please migrate that to a separate enhancement request that can be addressed once this bug is finished. Please update the patch for transient-only AST caching, so that the former eContents list at indexes synchronized with eDetails, is replaced by a Map of eDetail.key to AST in an adapter for the EAnnotation that has the eDetails. Please exploit the co-location of eDetails and eAdapter to optimize the control path for the commonest use case of a standard library function without an EAnnotation.
(In reply to comment #65) > is replaced by a Map of eDetail.key to AST Map is easiest, but excessive when there is only a 'body'. Use a more optimum approach if you like.
Created https://bugs.eclipse.org/bugs/show_bug.cgi?id=334821 to deal with the persistent caching clean-up and unification. This one now is only about transiently caching ASTs and short-cutting evaluation paths if OCL delegates are being used.
Created attachment 187182 [details] No persistent caching anymore, only transient in adapters As discussed, persistent caching was spun off into https://bugs.eclipse.org/bugs/show_bug.cgi?id=334821. This consolidated patch now only implements transient caching in respective adapters that transiently attach to the features (for operation / property bodies) or classes (for invariants). They don't affect Ecore resources anymore. No change and therefore no incompatibility regarding any clean-up of OCL_NAMESPACE_URI vs. OCL_DELEGATE_URI harmonization.
Created attachment 187188 [details] No persistent caching anymore, only transient in adapters Compared to 187182 fixes a minor problem in InvocationBehavior.getCachedOperationBody for the case where the cache was empty
Created attachment 187305 [details] Clarified/simpliufied NoOCLDefinition Great. It's got a lot simpler and easier to understand. Only minor tweaks. Hopefully, the streamlining should incur reduced costs even on the first access and obviously much more on subsequent accesses. An all round win. You need to enable API wrt the Helios baseline, then you will discover that the changes to .api_filters are not required. Placing the single expression cache support in AbstractDelegatedBehavior meant that ValidationBehavior potentially had two caches. Expression cache moved down to Setting/InvocationBehavior and shared functionality moved to static methods of Adapter. Since the adapters are small, they are inlined. The variation between getInvariant/... with and without Body suffix gave slightly different behaviors. The extra method has been removed and the control flow and cache access methods are now identical. There is no need for multiple NullExpressions any more, since there is no longer a containment or duplicate issue from eContents. Simplified to a single INSTANCE. hasUncompiled... is ambiguous; does it change from true to false after cacheing? Changed to hasCompileable... Resulting attachment has four fewer files to patch. No .api_filters, EcorePlugin change. Caches inline. Since there is no functionality change, I hope we're there. Commit if you're happy.
Committed. Thanks for the review and the improvements.
Closing
