eclipse-qvtd / org.eclipse.qvtd

Eclipse Public License 2.0
0 stars 0 forks source link

[scheduler] Rule related to singleFactor is not properly scheduled #230

Closed eclipse-qvtd-bot closed 1 week ago

eclipse-qvtd-bot commented 1 week ago

| --- | --- | | Bugzilla Link | 506751 | | Status | RESOLVED FIXED | | Importance | P3 normal | | Reported | Oct 30, 2016 12:25 EDT | | Modified | Nov 18, 2016 11:20 EDT | | Blocks | 506328 | | Reporter | Adolfo Sanchez-Barbudo Herrera |

Description

I think the following problem is related to the scheduler. When debugging the transformation, I'm observing an unexpected behaviour from the CGed transformation.

When executing the the method "CACHE_exprList_createActualParams" piece of code (invoked from "MAP_m_factor_ast_expList" mapping), the directly contained "simpleFactor" model element is processed and there is not corresponding AS element associated with it (it is null). This is a suspicious case of incorrectly scheduled rules. The method "MAP_m_simpleFactor_ast_designator" should have been previously invoked.

Repro and more analysis following next, as soon as I get the bugzilla ID.

eclipse-qvtd-bot commented 1 week ago

By Adolfo Sanchez-Barbudo Herrera on Oct 30, 2016 12:47

Repro:

Corresponding Delphi.ocl code that shows that the dependency is required:

context simpleFactor\ def : ast() : as::astm::Expression =\ self.designator.ast()

context factor\ def : ast() : as::astm::Expression = ...\ else\ if isAFunctionCall()\ then as::astm::DelphiFunctionCallExpression {\ calledFunction = self.designator.ast(), \ actualParams = expList.createActualParams()\ } ...

context cs::exprList -- a helper\ def : createActualParams() : OrderedSet(as::ActualParameterExpression) =\ self.exps->collect(x | \ as::ActualParameterExpression { value = x.ast.oclAsType(as::Expression) } )->asOrderedSet()\ \ The problem is that when execution java code corresponding to x.ast inside the helper, the .ast property has not been computed yet.


As a hint. Something to highlight that may be relevant is that the the mapping source of a) (i.e simpleFactor) is a subtype of the mapping source of b) (i.e factor). Both factor and simpleFactor are instantiable meta-classes. All corresponding AS model elements should be created before factor properties are computed.

eclipse-qvtd-bot commented 1 week ago

By Ed Willink on Oct 30, 2016 14:40

The operation has been correctly analyzed, although, Bug 506752, the cast has been ignored giving a pessimistic analysis.

Must more importantly, Bug 506753, the edge and extra guard dependencies are missing.

eclipse-qvtd-bot commented 1 week ago

By Ed Willink on Oct 31, 2016 06:38

