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 precompiles generation #47983

Merged
merged 23 commits into from
Dec 29, 2022

Conversation

petvana
Copy link
Member

@petvana petvana commented Dec 23, 2022

This PR parallelizes generate_precompile.jl. It saves half a minute building Julia, and, thanks to using Channel, the order of the precompiles should be still fully deterministic.

PR

...
    JULIA usr/lib/julia/sys-o.a
Generating REPL precompile statements... 40/40
Executing precompile statements... 1750/1754
Precompilation complete. Summary:
Total ─────── 112.036051 seconds
...

Master

...
    JULIA usr/lib/julia/sys-o.a
Generating REPL precompile statements... 40/40
Executing precompile statements... 1748/1752
Precompilation complete. Summary:
Generation ── 121.136666 seconds 80.6719%
Execution ───  29.023100 seconds 19.3281%
Total ─────── 150.159765 seconds
...

The next optimization would be to read precompile_file on the fly, that would save several seconds extra.

@petvana petvana added the building Build system, or building Julia or its dependencies label Dec 23, 2022
@petvana petvana marked this pull request as draft December 23, 2022 23:25
@petvana petvana marked this pull request as ready for review December 23, 2022 23:36
@IanButterworth
Copy link
Member

Seems like a good thing to do, but it could make the precompilation process harder to debug. It might be good to be able to fallback to the current serial approach for debugging purposes. I guess putting optional waits on the @async tasks could be a simple way to do that?

Also I think the progress printing needs figuring out. It's currently a bit chaotic and hard to interpret.

end

# Collect statements from running the script
mktempdir() do prec_path
@async mktempdir() do prec_path
Copy link
Member

Choose a reason for hiding this comment

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

These should have errormonitor on them so that their failure is loud

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you, should be loud now.

Copy link
Member Author

Choose a reason for hiding this comment

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

@vchuravy errormonitor causes CI to fail for some reason. Is the current approach with fetch(step1) == :ok || throw("Step 1 of collecting precompiles failed.") sufficient?

@petvana
Copy link
Member Author

petvana commented Dec 27, 2022

I've tried to improve the printing (link to asciinema below) and add PARALLEL_PRECOMPILATION = false option for serial debugging

@petvana
Copy link
Member Author

petvana commented Dec 27, 2022

Not sure, where the current CI problem comes from. I cannot reproduce it locally.

@IanButterworth
Copy link
Member

Try rebasing?

@petvana petvana marked this pull request as draft December 27, 2022 22:42
@IanButterworth
Copy link
Member

The REPL precomp generation script includes intentionally throwing errors. Perhaps that's why errormonitor etc was surfacing errors that were previously ignored?

@petvana
Copy link
Member Author

petvana commented Dec 28, 2022

The REPL precomp generation script includes intentionally throwing errors. Perhaps that's why errormonitor etc was surfacing errors that were previously ignored?

Perhaps, but I'm unable to confirm locally.

@petvana petvana marked this pull request as ready for review December 28, 2022 14:52
@petvana
Copy link
Member Author

petvana commented Dec 29, 2022

Besides errormonitor, the PR seems ready. Suggestions for improving the progress printing welcomed.

@IanButterworth
Copy link
Member

IanButterworth commented Dec 29, 2022

Looks great.
For formatting maybe this, where Waiting etc. are colored and the counts for the categories are colored appropriately. Also perhaps "Basic" instead of "normal"?

Precompile statements (Waiting, Running, Finished): 
└ Collect 1748 (Basic: 729, REPL 40/40: 1653) => Execute 1744 (4 failed)
Precompilation complete. Summary:
Total ─────── 114.175765 seconds
Outputting sysimage file...
Output ────── 162.738189 seconds

@IanButterworth
Copy link
Member

Looking great. Disabling the cursor is best with a try-finally because it can be annoying to lose it if the build process fails

Copy link
Member

@IanButterworth IanButterworth left a comment

Choose a reason for hiding this comment

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

Great!

@IanButterworth
Copy link
Member

I realized the 4 we're reporting as failures are just comments being skipped, so (I hope you don't mind) I pushed a fix for that

@IanButterworth IanButterworth force-pushed the pv/parallel-repl-statements-v2 branch from c7c05da to a1ee9af Compare December 29, 2022 17:36
@IanButterworth IanButterworth added the merge me PR is reviewed. Merge when all tests are passing label Dec 29, 2022
@IanButterworth
Copy link
Member

The next optimization would be to read precompile_file on the fly, that would save several seconds extra.

It seems like --trace-compile=$precompile_file is populated at the end of the process. Making that more parallel might take a lot of internals work

@KristofferC
Copy link
Member

We could flush the strean for every statement printed maybe.

@IanButterworth
Copy link
Member

I actually think I might be wrong, and we're already flushing. Investigating..

@petvana
Copy link
Member Author

petvana commented Dec 29, 2022

It seems like --trace-compile=$precompile_file is populated at the end of the process. Making that more parallel might take a lot of internals work

Actually, they are populated during the process. I've already tried a very naive approach and it seems to work.
petvana/julia@pv/parallel-repl-statements-v2...petvana:julia:pv/parallel-repl-statements-on-the-fly-v2

@IanButterworth
Copy link
Member

Nice. I've been trying an approach that doesn't re-read and unique the statements each loop, though it's not working yet

@petvana
Copy link
Member Author

petvana commented Dec 29, 2022

I propose to leave the asynchronous reading to a different PR because it adds one more level of complexity (and space for possible dead-locks).

@IanButterworth
Copy link
Member

Sounds good to me

@IanButterworth
Copy link
Member

This seems to work without re-reading the total file each time IanButterworth@5ee671a?w=1

@petvana
Copy link
Member Author

petvana commented Dec 29, 2022

This seems to work without re-reading the total file each time IanButterworth@5ee671a?w=1

Nice, but I'm slightly worried about readline if it may split the line before it's fully written. I'd rather use readavailable and split manually.

Moreover, we should wait until precompile_file exists.

@IanButterworth
Copy link
Member

Builds are passing, but win 32 tests are reporting failure without an obvious reason. Updating from master just to check

@vtjnash
Copy link
Member

vtjnash commented Dec 29, 2022

readline will never do that (if reading from a proper stream/pipe), but readavailable will often do that

@KristofferC KristofferC merged commit a8adb39 into JuliaLang:master Dec 29, 2022
@KristofferC
Copy link
Member

Wow, I didn't mean to merge this. I tried to merge it locally using gh merge and it merged it here instead 😆

@IanButterworth
Copy link
Member

Wasn't even a squash merge. That seems to easy to do accidentally..!

@IanButterworth IanButterworth removed the merge me PR is reviewed. Merge when all tests are passing label Dec 29, 2022
@KristofferC
Copy link
Member

I think it asked me and since I thought it was local I thought whatever. The last step said "submit" which in hind sight probably should have made me more alert...

@petvana
Copy link
Member Author

petvana commented Dec 29, 2022

readline will never do that (if reading from a proper stream/pipe), but readavailable will often do that

It seems possible if someone flushes (that is a correct think to do). The following MWE

t = tempname()
w = open(t, "w")
r = open(t, "r")
tr = @async begin
    for i = 1:30
        sleep(0.1)
        line = readline(r)
        line == "" && continue
        println(line)
    end
end

print(w, "a+")
flush(w)
sleep(0.3)
print(w, "b\n")
close(w)

wait(tr)

prints only

a+

but it is not a full line.

@vtjnash
Copy link
Member

vtjnash commented Dec 30, 2022

A file is not a stream/pipe (unless it was created with mkfifo)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
building Build system, or building Julia or its dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants