Skip to content

use new zinc 1.8 api for VirtualFile #18137

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
merged 10 commits into from
Jul 23, 2023
Merged

use new zinc 1.8 api for VirtualFile #18137

merged 10 commits into from
Jul 23, 2023

Conversation

@bishabosha
Copy link
Member

@bishabosha bishabosha commented Jul 4, 2023 •
edited
Loading

this prepares the compiler to be able to call new API's in Zinc, by implementing infrastructure to call back into Zinc with VirtualFile.

A good side effect of this change is improved performance when using sbt to "clean compile" code, as extra bridging methods from java.io.File to xsbti.VirtualFile in Zinc are avoided.

Also remains compatible with Zinc 1.3, to do this we introduce a new interface dotty.tools.dotc.sbt.interfaces.IncrementalCallback, this abstracts over VirtualFile, and so the sbt-bridge handles the entire mapping between VirtualFile to AbstractFile, and then back from SourceFile to either VirtualFile or java.io.File, depending on the implementation.

Even if we remove Zinc 1.3 support, we probably want to keep the new interface because all the VirtualFile mapping logic can be contained in sbt-bridge

@bishabosha bishabosha requested review from adpi2 and smarter July 4, 2023 15:36
smarter
smarter reviewed Jul 5, 2023
View reviewed changes
@bishabosha bishabosha force-pushed the topic/zinc-api-1.8.0-upgrade-only branch from 16f9ef0 to 0c8dcf3 Compare July 10, 2023 08:54
ckipp01
ckipp01 reviewed Jul 10, 2023
View reviewed changes
ckipp01
ckipp01 reviewed Jul 10, 2023
View reviewed changes
@bishabosha bishabosha force-pushed the topic/zinc-api-1.8.0-upgrade-only branch 3 times, most recently from e29e82a to 16b91ba Compare July 10, 2023 13:32
@bishabosha bishabosha mentioned this pull request Jul 10, 2023
Closed
5 tasks
@adpi2 adpi2 linked an issue Jul 10, 2023 that may be closed by this pull request
Follow-up on Zinc upgrade #10816
Closed
5 tasks
@bishabosha
use new zinc api for virtualfile
a7cec5f
@bishabosha
use compiler impl of protobuf for presentation compiler
34a5e45 
zinc 1.4 removes the dependency on protobuf-java, so fails
to compile mtags-shared due to the now missing transitive
dependency.

The compiler implements protobuf streams for semanticdb, so reuse
it in the source generator for mtags-shared.
@bishabosha bishabosha force-pushed the topic/zinc-api-1.8.0-upgrade-only branch from 8c22ff5 to 5e9fd97 Compare July 12, 2023 14:21
@bishabosha bishabosha force-pushed the topic/zinc-api-1.8.0-upgrade-only branch from 5e9fd97 to 2458b8f Compare July 12, 2023 17:01
@bishabosha
Copy link
Member Author

bishabosha commented Jul 12, 2023 •
edited
Loading

@smarter the files and sources caches get destroyed on each new run, so the initial source files still need to be stored separately

@bishabosha bishabosha force-pushed the topic/zinc-api-1.8.0-upgrade-only branch from b5c096f to 95ed582 Compare July 13, 2023 13:31
@bishabosha bishabosha requested a review from smarter July 13, 2023 16:04
@bishabosha
Copy link
Member Author

This is ready to go

smarter
smarter reviewed Jul 13, 2023
View reviewed changes
@bishabosha bishabosha force-pushed the topic/zinc-api-1.8.0-upgrade-only branch from 036107a to 8f128fd Compare July 13, 2023 20:36
@bishabosha bishabosha requested a review from smarter July 13, 2023 20:47
@bishabosha
Copy link
Member Author

bishabosha commented Jul 17, 2023 •
edited
Loading

@smarter I now only do the hash map lookup, no longer underlying, and AbstractZincFile is gone

smarter
smarter reviewed Jul 17, 2023
View reviewed changes
smarter
smarter reviewed Jul 17, 2023
View reviewed changes
@bishabosha bishabosha force-pushed the topic/zinc-api-1.8.0-upgrade-only branch from e99b9e5 to 5fde72c Compare July 17, 2023 14:05
@bishabosha bishabosha requested a review from smarter July 17, 2023 14:06
smarter
smarter reviewed Jul 17, 2023
View reviewed changes
smarter
smarter reviewed Jul 17, 2023
View reviewed changes
@bishabosha bishabosha requested a review from smarter July 17, 2023 14:53
@bishabosha
Copy link
Member Author

test performance please

@dottybot
Copy link
Member

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

@dottybot
Copy link
Member

Performance test finished successfully:

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

Benchmarks is based on merging with main (046ba60)

@bishabosha
Copy link
Member Author

again not sure if the benchmark regression was this or something older

@bishabosha
Copy link
Member Author

bishabosha commented Jul 18, 2023 •
edited
Loading

I did my own analysis with async-profiler - this is a clear win over the status quo - e.g. on main b7e797d this is the flame graph for binaryDependency:
Screenshot 2023-07-18 at 19 25 12

and with this PR:
Screenshot 2023-07-18 at 19 25 33

so there's an improvement from 9-11% of ExtractDependencies.run being spent on binaryDependency to only 3%, as now we avoid the extra conversions in zinc to convert the sourcefile from java.io.File then to java.nio.file.Path and then to VirtualFile.

Similar improvements exist for usedName and classDependency, but not for api because that's dominated by actually doing the hashing

smarter
smarter requested changes Jul 19, 2023
View reviewed changes
Copy link
Member

@smarter smarter left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM.

@smarter smarter assigned bishabosha and unassigned smarter Jul 19, 2023
@bishabosha bishabosha requested a review from smarter July 21, 2023 11:59
smarter
smarter approved these changes Jul 23, 2023
View reviewed changes
Copy link
Member

@smarter smarter left a comment

Choose a reason for hiding this comment

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

🚀

@smarter smarter merged commit 5afe621 into scala:main Jul 23, 2023
@smarter smarter deleted the topic/zinc-api-1.8.0-upgrade-only branch July 23, 2023 18:58
@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
Copy link
Member Author

it seems IntelliJ has issues building projects with a compiler version after this was merged, maybe someone else could confirm?

@bishabosha bishabosha mentioned this pull request Nov 1, 2023
Merged
@Kordyjan Kordyjan mentioned this pull request Dec 1, 2023
Merged
Kordyjan added a commit that referenced this pull request Dec 8, 2023
@Kordyjan
Backport "use new zinc 1.8 api for VirtualFile" to LTS (#19128)
32a6cfc 
Backports #18137 to the LTS branch.

PR submitted by the release tooling.
[skip ci]
@Kordyjan Kordyjan added this to the 3.3.2 milestone Dec 14, 2023
@bishabosha bishabosha mentioned this pull request Jul 12, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@eed3si9n eed3si9n eed3si9n left review comments

@julienrf julienrf julienrf left review comments

@ckipp01 ckipp01 ckipp01 left review comments

@smarter smarter smarter approved these changes

@adpi2 adpi2 Awaiting requested review from adpi2

Assignees

@bishabosha bishabosha

Labels

None yet

Projects

None yet

Milestone

3.3.2

Development

Successfully merging this pull request may close these issues.

Follow-up on Zinc upgrade

8 participants

@bishabosha @dottybot @smarter @eed3si9n @julienrf @adpi2 @ckipp01 @Kordyjan