Fix overcompilation when inheriting inline defs #9730
Conversation
|
test performance please |
|
performance test scheduled: 8 job(s) in queue, 1 running. |
smarter
force-pushed
the
tree-hashing
branch
3 times, most recently
from
1bd2581 to
e73a3f4
Compare
|
Performance test finished successfully: Visit http://dotty-bench.epfl.ch/9730/ to see the changes. Benchmarks is based on merging with master (1ab71c1) |
smarter
force-pushed
the
tree-hashing
branch
from
e73a3f4 to
d79a1ba
Compare
I just realized that -Yforce-sbt-phases is not enabled in the benchmark suite by default so that performance test won't actually show anything: lampepfl/bench#452 |
|
test performance with #sbt please |
|
performance test scheduled: 8 job(s) in queue, 1 running. |
|
@liufengyun If I understand lampepfl/bench#452 correctly I think we need to test two commits with sbt (master and this PR) otherwise there's going to be some missing points right? |
|
Performance test finished successfully: Visit http://dotty-bench.epfl.ch/9730/ to see the changes. Benchmarks is based on merging with master (1ab71c1) |
|
Performance test finished successfully: Visit http://dotty-bench.epfl.ch/9730/ to see the changes. Benchmarks is based on merging with master (d11e137) |
|
second test seems to be better performance? |
I think that's plausible. Computing a tree hash looks faster than converting the tree to a string. |
When the body of an inline def changes, all its callers need to be recompiled. So when producing an API signature for an inline def to be sent to Zinc, we need to somehow include that body. So far, this was done by simply including the `toString` of the tree in the API signature, but the API of a class includes its inherited members, and when calling `toString` on an inherited inline def, its body ends up being printed as just `TreeUnpickler$LazyReader@123...` which is clearly not what we want, in practice this lead to overcompilation rather than undercompilation because the string includes the identityHashCode of the LazyReader which changes at each compilation run. This commit fixes this by implementing a proper way to hash trees, this is still not perfect since types aren't included in the hash (see comments in the code), but that can be addressed at a later point (and the exact strategy we want to use also depends on how we evolve Zinc).
Previously they were warnings, which means we could miss them in the CI.
After the previous commit which turned some warnings into errors, some tests like tests/pos/tasty-tags-obscure.scala started failing which revealed the problem. We need to use a custom marker to handle them because it turns out that zinc upstream doesn't handle them either even though they're a part of Scala 2 too!
When the body of an inline def changes, all its callers need to be
recompiled. So when producing an API signature for an inline def to be
sent to Zinc, we need to somehow include that body. So far, this was
done by simply including the
toStringof the tree in the APIsignature, but the API of a class includes its inherited members, and
when calling
toStringon an inherited inline def, its body ends upbeing printed as just
TreeUnpickler$LazyReader@123...which is clearlynot what we want, in practice this lead to overcompilation rather than
undercompilation because the string includes the identityHashCode of the
LazyReader which changes at each compilation run.
This commit fixes this by implementing a proper way to hash trees, this
is still not perfect since types aren't included in the hash (see
comments in the code), but that can be addressed at a later point (and
the exact strategy we want to use also depends on how we evolve Zinc).