Fix #11885: Always clone trees if position already set #11960
Conversation
liufengyun
changed the title
liufengyun
force-pushed
the
fix-11885b
branch
2 times, most recently
from
f26c27b to
8e1fb65
Compare
There was a problem hiding this comment.
We should fix the non-determinism problem rather than working around it in the tests because we really want the compiler output to be idempotent to make builds reproducible: scala/scala-dev#405
liufengyun
force-pushed
the
fix-11885b
branch
3 times, most recently
from
7397e6f to
0bda3f9
Compare
| * On Windows we observe non-deterministic failure for the | ||
| * idempotency test tests/pos/i6507b.scala. The cause of the | ||
| * non-determinism is that the positions of the to be pickled | ||
| * trees might change in a later phase `Inlining` after pickling. |
There was a problem hiding this comment.
@nicolasstucki Could Inlining avoid changing tree positions? Otherwise we could copy the tree first, this should still be more efficient than forcing pickling early.
There was a problem hiding this comment.
Inlining is not supposed to change positions, Inlined nodes are supposed to have the same position as the original tree. Could be a bug setting the positions or maybe it is constant folding that removes some positions.
There was a problem hiding this comment.
Even if we fix the bug in inlining, it's so easy to re-introduce the bug in the future.
Or, we add an assertion in def span_= to check that spans are only set once. It seems to be a reasonable invariant.
|
test performance please |
|
performance test scheduled: 1 job(s) in queue, 0 running. |
|
Performance test finished successfully: Visit https://dotty-bench.epfl.ch/11960/ to see the changes. Benchmarks is based on merging with master (4ab402c) |
|
performance test scheduled: 1 job(s) in queue, 1 running. |
|
Performance test finished successfully: Visit https://dotty-bench.epfl.ch/11960/ to see the changes. Benchmarks is based on merging with master (86b4d62) |
liufengyun
changed the title
|
As an update, I've changed the fix to always clone if the position is not empty. For the Dotty project, the statistics is as follows:
Performance-wise, this should be better than synchronizing the pickling tasks. |
|
Since we're currently tracking a performance regression (#12153 (comment), #12229), I'd like to wait until we've fixed it before investigating the potential performance impact of this PR |
|
test performance please |
|
Performance test finished successfully: Visit https://dotty-bench.epfl.ch/11960/ to see the changes. Benchmarks is based on merging with master (b559209) |
| if (mySpan.isSynthetic) { | ||
| if (!mySpan.exists && span.exists) | ||
| envelope(source, span.startPos) // fill in children spans | ||
| this | ||
| this | ||
| else | ||
| cloneIn(source) | ||
| } | ||
| else cloneIn(source) |
There was a problem hiding this comment.
I think this can be simplified to:
if !mySpan.exists then
if span.exists then envelope(source, span.startPos) // fill in children spans
this
else
cloneIn(source)The cause of the problem is that we use parallelism in Pickler:
if !Pickler.ParallelPickling || ctx.settings.YtestPickler.value then force()
Sometimes on Windows, the futures run a little slower, and the later
phase `Inlining` can change the positions of the trees to be pickled,
thus non-determinism.
For the Dotty project, the statistics is as follows:
- Before: ntrees = 5331539
- After: ntrees = 5334075
Performance-wise, this should be better than synchronizing the pickling tasks.
|
retest performance please |
|
performance test scheduled: 1 job(s) in queue, 0 running. |
|
Performance test finished successfully: Visit https://dotty-bench.epfl.ch/11960-1/ to see the changes. Benchmarks is based on merging with master (af0de81) |
Fix #11885: Always clone trees if position already set
The cause of the problem is that we use parallelism in Pickler:
Sometimes on Windows, the futures run a little slower, and the later
phase
Inliningcan change the positions of the trees to be pickled,thus non-determinism.
For the Dotty project, the statistics is as follows:
Performance-wise, this should be better than synchronizing the pickling tasks.
[test_windows_full]