Lazy attribute connecting two separate circular attributes caches intermediate value

[Jesper Öqvist]

JastAdd 2.1.13

A lazy attribute that is called by one circular attribute and evaluates another circular attribute (not connected to each other) can cache an intermediate value of the second attribute. This is demonstrated in the JastAdd test suite by the tests circular/cacheNonCircularAttribute0{1,2}.

[Jesper Öqvist]

Fixing this by starting a new circular evaluation for the next circular attribute encountered recursively during a lazy attribute evaluation means that some old code with incorrect attribute specifications will behave differently. I have documented this in the ChangeLog:

JastAdd now always starts a new circular evaluation if a circular
attribute is called by a cached attribute that in turn was called by
another circular evaluation. This ensures correct caching behaviour for
the lazy attribute by avoiding caching an intermediate circular value,
but it can cause incorrectly specified circular attributes to behave
differently.

Cached attribute checking for circular evaluation used to be part of the
--componentCheck flag, so part of the original purpose of --componentCheck
is now removed, but the flag can still be used to generate code that
throws a runtime exception whenever a non-circular attribute is found to
be effectively circular.

[Jesper Öqvist]

Safer circular attribute evaluation

The --componentCheck flag could be used to ensure correct computation of circular attributes together with cached attributes. This is now always enabled using a improved implementation of the old component checking system.

The --componentCheck flag still adds exceptions if mutually circular attributes have a non-circular attribute in the cycle.

fixes #244 (bitbucket)

→ <<cset 18c936c38da5>>

[Niklas Fors]

Do you know how performance is affected by fix?

[Jesper Öqvist]

Can't compute performance for something that does not work.

[Niklas Fors]

You can measure the performance on programs that are not affected by this bug...

[Jesper Öqvist]

I think a better question is what the computation overhead is for lazy attributes in programs that don't use circular attributes. The answer is probably "not measurable", for any real program.

[Niklas Fors]

Yes. Or a program that use both lazy attributes and circular attributes, but don't have this behavior. I'm asking since the last fix for this bug made ExtendJ very slow, and by interpreting your answer, this fix will probably not affect the performance for ExtendJ.

[Jesper Öqvist]

While I do agree that it is worthwhile to measure programs that use both lazy and circular attributes, I do feel that the fix to this issue should always be enabled in this case, even if it adds some performance penalty. It helps expose incorrect attribute specifications earlier. For example, it did help with finding problematic attributes in ExtendJ (for example this issue would have been identified much earlier: https://bitbucket.org/extendj/extendj/issues/117/circularity-in-type-lookup).

The last time I tried to evaluate the component check option on ExtendJ there were incorrectly specified circular attributes in ExtendJ, making those results unusable. The component check option does not work anyway in ExtendJ since it disallows non-circular non-lazy attributes in a circular evaluation, which is problematic because such attribute declarations can still be functionally correct and are currently needed and unavoidable in ExtendJ due to inherited circular attributes not being supported (see issue #212 and #243).

I don't remember any other evaluation of a similar fix to this issue on ExtendJ. If there was one it was done on an incorrect circular attribute specification.

My guess is that there is no measurable difference in the performance of the current version of ExtendJ (8.0.1-16-g930e00d) with or without this fix. This is after some fixes to the circular attributes in ExtendJ. There may still be incorrect attributes in ExtendJ, but they just happen to compute something that doesn't cause a visible error in my tests.

I have not done any rigorous performance measurement on the current version of ExtendJ because I don't have time to do that, however I base my guess on the performance change on the following:

• I have not seen anything compile dramatically slower in ExtendJ yet.
• My intuition about the current fix for this issue makes me believe that I won't be able to measure a performance difference for ExtendJ. The reasoning is that the fix is inexpensive because it only affects lazy and circular attributes. Lazy attributes have some new code inserted after the cache check, which is never reached after the first evaluation. The circular attributes in ExtendJ already are so expensive that the additional work to detect and start a new circular evaluation does not make any difference.

You could argue for disabling the circular attribute fix when no circular attributes are present (see issue #160), but again the cost for lazy attributes is so small, so you'd have to actually measure that to know if it will be worth your time.

[Niklas Fors]

I just remember that Emma implemented a fix to this bug that made ExtendJ very slow. This may have been because of some bug in ExtendJ, I don't know, but the fix was reverted because of the performance results. So therefore I was just curious how this fix affected the performance.

It's great that you haven't seen any significant performance penalties.

I agree that this fix should be enabled by default.

[Jesper Öqvist]

I believe that the new implementation of this fix is more efficient than the old one, but I also think that there were some critical mistakes made when measuring on ExtendJ previously. I think we jumped to conclusions that made us incorrectly assume that the fix was causing ExtendJ to be slower. I've seen hangs or crashes for incorrect attribute systems, rather than slowdowns.