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.

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

Add support for Pipelined builds #18880

Merged
merged 7 commits into from
Apr 4, 2024
Merged

Add support for Pipelined builds #18880

merged 7 commits into from
Apr 4, 2024

Conversation

bishabosha
Copy link
Member

@bishabosha bishabosha commented Nov 8, 2023 •
edited
Loading

This includes support for a single pass pipelined build, compatible with sbt's ThisBuild/usePipelining,

  • adds -Ypickle-java and -Ypickle-write flags, expected by Zinc when pipelining is enabled in sbt.
  • when -Ypickle-write <directory|jar> is set, then write tasty from pickler to that output, (building upon First step to pipelining support - enable reading Java symbols from TASTy #19074 support for Java signatures in TASTy files).
  • call apiPhaseCompleted and dependencyPhaseCompleted callbacks, which will activate early downstream compilation
  • calls generatedNonLocalClass callbacks early, which enables Zinc to run the incremental algorithm before starting downstream compilation (including checking for macro definitions).

generally this can be reviewed commit-by-commit, as they each do an isolated feature.

As well as many tests in the sbt-test/pipelining directory, this has also been tested locally on akka/akka-http, apache/incubator-pekko, lichess-org/lila, scalacenter/scaladex, typelevel/fs2, typelevel/http4s, typelevel/cats, slick/slick.

This PR sets the ground work for an optional 2-pass compile (reusing the OUTLINEattr), which should use a faster frontend (skipping rhs when possible) before producing tasty signatures

fixes #19743

@bishabosha bishabosha added the needs-minor-release This PR cannot be merged until the next minor release label Nov 8, 2023
@bishabosha bishabosha added the release-notes Should be mentioned in the release notes label Nov 8, 2023
@bishabosha bishabosha force-pushed the topic/zinc-pipelining branch 2 times, most recently from bede0a0 to da582f7 Compare November 9, 2023 21:07
@bishabosha
Copy link
Member Author

bishabosha commented Nov 11, 2023 •
edited
Loading

We should probably add tests of incremental compilation with pipelining turned on, just to be sure there are no concurrency issues

@bishabosha
Copy link
Member Author

TODO: sbt will write -Ypickle-java and -Ypickle-write twice in the Test scope (basically prepend to whatever was there), so we need to filter that out

@bishabosha bishabosha assigned bishabosha and unassigned sjrd Nov 22, 2023
@bishabosha
Copy link
Member Author

bishabosha commented Nov 23, 2023 •
edited
Loading

We decided I should separate out the Java pickling ability as that is the only change that requires a minor release (changing TASTy Format), this will make the review much more simple to digest - then the rest of the features could come in patch releases (but I hope to still land this before 3.4.0)

edit: done in #19074

sjrd
sjrd previously requested changes Nov 23, 2023
View reviewed changes
Copy link
Member

@sjrd sjrd left a comment

Choose a reason for hiding this comment

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

Review of everything except the pipelining commit (but including the Java pickling commit).

compiler/src/dotty/tools/dotc/classpath/DirectoryClassPath.scala Show resolved Hide resolved
compiler/src/dotty/tools/dotc/util/EnumFlags.scala Outdated Show resolved Hide resolved
compiler/src/dotty/tools/io/AbstractFile.scala Outdated Show resolved Hide resolved
compiler/src/dotty/tools/io/FileExtension.scala Outdated Show resolved Hide resolved
compiler/src/dotty/tools/io/Path.scala Outdated Show resolved Hide resolved
compiler/src/dotty/tools/dotc/core/Definitions.scala Outdated Show resolved Hide resolved
compiler/src/dotty/tools/dotc/core/Phases.scala Outdated Show resolved Hide resolved
compiler/src/dotty/tools/dotc/core/SymDenotations.scala Outdated Show resolved Hide resolved
compiler/src/dotty/tools/dotc/core/tasty/TreeUnpickler.scala Outdated Show resolved Hide resolved
compiler/src/dotty/tools/dotc/core/tasty/TreePickler.scala Outdated Show resolved Hide resolved
@bishabosha bishabosha marked this pull request as draft November 24, 2023 16:58
@bishabosha
Copy link
Member Author

bishabosha commented Nov 24, 2023 •
edited
Loading

#19074 should be merged before this
Edit: its merged :)

@bishabosha bishabosha force-pushed the topic/zinc-pipelining branch 4 times, most recently from 713b005 to e722ae2 Compare November 28, 2023 14:10
@bishabosha bishabosha removed the needs-minor-release This PR cannot be merged until the next minor release label Nov 28, 2023
@bishabosha bishabosha force-pushed the topic/zinc-pipelining branch 4 times, most recently from bd85c84 to 08460c4 Compare November 28, 2023 15:56
@bishabosha bishabosha mentioned this pull request Feb 1, 2024
Draft
nicolasstucki
nicolasstucki reviewed Feb 9, 2024
View reviewed changes
Copy link
Contributor

@nicolasstucki nicolasstucki left a comment

Choose a reason for hiding this comment

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

What is the status of this PR?

@bishabosha
Copy link
Member Author

