Guard lookup in BoundSet::allSuperPairsWithCommonGenericTypeRecursive

[fedejeanne]

See proposal from @szarnekow in #3387 (comment)

The compilation time was further reduced from ~12 min to ~10.5 min 👏

Sample.zip

Contributes to #3327

[fedejeanne]

cc @stephan-herrmann , @jukzi

[jukzi]

While the change itself looks good, the description looks wrong. in #3387 (comment) you stated your build took only 9 min so the impact of this change alone should not be that drastic. Please create a dedicated Issue for this problem (its not a regression as in the other PRs), link it in the PR and provide before/after measurements that only account to this PR.

[fedejeanne]

While the change itself looks good, the description looks wrong. in #3387 (comment) you stated your build took only 9 min so the impact of this change alone should not be that drastic. Please create a dedicated Issue for this problem (its not a regression as in the other PRs), link it in the PR and provide before/after measurements that only account to this PR.

I think some wires got crossed: in the previous comment I stated that the build took ~12 min and it took time (or better said: took place) between ~9:23 and ~9:35

In any case, I can confirm that the build time after merging #3384 and #3387 took 721 s ...

... and the build with this PR took 626 s

So the total gain of this PR is 721s - 626s = 95s which is a reduction of 95 / 721 = 0.13 --> 13%.

[jukzi]

721s - 626s = 95s

ok, that matches the prediction in #3387 (comment). I got that 9 min wrong, sorry. But the 7 min stated in #3411 (comment) still do not match 626s (10,4 min)

Please also note that the provided snapshot still contains hotspots that should not be there. for example
spending 65 sec below org.eclipse.jdt.internal.core.builder.ClasspathDirectory.isPackage() does not make sense, so there should be much more room for optimization. or
still 36 sec below org.eclipse.jdt.internal.compiler.lookup.TypeBinding.findSuperTypeOriginatingFrom ()
12 sec below org.eclipse.osgi.internal.resolver.StateHelperImpl.getPackages () WTH

I hope you can contribute more performance improvements :-)

[jukzi]

or 40 sec below org.eclipse.pde.ds.internal.annotations.DSAnnotationCompilationParticipant.canSkipFile () really?

[fedejeanne]

But the 7 min stated in #3411 (comment) still do not match 626s (10,4 min)

😓 I changed the description, thank you!

Please also note that the provided snapshot still contains hotspots that should not be there. for example spending 65 sec below org.eclipse.jdt.internal.core.builder.ClasspathDirectory.isPackage() does not make sense, so there should be much more room for optimization. or still 36 sec below org.eclipse.jdt.internal.compiler.lookup.TypeBinding.findSuperTypeOriginatingFrom () 12 sec below org.eclipse.osgi.internal.resolver.StateHelperImpl.getPackages () WTH

I hope you can contribute more performance improvements :-)

I'll look into them and I'll try to provide PRs for them too 👍

But that doesn't affect this PR. A performance gain of 95s is also worth merging, don't you think?

[jukzi]

But that doesn't affect this PR. A performance gain of 95s is also worth merging, don't you think?

sure.

[fedejeanne]

@jukzi does this mean that this PR can be merged today? . I would like to roll-out this change in our IDE next week so some of our developers can start battle-testing it :-)

[jukzi]

Please wait to give @stephan-herrmann a chance to review. It's his code.

[stephan-herrmann]

Thanks @szarnekow @fedejeanne @jukzi , your continued interest in this issue led me to one more debugging session, where I made a few "interesting" observations:

• my use of TypeBinding.prototype() was well-intentioned but wrong, since ParameterizedTypeBinding constantly answers null , ups!
  • on closer look, we have something much better TypeBinding.id (the same id that is also used in TypeBinding.equalsEquals(). So no more worries about hashCode() in this issue :)
• to check effectiveness of the algorithm I tried to make the original tests from #2413 fail, but I couldn't
  • this could simply depend on some order that changed in the meanwhile
  • I'll definitely follow up on that
  • it's an unfortunate property of these details deep in type inference, that creating a test case to challenge one particular code region can be arbitrarily difficult.

With these observations, I left no stone unturned trying to let theory demonstrate correctness, where testing fails. As part of this polish I also avoid creating fresh result lists for every recursive invocation.

As for the performance impact my poor-man's measurements still show a tiny improvement over #3387

[szarnekow]

Previously we added (s, t) before checking equalsEquals. It's no longer the same algorithm, right?

Old:

s -> List<String>
t ->  Collection<String>
s.superInterfaces recurses into visiting (Collection<String>, Collection<String>) and adds that pair

Now:

s -> List<String>
t ->  Collection<String>
s.superInterfaces recurses into visiting (Collection<String>, Collection<String>) and exits immediately

Is that intentional?

[szarnekow]

That being said, would it be valid to exit after adding the pair in line 1278 of the old version without relying on equalsEquals(s, t)?

[stephan-herrmann]

Previously we added (s, t) before checking equalsEquals. It's no longer the same algorithm, right?
...
Is that intentional?

Yes, adding a pair of equal types is unnecessary. This change avoids that.

[stephan-herrmann]

That being said, would it be valid to exit after adding the pair in line 1278 of the old version without relying on equalsEquals(s, t)?

That might be difficult to show. At that point we only know that generic types (original()) are equal. How the differences in type arguments percolate into super types is a non-trivial question. As ecj now is more than twice as fast than javac (in my poor-man's measurements), I'd rather not invest more in this particular code section. There are probably areas more in need of optimization.