Skip to content

Reduce IO calls in extractDependencies #18266

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.

Sign up for GitHub

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

Jump to bottom
Merged
bishabosha merged 6 commits into from
Aug 2, 2023
Merged

Reduce IO calls in extractDependencies #18266

bishabosha merged 6 commits into from
Aug 2, 2023

Conversation

@bishabosha
Copy link
Member

@bishabosha bishabosha commented Jul 21, 2023 •
edited
Loading

fix two hotspots in extractDependencies - checking if depFile is a class, (does file IO on isDirectory), and resolving sibling class file of a TASTy file (more file IO).

To fix this, only check the file extension, and also avoid unnecessary resolving of sibling .class file (always failing) when we have a .scala file as the associated file.

Second we can cache the associated class file of a TASTy file.

before:
Screenshot 2023-08-02 at 12 08 42

after:
Screenshot 2023-08-02 at 13 43 15

as you can see recordClassDependency after optimisation is now completely dominated by calling to zinc

@bishabosha bishabosha requested a review from nicolasstucki July 21, 2023 15:38
@bishabosha
Copy link
Member Author

bishabosha commented Jul 21, 2023

test performance please

@dottybot
Copy link
Member

dottybot commented Jul 21, 2023

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

bishabosha
bishabosha commented Jul 21, 2023
View reviewed changes
compiler/src/dotty/tools/dotc/core/Contexts.scala Outdated
def getFile(name: String): AbstractFile = getFile(name.toTermName)