(In reply to Adolfo Sanchez-Barbudo Herrera from comment #1)

context cs::exprList -- a helper def : createActualParams() : OrderedSet(as::ActualParameterExpression) = self.exps->collect(x | as::ActualParameterExpression { value = x.ast.oclAsType(as::Expression) } )->asOrderedSet()

This is a very smelly function. It creates a Collection of shadow ActualParameterExpression (rather than XXXXActualParameterExpression) using OCL rather than CS2AS semantics. This is therefore getting into a very dark corner of shadow object sharing for use / cloning for consumption.

The QVTs operation analysis fails to identify that an ActualParameterExpression creation occurs and so fails to provide dependencies to the producers. Since the actual producer is internal, it kind of just works.

Suggest that there should be 'conventional' ast() mappings for ByValueActualParameterExpression, ByReferenceActualParameterExpression and possibly ActualParameterExpression.

(This is a style observation. It does not actually affect this bug.)

eclipse-qvtd-bot commented 1 week ago

By Adolfo Sanchez-Barbudo Herrera on Oct 31, 2016 07:24

(In reply to comment #3)

(In reply to Adolfo Sanchez-Barbudo Herrera from comment #1)

context cs::exprList -- a helper def : createActualParams() : OrderedSet(as::ActualParameterExpression) = self.exps->collect(x | as::ActualParameterExpression { value = x.ast.oclAsType(as::Expression) } )->asOrderedSet()

This is a very smelly function. It creates a Collection of shadow ActualParameterExpression (rather than XXXXActualParameterExpression) using OCL rather than CS2AS semantics. This is therefore getting into a very dark corner of shadow object sharing for use / cloning for consumption.

The QVTs operation analysis fails to identify that an ActualParameterExpression creation occurs and so fails to provide dependencies to the producers. Since the actual producer is internal, it kind of just works.

In non-trivial cs2as scenarios, not always is possible to have 1-to-1 mappings, so that every AS model element not always have a single CS element to be created from. There are needs of 1-to-N mappings, so that from one single model element, a set of inter-connected model elements are created. The additional inner ShadowExp are used in the OCL-based internal DSTL for this purpose. The trace is still 1-to-1, so just one output element is traced (which normally contains the additional model elements to be created via ShadowExps).

Suggest that there should be 'conventional' ast() mappings for ByValueActualParameterExpression, ByReferenceActualParameterExpression and possibly ActualParameterExpression.

(This is a style observation. It does not actually affect this bug.)

As as reminder, the Gra2Mol example is incomplete. AS types such as ByValueActualParameterExpression and ByReferenceActualParameterExpression do not appear in the reference transformation.

eclipse-qvtd-bot commented 1 week ago

By Adolfo Sanchez-Barbudo Herrera on Oct 31, 2016 07:34

I'm not sure how this kind of 1-to-N mappings are expressed in QVTr, and how the trace is internally handled, but these scenarios are definitely a requirement in M2M transformations.

The trace does not mandatorily need to be 1-to-1, and it is currently driven by the implementation solution. However, the important thing is how QVTr addresses this kind of scenarios, because I expect we can generate QVTr transformations rather than instances of the OCL-based internal DSTL.

eclipse-qvtd-bot commented 1 week ago

By Ed Willink on Oct 31, 2016 07:45

A genuine 1:known-N can be enumerated in the mapping.\ A genuine 1:unknown-N may need a an iteration over 1:1 with the iterator as an additional disambiguating input.

Your example does not seem genuine. x=>as is 1:1.

eclipse-qvtd-bot commented 1 week ago

By Ed Willink on Oct 31, 2016 10:59

(In reply to Ed Willink from comment #2)

Must more importantly, Bug 506753, the edge and extra guard dependencies are missing.

ewillink/miniocl fixes this. Still to be reviewed and tidied up.

The operation has been correctly analyzed, although, Bug 506752, the cast has been ignored giving a pessimistic analysis.

The "...r_9_final" schedule still does not show the 'missing' dependencies; you need to look at "...r_5_cycled" to see the dependencies before those that are inherently satisfied by linear ordering are removed.

The overall schedule is much simpler than it looks; yEd has wrapped a very long horizontal row of siblings into a couple of rows. The schedule actually just consists of a root, N-3 carefully ordered CS2AS siblings, and 2 recursions.

Once Bug 506752 is fixed, some of the recursions may vanish, since CSTrace::ast should be less used without a cast.

eclipse-qvtd-bot commented 1 week ago

By Adolfo Sanchez-Barbudo Herrera on Oct 31, 2016 12:07

(In reply to comment #7)

(In reply to Ed Willink from comment #2)

Must more importantly, Bug 506753, the edge and extra guard dependencies are missing.

ewillink/miniocl fixes this. Still to be reviewed and tidied up.

I start to see some ActualParameterExpression appearing in right places, so I think this fix makes the generated transformation do more good things. Well done.

The operation has been correctly analyzed, although, Bug 506752, the cast has been ignored giving a pessimistic analysis.

The "...r_9_final" schedule still does not show the 'missing' dependencies; you need to look at "...r_5_cycled" to see the dependencies before those that are inherently satisfied by linear ordering are removed.

The overall schedule is much simpler than it looks; yEd has wrapped a very long horizontal row of siblings into a couple of rows. The schedule actually just consists of a root, N-3 carefully ordered CS2AS siblings, and 2 recursions.

Once Bug 506752 is fixed, some of the recursions may vanish, since CSTrace::ast should be less used without a cast.

Bug 506752 rationale makes sense to me, but I'm unsure if it is actually affecting to this bug (according to my comment above, I don't think so). I'm gonna do more debugging to see if I properly identify (and send a repro for) the next issue.

eclipse-qvtd-bot commented 1 week ago

By Ed Willink on Nov 04, 2016 14:54

(In reply to Ed Willink from comment #7)

Once Bug 506752 is fixed, some of the recursions may vanish, since CSTrace::ast should be less used without a cast.

Makes no obvious difference. Schedule is already constructions first, so a pessimistic dependency requiring excessive constructions first is trivially/already satisfied.

eclipse-qvtd-bot commented 1 week ago

By Ed Willink on Nov 04, 2016 15:45

Related bugs fixed.

eclipse-qvtd-bot commented 1 week ago

By Adolfo Sanchez-Barbudo Herrera on Nov 05, 2016 12:45

Hi Ed,

I'd say that this bug was fixed/working, but with current masters, the rules (comment 1) are not properly scheduled again:

a) m_simpleFactor_ast_designator usimpleFactor_ast 54 \ b) m_factor_ast_expList ufactor_2_DelphiFunctionCallExpression_actualParams 39

eclipse-qvtd-bot commented 1 week ago

By Adolfo Sanchez-Barbudo Herrera on Nov 09, 2016 13:57

I've just advanced cs2as-master, so I've created pushed a cs2as-asanchez/506751 to track the repro (ocl-master and qvtd-master still applies)

eclipse-qvtd-bot commented 1 week ago

By Ed Willink on Nov 10, 2016 03:57

(In reply to Adolfo Sanchez-Barbudo Herrera from comment #12)

I've just advanced cs2as-master, so I've created pushed a cs2as-asanchez/506751 to track the repro (ocl-master and qvtd-master still applies)

cs2as asanchez/506751 was committed on 1-Nov.

Have you checked that this wasn't fixed by Bug 506806 / Bug 506752 fixes on 6-Nov?

eclipse-qvtd-bot commented 1 week ago

By Adolfo Sanchez-Barbudo Herrera on Nov 10, 2016 06:05

The reopening was from 5-Nov, so I guess that any fix after that could have fixed the problem again, even if they do not seem to be related.

I've re-checked the repro again, and I'm getting the same outcome that comment 11 states.

Just guessing, but since the related Bug 506752 came after the one that originally fixed the repro (Bug 506753), the former may be the culprit.

eclipse-qvtd-bot commented 1 week ago

By Ed Willink on Nov 10, 2016 15:41

(In reply to Adolfo Sanchez-Barbudo Herrera from comment #14)

Just guessing, but since the related Bug 506752 came after the one that originally fixed the repro (Bug 506753), the former may be the culprit.

Yes. Revert the [506752] commit and you should be good to go.

eclipse-qvtd-bot commented 1 week ago

By Ed Willink on Nov 18, 2016 11:20

(In reply to Ed Willink from comment #15)

Yes. Revert the [506752] commit and you should be good to go.

Reverted on master for M4.