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

Parallelize position pickling #9619

Merged
merged 5 commits into from
Aug 25, 2020

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Aug 21, 2020

Run position pickling in parallel with other middle-end transformations. Only force in backend.

Run position pickling and byte array assembly in a separate task
# Conflicts:
#	compiler/src/dotty/tools/dotc/transform/Pickler.scala
@odersky
Copy link
Contributor Author

odersky commented Aug 21, 2020

test performance please

@dottybot
Copy link
Member

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

@dottybot
Copy link
Member

Performance test finished successfully:

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

Benchmarks is based on merging with master (387c562)

pickled
}(using ExecutionContext.global)
def force(): Array[Byte] = Await.result(pickledF, Duration.Inf)
if ctx.settings.YtestPickler.value || ctx.mode.is(Mode.Interactive) then force()
Copy link
Member

Choose a reason for hiding this comment

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

I tried removing the special-case for Interactive mode here and the CI passed: https://github.com/smarter/dotty/actions/runs/219013322, are you still able to reproduce the issue you saw that lead to that 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.

If the CI passes I assume it's OK, and we can do without the restriction.

@odersky odersky merged commit 0246f72 into scala:master Aug 25, 2020
@odersky odersky deleted the parallel-position-pickler branch August 25, 2020 13:22
@nicolasstucki
Copy link
Contributor

nicolasstucki commented Aug 26, 2020

@oderky it looks like this PR broke master. There is some issue when running the REPL tests.
https://github.com/lampepfl/dotty/runs/1029579877?check_suite_focus=true#step:10:967

@nicolasstucki
Copy link
Contributor

smarter added a commit that referenced this pull request Aug 26, 2020
This disables #9619 as it lead to CI failures.
smarter added a commit that referenced this pull request Aug 26, 2020
This disables #9619 as it lead to CI failures.
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.

5 participants