[evaluator] Confusing and possibly invalid evaluation if genmodel had Operation Reflection false

[eclipse-ocl-bot]

| --- | --- | | Bugzilla Link | 322159 | | Status | CLOSED FIXED | | Importance | P3 normal | | Reported | Aug 09, 2010 12:58 EDT | | Modified | May 27, 2011 03:13 EDT | | Version | 3.0.0 | | Depends on | 322163 | | Reporter | Ed Willink |

Description

Further to Bug 308944, if a delegate operation is invoked in a non-reflective context, e.g an OCL validation constraint invokes an OCL operation body, the lack of an eInvoke() in the generated class gives confusing results, since the operationIds are dispatched to the base class; most commonly executing eClass() as operation 0.

The problem arises because most of the delegate functionality is always available, it is only EOperation that has a space-saving compatibility option that fails when eInvoke is explictly invoked.

[eclipse-ocl-bot]

By Ed Willink on Aug 09, 2010 13:10

Created attachment 176170 (attachment deleted)\ Fix tio fall back on eDynamicInvoke

Attached patch detects the attempt to invoke eInvoke when it has not been overriden by a genmodel with 'Operation Reflection' true, and then generates a warning and falls back on eDynamicInvoke.

Users should therefore get correct results however genmodel is configured, which is pretty important since probably most users will not read the documentation. Users who don't set 'Operation Reflection' will get one warning per EPackage per EcoreEvaluationEnvironment construction.

Users can mitigate the reflection overhead by setting setOperationReflectionCheckDisabled(true) at the expense of risking bad results. So long as the OCL is interpreted the saving on a reflection call is probably immaterial; once we use optimised Java execution this may be more significant.

[eclipse-ocl-bot]

By Ed Willink on Aug 17, 2010 17:10

Adolfo: Do you think this is a worthwhile help to users?

[eclipse-ocl-bot]

By Adolfo Sanchez-Barbudo Herrera on Aug 23, 2010 07:14

(In reply to comment #2)

Adolfo: Do you think this is a worthwhile help to users?

Ed... Yes, I agree...

Feedback:

I think that by default, we should warn users which:

1. are using InvocationDelegates, and
2. have generated the java classes, and
3. have not established 'Operation Reflection' to true

The current patch could be improved:\ a) From my point of view, if we are in 1 and not 2 then, we should not warn or even try to use the java reflection. I think that the current patch provokes a warning if pure dynamic EMF is being used.\ b) If we are in 1, and 2, and 3, we should ALWAYS warn and use the dynamic invoke, since, in my opinion not stablishing the operation reflection is a clear miss from the user.

I'm working on the a test case to demonstrate a). If you agree, I'll also try to rework the current patch to cover a) and b)

Cheers,\ Adolfo.

[eclipse-ocl-bot]

By Ed Willink on Aug 23, 2010 11:50

You're welcome to provide a better patch. Certainly pure dynamic should not give a warning, bit I think the current delegates tests would have blown up if this was the case.

Note that it is only possible to attempt a Java invocation delegate when already execution some other delegate.

[eclipse-ocl-bot]

By Adolfo Sanchez-Barbudo Herrera on Aug 23, 2010 12:21

The current patch could be improved: a) From my point of view, if we are in 1 and not 2 then, we should not warn or even try to use the java reflection. I think that the current patch provokes a warning if pure dynamic EMF is being used.

My thought was wrong, since the DynamicObjectImpl has its eInvoke method, the java reflection checking doesn't fail for them.

I had created a new metamodel with no generated classes, but I didn't realize that when the packages are not programatically registered, dynamic instances are used instead.

So, no new test case ;P

b) If we are in 1, and 2, and 3, we should ALWAYS warn and use the dynamic invoke, since, in my opinion not stablishing the operation reflection is a clear miss from the user.

In any case, I'll submit an alternative patch which some changes:

• Now, a warning log and dynamic invocation is done when 1,2 and 3, occurs. As suggested, I think that the user should correct the generation of the classes.
• The non-trivial null/not-null empty/not-empty logic for the registered has been simplified to a boolean.
• The method which computes if the OperationReflection has been set, could be overriden by derived evaluation environments if they found a best way to
• Some refactoring in the DelegateTest which exploits the EMF reflexivity and avoids some copy-paste done for and No-Reflection cases.
• Fixing the plugin versions and @since (has you established the new API base line) ?

Regarding this last question, how are we dealing with a new method which should be annotated with a @since 3.1, and we wanted to include in the 3.0.1 branch version ? ;P

Patch coming in a little.

Cheers,\ Adolfo.

[eclipse-ocl-bot]

By Adolfo Sanchez-Barbudo Herrera on Aug 23, 2010 12:44

Created attachment 177236 (attachment deleted)\ Alternative patch

Ed,

Here you have an alternative, if you want to do a mix ;P.

I've eventually simplified the DelegateTests. I think that a new variant of the tests (which had some programatically created models) were created in the previous patch to demonstrate this uses case, which could be demonstrated using the withoutReflection_registered version test. Now it's included.

Cheers,\ Adolfo.

[eclipse-ocl-bot]

By Ed Willink on Aug 24, 2010 02:28

Created attachment 177283 Combined patch

The merged test is much better. Pity that the Company.ecore cannot be shared too.

I'm not keen on filling up the log with messages, but the extra overhead of the first time Set doesn;t seem worth it, so the revised patch is your way.

I've changed the return of the checkOperationReflectionConsistency to null/Exception so that the NSME can appear in the log.

I've tidied up the English and put a @noextend on checkOperationReflectionConsistency since I hope to remove it soon.

The patch omits the feature version changes, which need separate update for both 3.0.1 and HEAD. (Attachment 177236 omitted two feature changes).

For 3.0.1 we will have to have an API filter since Javadoc doesn't support 3-part @since.

Are you okay to commit this for 3.0.1 too?

:notepad_spiral: Bug322159c.patch

[eclipse-ocl-bot]

By Adolfo Sanchez-Barbudo Herrera on Aug 24, 2010 05:16

Are you okay to commit this for 3.0.1 too?

Yes, I'm ok.

The only reservation I have with the patch is that the performance decreases just to warn users which should have activated the Operation Reflection option in the genmodel. In any case, the time taken to execute the Delegate Tests oscillates between 3,6 - 3,7 seconds, regardless the checkOperationReflectionConsistency is executed or is not.

Maybe we could start doing a "How to optimize MDT/OCL" in our wiki.

Cheers,\ Adolfo.

[eclipse-ocl-bot]

By Ed Willink on Aug 24, 2010 13:12

Committed to HEAD and R3_0_maintenance.

I'm not fond of the extra overhead, but proportionately it's small.

With the model-driven library evetything can be a registered operation so we can have a dedicayed calling API which will eliminate the Visitor overheads. With generated Java the code too will be fast. Then we can really worry about everything else.

[eclipse-ocl-bot]

By Ed Willink on May 27, 2011 03:13

Closing