Spread transform/traverse/mapOver into virtual methods of Tree/Type #6060
Conversation
|
Benchmarks scheduled to appear soon. I measured approx 4% speedup for the |
|
Reviving #5906, although I'd forgotten about the test failures that need investigation |
27e38bf to
0c04d64
Compare
| @@ -1129,6 +1131,7 @@ trait Types | |||
| override def safeToString: String = "<error>" | |||
| override def narrow: Type = this | |||
| override def kind = "ErrorType" | |||
| override def mapOver(map: TypeMap): Type = this | |||
There was a problem hiding this comment.
There are many override def mapOver(map: TypeMap): Type = this added in this commit, and they are unnecessary in principle. I suggest to either make mapOver in Type abstract or throw, or remove all redundant overrides.
There was a problem hiding this comment.
Hmm, I can't remember the sequence of how I arrived at this. I do remember running into problems when Type became a SAM and the inliner (prior to #5895) was overzealous. Maybe I made this non-abstract as a workaround.
|
I see a lot of in transforms. Are these mandatory with the new scheme? Will this likely break compiler plugins? |
|
They continue to work, transformers still have a |
|
Cool thanks :) |
I was aiming for cleaner stack traces in the transformers in order to make it a little easier to navigate through profiles. It didn't seem to make a difference to performance. |
0290c18 to
9186c9d
Compare
|
Anything more needed by the reviewers? |
This appears to perform better than the pattern match.
Possible factors at play:
- vtable lookup is constant time vs pattern match requires a cascade
of instanceof-s
- megamorphic invokevirtual has more stable JIT profiles than a the
match (parts of the match can appear to be dead code depending on
what phase of the compiler JIT profiles the method.)
- The new, small methods might in more inlinable into callers that
are effectively mono morphic.
…ixin" This reverts commit c2dc346.
|
There's a (trivial) conflict. I resolved and moved the revert to the last commit to make it easier to adapt the commit message. I didn't want to push -f to your branch. My branch is at https://github.com/adriaanm/scala/tree/faster/virtual-transform-minimal |
|
After that, LGTM |
9186c9d to
962112a
Compare
|
Added a merge commit to resolve the conflict with #6089 |
c847b52 to
02537a1
Compare
|
/rebuild |
|
Something was wonky with Jenkins in https://scala-ci.typesafe.com/job/scala-2.13.x-validate-main/567/console, the build flow claimed it was waiting for the integrate-ide job, but that has already finished. |
|
Merging on Adriaan's conditional LGTM. |
Analysis of JIT logs shows that a handful of very hot pattern matches in tree and type traversal run through ~10 deoptimizations before finding a steady state. Even once JITted, they show up in profiles as consuming a few percent of time, even though they are mostly just doing dispatching.
In this PR, I have changed them to virtual calls. Analysis of the JIT logs shows better stability for these methods, and the benchmarks suggest that the compiler has better peak performance.