-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Optimize and simplify SourcePosition handling #9561
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
I propose to get this in quickly. It's a significant simplification to what we had before. Before, report methods took SourcePositions which are expensive to create. So we did various tricks to avoid creating them, for instance by using Now, we have a new type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
One conceptual question: can we reuse Positioned
to play the role of SrcPos
?
test performance please |
performance test scheduled: 7 job(s) in queue, 1 running. |
performance test failed: Please check http://lamppc37.epfl.ch:8000/pull-9561-08-14-14.54.out for more information |
SourcePosition shows up high in the allocation charts: ~130'000 for typer/*.scala, i.e. 10 per line. We already have cut down on SourcePosition allocations by passing Positioned instead of SourcePosition where it makes sense. This PR generalizes this aproach: A new type `SrcPos` creates a `SourcePosition` on demand. it is implemented by `SourcePosition` itself, as well as `Positioned` and `Symbol`. Where possible we use `SrcPos` instead of `SourcePosition`, thereby delaying the creation of source positions. For typer/*.scala, this cut down allocations from 130K to 9K.
Co-authored-by: Fengyun Liu <[email protected]>
9369905
to
70d76ea
Compare
test performance please |
performance test scheduled: 8 job(s) in queue, 1 running. |
@liufengyun |
Performance test finished successfully: Visit http://dotty-bench.epfl.ch/9561/ to see the changes. Benchmarks is based on merging with master (17c5dc1) |
SourcePosition shows up high in the allocation charts: ~130'000 for
typer/.scala, i.e. 10 per line. We already have cut down on
SourcePosition allocations by passing Positioned instead of SourcePosition
where it makes sense. This PR generalizes this aproach: A new type
SrcPos
creates aSourcePosition
on demand. it is implementedby
SourcePosition
itself, as well asPositioned
andSymbol
.Where possible we use
SrcPos
instead ofSourcePosition
, therebydelaying the creation of source positions. For typer/.scala, this
cut down allocations from 130K to 9K.