RFR: 8342090: Infer::IncorporationBinaryOp::equals can produce side-effects [v2]

[Maurizio Cimadamore]

On Wed, 23 Oct 2024 05:22:47 GMT, Vicente Romero <vromero at openjdk.org> wrote:

>> Type inference uses a cache to store incorporation operations already done. This way we can avoid redoing operations that once done won't produce any change. This can reduce the compilation time when the number of inference variables involved is not trivial. The elements in the cache are implemented with class `com.sun.tools.javac.comp.Infer::IncorporationBinaryOp` the problem here is that the equals method of this class can produce side effects, implying that after a comparison the compared objects can mutate. This patch is fixing this issue,
>> 
>> TIA
>
> Vicente Romero has updated the pull request incrementally with one additional commit since the last revision:
> 
>   stress tests

Looks good. I've suggested a couple of simplifications.
I believe we need another way to test. I think, for now, let's include the tests we have in related JBS issues, and run them with both normal and alternate (`-XX:hash=2`) strategies. Then, in a separate JBS issue, maybe figure out how to run a subset of tests automatically, using the alternate hashing.

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Infer.java line 1253:

> 1251:             return (o instanceof IncorporationBinaryOp incorporationBinaryOp)
> 1252:                     && opKind == incorporationBinaryOp.opKind
> 1253:                     && types.isSameType(undetVarToTVar(op1), undetVarToTVar(incorporationBinaryOp.op1))

I believe we probably need to compute these things only once - as these mappings are expensive. I wonder if this code is trying to do too much. All we are after here is a way to check that we haven't already executed an incorporation op. This is only really created in one place (`doIncorporationOp`). So I suggest a reshuffling:

* rename this class IncorporationBinaryOpKey, and make it a record
* the key record will always hold "unmapped types" (the record constructor can do the unmapping)
* get rid of the `apply` method
* in `doIncorporationOp`, create a key, check if a result is already in the cache. If so return it. If not, compute a new result, by calling `apply` on the incorporation op kind (with the "original" types).

-------------

PR Review: https://git.openjdk.org/jdk/pull/21651#pullrequestreview-2388140758
PR Review Comment: https://git.openjdk.org/jdk/pull/21651#discussion_r1812441797