Don't normalize in AppliedType#superType #15453
Conversation
isProvisional should never force things, if it needs to complete a denotation, it should instead return false instead.
|
test performance please |
|
performance test scheduled: 5 job(s) in queue, 1 running. |
|
There are a number of smaller changes and one big: Move tryCompiletimeConstantFold to a new TypeEval object. The function itself was not changed, except updating the syntax to the recommended Scala 3 layout. |
| // We need to invalidate the cache when the period changes because the | ||
| // case `TermRef` of `Type#isStable` reads denotations, which depend on | ||
| // the period. See docs/_docs/internals/periods.md for more information. | ||
| if myIsStablePeriod != ctx.period then | ||
| if myisStableRunId != ctx.runId then |
There was a problem hiding this comment.
Looks like the comment needs to be updated to reflect the code, but I don't understand why this is correct since StableRealizable is not a FromStartFlags or AfterLoadFlags.
There was a problem hiding this comment.
StableRealizable is computed on completion, and the critical window is covered by is(Provisional).
There was a problem hiding this comment.
// We need to invalidate the cache when the run changes because the case
// `TermRef` of `Type#isStable` reads denotations, which depend on the
// run. See docs/_docs/internals/periods.md for more information. We do
// not need to check the phase because once a type is not provisional, its
// stability should not change anymore.Would this updated comment make sense?
|
Side note: I tried to add a |
|
Performance test finished successfully: Visit https://dotty-bench.epfl.ch/15453/ to see the changes. Benchmarks is based on merging with main (520beb2) |
|
What's up with the MiMa failures? There'a bunch of Tuple related stuff that's now flagged on all new PRs. |
|
Looks like they come from #15454 being force merged? |
|
To fix the MiMa tests, we should be able to re-run the tests after #15457 is merged (no-rebase needed). |
1 similar comment
|
To fix the MiMa tests, we should be able to re-run the tests after #15457 is merged (no-rebase needed). |
|
As predicted in #14864 (comment), caching
It also improves performance of one Tuple benchmark:
Benchmarks from https://dotty-bench.epfl.ch, lower is better. |
superTypeis intended to be a lightweight method, so it should not includenormalize. I ran into this when I tried to rename more calls tounderlyingtosuperTypeand running into cyclic reference errors. The problem is that we really need these renamings for correctness. So to be able to do this,normalizehas to be decoupled fromsuperTypefirst.