Skip to content
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

Loading symbols from TASTy files directly #17594

Merged
merged 2 commits into from
Jul 3, 2023

Conversation

nicolasstucki
Copy link
Contributor

@nicolasstucki nicolasstucki commented May 26, 2023

Before this PR we used to parse the classfiles first and when we found a classfile that has a TASTY attribute we switched and loaded the tasty. Now we find the .tasty files directly and load the symbols directly from them. We still load the class files to check that the UUID in that classfile matches the UUID in the TASTy file.

When looking for classes in the classpath, we prioritize the TASTy files over classfiles. This implies that the symbol loader will receive the .tasty files for Scala 3 code and .class for Scala 2 and Java code.

A variant of the ClassfileParser called ClassfileTastyUUIDParser was added to have a way to check the UUID in the TASTY attribute of the classfile. The ClassfileParser could not be used directly because it eagerly tries to initialize parts of the symbols that are already loaded from the TASTy file, causing some conflicts.

Open question: should we only check the TASTy UUID under some flag to avoid loading both the .tasty and the .class files? The second commit introduces this check.

@nicolasstucki nicolasstucki self-assigned this May 26, 2023
@nicolasstucki nicolasstucki force-pushed the load-symbols-from-tasty branch 9 times, most recently from 95cb5c4 to 26e8f85 Compare June 2, 2023 06:39
@nicolasstucki nicolasstucki force-pushed the load-symbols-from-tasty branch 4 times, most recently from e545d54 to 5213b9e Compare June 6, 2023 08:02
@nicolasstucki nicolasstucki marked this pull request as ready for review June 6, 2023 11:26
@nicolasstucki nicolasstucki requested a review from smarter June 6, 2023 11:27
@bishabosha
Copy link
Member

This is also going to be very useful for TASTy-only class paths, such as the coming changes to include build-pipelining

@smarter
Copy link
Member

smarter commented Jun 28, 2023

@bishabosha Can I assign this PR to you?

@bishabosha bishabosha assigned bishabosha and unassigned smarter Jun 28, 2023
@bishabosha bishabosha self-requested a review June 30, 2023 11:43
@@ -1,2 +1,2 @@
List(/tastyPaths/I8163.class)
List(/tastyPaths/I8163.tasty)
Copy link
Member

Choose a reason for hiding this comment

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

does this count as an API change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. This falls into undefined behavior in the documentation. Therefore we can change it.

This comes from a call to quotes.reflect.SourceFile.current. In the inspector, we explicitly state that this method will not work

https://github.com/lampepfl/dotty/blob/c62909009506171b03e092c4a391483b044d3b49/scaladoc/src/scala/tasty/inspector/Inspector.scala#L27

@nicolasstucki nicolasstucki force-pushed the load-symbols-from-tasty branch from 5213b9e to f9e8b36 Compare June 30, 2023 13:11
@bishabosha bishabosha self-requested a review June 30, 2023 17:45
Copy link
Member

@bishabosha bishabosha left a comment

Choose a reason for hiding this comment

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

I did a benchmark on my machine of memory usage by compiling org-lichess/lila (140k scala LOC) and did not observe any significant change between either including the check or not, and the old way of loading tasty

@bishabosha
Copy link
Member

test performance please

@nicolasstucki
Copy link
Contributor Author

I restarted the tests to make sure we do not have any conflicts before we merge this.

@nicolasstucki nicolasstucki enabled auto-merge July 3, 2023 12:39
@nicolasstucki nicolasstucki merged commit 65c7072 into scala:main Jul 3, 2023
@nicolasstucki nicolasstucki deleted the load-symbols-from-tasty branch July 3, 2023 14:50
@Kordyjan Kordyjan added this to the 3.4.0 milestone Aug 1, 2023
@mbovel
Copy link
Member

mbovel commented Aug 7, 2023

This PR might have caused a performance regression:

image

See https://dotty-bench.epfl.ch.

@bishabosha
Copy link
Member

bishabosha commented Aug 15, 2023

there's just a lot more IO being done here now, i wonder if we can work more on assumption or caching rather than constantly asking the file system for proof of things

@bishabosha
Copy link
Member

I can't really reproduce any large spikes caused by these changes

@nicolasstucki
Copy link
Contributor Author

Are we sure that the CPU clock speed is still limited? In the past we had some issues with that when the machine rebooted.

@bishabosha
Copy link
Member

bishabosha commented Aug 22, 2023

Why should this be 3.4? It doesnt have any effect on the compatibility - also it will be needed to bring pipelining to 3.3

@bishabosha bishabosha added the backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. label Aug 22, 2023
@nicolasstucki
Copy link
Contributor Author

It does not introduce any binary compatibility, but some external tools might start behaving differently. For example, if a build tool does not properly invalidate/remove the old .tasty files (only .class files) we could encounter an issue. We decided that we should wait and see what happens in 3.4. If some issues are found, then the build tools can be fixed. Later, when we are sure that all tools work properly we can backport it to 3.3.

@Kordyjan Kordyjan removed the backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. label Oct 10, 2023
@bishabosha bishabosha added the backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. label Oct 11, 2023
@bishabosha
Copy link
Member

bishabosha commented Oct 11, 2023

@Kordyjan this would be eventually necessary if we ever want to have build pipelining support for scala 3.3, it seems from @nicolasstucki comment we should only wait to see if it's a problem or not. The proposed flowchart in #18636 would not reject this PR (or we need clarity on what breaking forward compatibility means)

@bishabosha
Copy link
Member

Alternatively, I am proposing a new file extension for "signature" tasty files used in pipelining or other purposes, which would depend on some of the changes here, so that would be more simple to nominate for backport (older scala version would simply ignore the file, and pipelining flags would be ignored)

@Kordyjan Kordyjan removed the backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. label Nov 20, 2023
@bishabosha
Copy link
Member

bishabosha commented Nov 21, 2023

I have noticed a 3% slowdown compiling lichess-org/lila with 3.3.1 vs current main build, didn't verify if this is a cause -

Scala 3.3.1 (clean; System.gc; compile runs after cold boot sbt)
time (s) Memory (GB)
107 5.72
71 5.79
68 6.28
64 5.31
65 6.10
64 6.34
Scala 3.4.0-RC1-bin-SNAPSHOT (clean; System.gc; compile runs after cold boot sbt)
time (s) Memory (GB)
107 4.97
74 6.40
68 6.04
68 5.71
67 7.23
66 6.54

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.

6 participants