I need to rebase, which I aim to do today

@bishabosha bishabosha force-pushed the topic/zinc-pipelining branch 2 times, most recently from b32569e to ba2cc3f Compare February 12, 2024 17:51
@bishabosha
Copy link
Member Author

bishabosha commented Feb 12, 2024 •
edited
Loading

I have a couple of usability bits to backport from my outlining branch (e.g. handle duplication of Ypickle-write and -Ypickle-java in test scope by Zinc), and probably move the pipeline jar writing to a separate (skippable) phase, then I can change the PR not to be draft

@bishabosha bishabosha force-pushed the topic/zinc-pipelining branch from ba2cc3f to b087eab Compare February 14, 2024 14:56
@bishabosha bishabosha force-pushed the topic/zinc-pipelining branch from b087eab to 5ce20cb Compare February 22, 2024 15:51
@bishabosha bishabosha force-pushed the topic/zinc-pipelining branch 3 times, most recently from 7134df9 to 7e11005 Compare March 16, 2024 00:56
@bishabosha
Copy link
Member Author

bishabosha commented Mar 19, 2024 •
edited
Loading

This PR is feature complete, there is one extra enhancement which is to asynchronously write the TASTy files to the early output - should it be included here?

Edit: @sjrd recommends to do that in a follow up PR.

@bishabosha bishabosha marked this pull request as ready for review March 19, 2024 08:13
@bishabosha bishabosha force-pushed the topic/zinc-pipelining branch from 7e11005 to 2b48e2e Compare March 19, 2024 12:28
@bishabosha bishabosha requested a review from sjrd March 19, 2024 12:30
@bishabosha bishabosha assigned sjrd and unassigned bishabosha Mar 19, 2024
@bishabosha bishabosha force-pushed the topic/zinc-pipelining branch from 2b48e2e to dcb06b2 Compare March 19, 2024 13:43
@bishabosha
Remove the findClass method from ClassPath
5d6a0a8 
the method was never used, and not well defined, e.g. with branches
to search in both tasty files and class files, which could be
severely inefficient.
@bishabosha
use an enum to store file extensions,
8168d1e 
This caches common file extensions, while still being extensible.
Also fixes many operations with unexpected behavior (manipulation of
file extensions where toLowerCase behaves differently with certain locales.)
@bishabosha
Notify Zinc about non-local classes early
a975398 
For pipelining Zinc needs to know about non-local classes early.
e.g. it enables Zinc to disable pipelining if a non-local class
contains macros.

The changes in this commit are based of changes made originally in Zinc:
sbt/zinc@856d416
@bishabosha bishabosha force-pushed the topic/zinc-pipelining branch from dcb06b2 to 6e471d1 Compare March 19, 2024 14:45
@bishabosha
Copy link
Member Author

I will not make be making any changes to this PR unless review demands it

@bishabosha bishabosha dismissed sjrd’s stale review April 4, 2024 12:55

changes addressed

@bishabosha
add support for pipeline build of Scala and Java files
c22e313 
- rename '-Yjava-tasty-output' to '-Yearly-tasty-output' because
  now Scala TASTy will also be written to this destination.
- add '-Ypickle-java' alias of '-Yjava-tasty', as expected by Zinc
- add '-Ypickle-write' alias of '-Yearly-tasty-output', as expected by Zinc
- move ExtractAPI phase to after Pickler, this way we can do it in
  parallel with generating TASTy bytes. At the end of this phase we
  write the TASTy to the '-Yearly-tasty-output' destination.
  Also ensure that ExtractAPI phase runs with '-Yjava-tasty', even if no
  incremental callback is set (don't extract the API in this case).
- test the pipelining with sbt scripted tests, including for inline
  methods and macros with pipelining
- describe semantics with respect to suspensions,
  introduce -Yno-suspended-units flag for greater control by the user.
@bishabosha
fix prediction in ProgressCallbackTest
94162a3
@bishabosha
support test scope by ignoring repeated pipelining flags
20d635d
@bishabosha
add test to show incremental compilation works under pipelining
c19b67e
@bishabosha bishabosha force-pushed the topic/zinc-pipelining branch from 6e471d1 to c19b67e Compare April 4, 2024 14:32
@bishabosha bishabosha enabled auto-merge April 4, 2024 14:47
@bishabosha bishabosha merged commit a7f00e2 into scala:main Apr 4, 2024
18 checks passed
@bishabosha bishabosha deleted the topic/zinc-pipelining branch April 4, 2024 16:14
@eed3si9n eed3si9n mentioned this pull request Apr 21, 2024
Merged
@Kordyjan Kordyjan added this to the 3.5.0 milestone May 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolasstucki nicolasstucki nicolasstucki left review comments

@sjrd sjrd sjrd approved these changes

@smarter smarter Awaiting requested review from smarter

Assignees

@sjrd sjrd

Labels
release-notes Should be mentioned in the release notes
Projects
None yet
Milestone
3.5.0
Development

Successfully merging this pull request may close these issues.

Support Zinc pipelining
4 participants
@bishabosha @sjrd @nicolasstucki @Kordyjan