Skip to content

Improve Source Positions #5644

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

Closed
wants to merge 75 commits into from
Closed

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Dec 19, 2018

The goal of this PR is to be able to produce the full source position of a tree without
relying on context in Inlined nodes. Such context is fragile because it is very hard to preserve
during staging and other operations on Tasty trees. On the other hand, we do not want to spend one extra field per tree. So the idea is to use the uniqueId field of a tree as an index to determine the source file in which it was created.

@odersky odersky force-pushed the add-sourcepos-2 branch 2 times, most recently from 672199e to 63765a7 Compare December 23, 2018 11:56
@odersky odersky force-pushed the add-sourcepos-2 branch 3 times, most recently from 042615f to 457f814 Compare January 3, 2019 18:36
@odersky
Copy link
Contributor Author

odersky commented Jan 4, 2019

Status so far:

  • Trees now produce full source positions.
  • Source positions are encoded by the uniqueId of the tree and its offset range.
  • Offset ranges are renamed from Position to Span
  • SourcePositions are no longer generated implicitly from the context position.

Still to be done:

  • Make the tests pass. There is some funky stuff going on in ReplTests where the same SourceFile gets created several time (i.e. copies have the same file component) but some copies have empty contents. These weird source files are in the context when we create trees in REPL tests. They seem to upset message renderings, adding an empty line. This problem did not get noticed before because by the time we rendered the messages we did have the source with the right contents in the context. But now that trees get source positions at the time they are created this does not work any longer.

  • Rename SourcePosition to Position, aligning uses of the term with Dotty backend, JS backend and Tasty.

  • Rename SourceFile to Source, avoiding the confusion with AbstractFile.

@odersky
Copy link
Contributor Author

odersky commented Jan 4, 2019

@nicolasstucki I am done with it for now. Please have a look to see what we can do for the tests.

@odersky
Copy link
Contributor Author

odersky commented Jan 4, 2019

Rebased. I was hoping that the recently changes to ReplCompiler had an effect, but no luck.

@nicolasstucki nicolasstucki force-pushed the add-sourcepos-2 branch 2 times, most recently from 7a92e02 to 216a89a Compare January 5, 2019 13:23
@nicolasstucki
Copy link
Contributor

Rebased again due to conflicts

@odersky
Copy link
Contributor Author

odersky commented Jan 5, 2019

test performance please

@dottybot
Copy link
Member

dottybot commented Jan 5, 2019

performance test scheduled: 1 job(s) in queue, 0 running.

@@ -38,12 +38,6 @@ class VirtualFile(val name: String, override val path: String) extends AbstractF
this.content = content
}

override def hashCode: Int = path.hashCode
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems a shame to lose that. Should virtual files not be structurally equal? I fear if they are not we get a ripple down effect with e.g. too many source file entries in contexts since duplicates are no longer discovered.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I already noticed that this will not work as interested. I will roll back this change and use unique paths for all the virtual source files created by the repl. Currently all paths are <console> or rs$line$i which the wrong memorized source file.

@dottybot
Copy link
Member

dottybot commented Jan 5, 2019

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/5644/ to see the changes.

Benchmarks is based on merging with master (5b7d5b6)

@odersky
Copy link
Contributor Author

odersky commented Jan 6, 2019

Performance test is a lot worse, which is what I feared. There's a price to pay for carrying around all these source positions. We have to see how to optimize that.

@odersky
Copy link
Contributor Author

odersky commented Jan 6, 2019

Rebased to master

@nicolasstucki nicolasstucki force-pushed the add-sourcepos-2 branch 3 times, most recently from e8ff6b0 to c946591 Compare January 6, 2019 18:01
@nicolasstucki
Copy link
Contributor

Rebased

nicolasstucki and others added 27 commits January 15, 2019 14:01
To avoid the trying to load virtual files based on their pickled names.
Now we keep only flags on the parameters (except the first one), so that parameter positions don't overlap.
Previously they where infered from the mods.
Used to depend on file's path, but that does not work for VirtualFiles
that might be different even though they have the same path.
Use interned strings for the comparison of their paths.
Reduce concurrent hashmap lookups in TreeIds by caching the last result in SourceInfo.
This reduces lookups for typer/*.scala from 500K+ to 7.6K.
Deemphasize AbstractFile, drop TreeIds.
When set, prints full source positions, not just file + line.
The decision when to pickle a position is a time sink. Try to optimize
this by not going through Positioned#{initialSpan,elemSource} functionality.
Single traversal to set id and span.
It's called more often now that we generation source positions. Time
for some caching.
Don't check whether a sourcefile exists when creating it - it might
lead to spurious errors. Do it instead at the point where we create
a CompilationUnit from a source file.
@odersky
Copy link
Contributor Author

odersky commented Jan 15, 2019

Superseded by #5713.

@odersky odersky closed this Jan 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants