search

eclipse-ocl / org.eclipse.ocl

Eclipse Public License 2.0
0 stars 0 forks source link

[ast] Compiled expressions in EAnnotation contents should be considered #2352

Closed eclipse-ocl-bot closed 1 month ago

eclipse-ocl-bot commented 1 month ago

| --- | --- | | Bugzilla Link | 329167 | | Status | CLOSED FIXED | | Importance | P3 normal | | Reported | Oct 31, 2010 19:13 EDT | | Modified | May 27, 2011 03:13 EDT | | Blocks | 323223, 332400 | | Reporter | Axel Uhl |

Description

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

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Oct 31, 2010 19:14

Created attachment 182124 (attachment deleted)\ Patch for consistently using compiled OCL in EAnnotations

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Nov 01, 2010 02:22

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.

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Nov 01, 2010 04:59

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

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Nov 01, 2010 05:00

Created attachment 182131 (attachment deleted)\ Patch for consistently using compiled OCL in EAnnotations

This version of the patch relies on InvocationBehavior applying only to the default OCL delegate domain.

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Nov 01, 2010 07:55

(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.

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Nov 01, 2010 16:46

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.

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Nov 09, 2010 02:06

The attached 'patch' cannot be applied.

Please use Create Patch to create a patch of your code with respect to CVS HEAD.

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Nov 09, 2010 18:09

Created attachment 182780 (attachment deleted)\ 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.

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Nov 09, 2010 18:11

(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.

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Nov 10, 2010 04:37

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?

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Nov 10, 2010 08:14

Created attachment 182806 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.

EvaluationSequences.pdf

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Nov 10, 2010 08:40

Created attachment 182809 (attachment deleted)\ 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 (attachment deleted). Tested to apply properly on HEAD from 2010-11-10; tests all green.

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Nov 10, 2010 12:27

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.

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Nov 10, 2010 12:57

(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.

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Nov 10, 2010 13:38

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.

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Nov 12, 2010 08:10

Created attachment 182988 (attachment deleted)\ 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.

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Nov 12, 2010 09:09

(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.

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Dec 10, 2010 16:41

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

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Dec 12, 2010 13:39

Created attachment 185032 (attachment deleted)\ Regenerated patch against current CVS Head

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Dec 12, 2010 17:30

(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 {\

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Dec 12, 2010 17:34

Created attachment 185034 (attachment deleted)\ 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.

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Dec 16, 2010 17:51

Created attachment 185383 (attachment deleted)\ Cumulative patch considering stereotypes, after 251621

Re-produced the patch after updating the 251621 patch from CVS

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Dec 17, 2010 16:05

Created attachment 185460 (attachment deleted)\ 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.

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Dec 18, 2010 04:17

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.

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Dec 18, 2010 04:49

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".

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Dec 18, 2010 04:51

(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.

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Dec 18, 2010 11:01

(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.

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Dec 18, 2010 14:37

Created attachment 185483 (attachment deleted)\ 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.

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Dec 18, 2010 17:27

Created attachment 185489 (attachment deleted)\ 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.

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Dec 19, 2010 05:05

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

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Dec 19, 2010 06:58

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.

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Dec 19, 2010 09:17

(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.

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Dec 19, 2010 09:35

(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.

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Dec 19, 2010 10:02

(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);\
    }\
}
eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Dec 19, 2010 10:34

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.

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Dec 19, 2010 11:06

Created attachment 185497 (attachment deleted)\ 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.

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Dec 19, 2010 11:29

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.

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Dec 19, 2010 11:41

(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.

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Dec 19, 2010 11:42

Created attachment 185498 (attachment deleted)\ Asserting that evaluation visitor uses cache + fix

Re-created patch as Ed said 185497 didn't apply property to CVS head.

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Dec 19, 2010 15:26

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.]

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Dec 20, 2010 06:37

(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.

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Dec 20, 2010 06:55

Created attachment 185528 (attachment deleted)\ 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().

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Dec 20, 2010 06:59

Created attachment 185529 (attachment deleted)\ 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()).

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Dec 22, 2010 15:44

Created attachment 185736 (attachment deleted)\ 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.

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Dec 22, 2010 16:24

(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.

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Dec 22, 2010 16:28

(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.

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Dec 22, 2010 17:18

(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...

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Dec 22, 2010 17:35

Created attachment 185741 (attachment deleted)\ 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...)

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Dec 23, 2010 05:49

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?

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Dec 23, 2010 06:30

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.