def getSiblingClassfile(tastyFile: AbstractFile): AbstractFile =
base.siblingClassfiles.getOrElseUpdate(tastyFile, {
Copy link
Member Author

@bishabosha bishabosha Jul 21, 2023

Choose a reason for hiding this comment

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

we could possibly check before caching to see if Zinc is enabled.

Copy link
Contributor

@nicolasstucki nicolasstucki Jul 25, 2023

Choose a reason for hiding this comment

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

Did you check how many cache hits we get?

@dottybot
Copy link
Member

dottybot commented Jul 21, 2023

Performance test finished successfully:

Visit https://dotty-bench.epfl.ch/18266/ to see the changes.

Benchmarks is based on merging with main (7613234)

@bishabosha
Copy link
Member Author

bishabosha commented Jul 21, 2023

Performance test finished successfully:

Visit https://dotty-bench.epfl.ch/18266/ to see the changes.

Benchmarks is based on merging with main (7613234)

note that the sbt phases are not ran in the benchmark

nicolasstucki
nicolasstucki reviewed Jul 24, 2023
View reviewed changes
@bishabosha bishabosha force-pushed the reduce-extract-deps-io branch from e4d25ef to 314e731 Compare July 24, 2023 09:16
@bishabosha bishabosha requested a review from nicolasstucki July 24, 2023 13:17
@bishabosha
Copy link
Member Author

bishabosha commented Jul 24, 2023 •
edited
Loading

is there any objection to adding new caches to ContextBase like this?

nicolasstucki
nicolasstucki reviewed Jul 25, 2023
View reviewed changes
compiler/src/dotty/tools/dotc/core/Contexts.scala Outdated
def getFile(name: String): AbstractFile = getFile(name.toTermName)

def getSiblingClassfile(tastyFile: AbstractFile): AbstractFile =
base.siblingClassfiles.getOrElseUpdate(tastyFile, {
Copy link
Contributor

@nicolasstucki nicolasstucki Jul 25, 2023

Choose a reason for hiding this comment

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

Did you check how many cache hits we get?

@nicolasstucki
Copy link
Contributor

nicolasstucki commented Jul 25, 2023

is there any objection to adding new caches to ContextBase like this?

Not sure if that is a good idea.

@smarter WDYT?

smarter
smarter reviewed Jul 25, 2023
View reviewed changes
smarter
smarter reviewed Jul 25, 2023
View reviewed changes
compiler/src/dotty/tools/dotc/sbt/ExtractDependencies.scala Outdated
Comment on lines 139 to 141
// did not find associated class file, e.g. for a TASTy-only classpath.
// The file that Zinc recieves with binaryDependency is used to lookup any either any
// generated non-local classes or produced xsbti.API associated with the file.
Copy link
Member

@smarter smarter Jul 25, 2023

Choose a reason for hiding this comment

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

Does incremental compilation works at all with a tasty-only classpath? What happens if we just always pass to sbt the name of a .class file even if it doesn't exist?

Copy link
Member

@smarter smarter Jul 29, 2023

Choose a reason for hiding this comment

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

What happens if we just always pass to sbt the name of a .class file even if it doesn't exist?

I guess that would lead to overcompilation:
https://github.com/lampepfl/dotty/blob/a73316af791502b82525471816fe9153c36f845a/compiler/src/dotty/tools/dotc/core/SymDenotations.scala#L536-L540
But what happens if we pass the .tasty file instead?

Copy link
Member Author

@bishabosha bishabosha Jul 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

We decided that whatever we record as the binaryDependency will need to be emitted eventually as a real class file with generatedNonLocalClass

@bishabosha bishabosha force-pushed the reduce-extract-deps-io branch from dd5d0df to 1e5ac78 Compare July 31, 2023 13:47
@bishabosha bishabosha force-pushed the reduce-extract-deps-io branch from 1e5ac78 to 529eaf4 Compare July 31, 2023 13:51
@bishabosha bishabosha assigned smarter and unassigned nicolasstucki Jul 31, 2023
@bishabosha
Copy link
Member Author

bishabosha commented Jul 31, 2023 •
edited
Loading

one CI failure so far for ea75836 (scala3-staging/Compile/doc task) but I couldn't reproduce, running CI again

@bishabosha bishabosha force-pushed the reduce-extract-deps-io branch from 91e2b7e to daf159c Compare August 2, 2023 12:12
smarter
smarter approved these changes Aug 2, 2023
View reviewed changes
@smarter smarter removed their assignment Aug 2, 2023
@bishabosha bishabosha force-pushed the reduce-extract-deps-io branch from daf159c to 2dfc28f Compare August 2, 2023 12:35
@bishabosha bishabosha requested a review from smarter August 2, 2023 12:35
@bishabosha bishabosha enabled auto-merge (squash) August 2, 2023 12:41
@bishabosha
Copy link
Member Author

bishabosha commented Aug 2, 2023

test performance with #sbt please: ec59c31 2dfc28f

@dottybot
Copy link
Member

dottybot commented Aug 2, 2023

performance test scheduled for githubcomlampepfldottypull18266commits2dfc28ff27c14e33cf110456a7ddb6b596fb4525: 92 job(s) in queue, 1 running.

@dottybot
Copy link
Member

dottybot commented Aug 2, 2023

performance test failed:

Please check https://dotty-bench.epfl.ch/logs/ for more information

@bishabosha bishabosha merged commit 1b79cb3 into scala:main Aug 2, 2023
@bishabosha bishabosha deleted the reduce-extract-deps-io branch August 2, 2023 16:14
@bishabosha bishabosha added the backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. label Aug 3, 2023
@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
@Kordyjan
Copy link
Contributor

Kordyjan commented Dec 1, 2023

@bishabosha Does this also need to wait for #17594 backport?

@bishabosha
Copy link
Member Author

bishabosha commented Dec 1, 2023

Does this also need to wait for #17594 backport?

that's right

@Kordyjan Kordyjan added this to the 3.4.0 milestone Dec 20, 2023
@WojciechMazur WojciechMazur removed the backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. label Nov 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@smarter smarter smarter approved these changes

@nicolasstucki nicolasstucki Awaiting requested review from nicolasstucki

Assignees

@bishabosha bishabosha

Labels

None yet

Projects

None yet

Milestone

3.4.0

Development

Successfully merging this pull request may close these issues.

6 participants

@bishabosha @dottybot @nicolasstucki @Kordyjan @smarter @WojciechMazur