-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
Add lock around coverage operations #59517
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
Conversation
Any performance numbers for how this affects the runtime of a "typical" coverage run? |
Haven't measured but uncontended locks are very cheap. If you are contending on the coverage it starts losing performance |
xref #59355 |
We should run julia-buildkite CI with this branch before merging so that the |
The coverage job on JuliaCI/julia-buildkite#482 seems to have aborted early? Or has somehow sped up significantly.. it's to tell hard because the logs are huge and tests don't pass. I didn't see the test report on a quick look. |
It seems the macOS run was actually complete.. huh, went from 5h -> 3h 26m. Maybe nothing else was running on the mac at that time? Linux slowed down 6h 3m -> 6h 38m https://buildkite.com/julialang/julia-buildkite-scheduled/builds/1421#01992fb5-bdfb-4986-bd65-5499c11a8746 Maybe there's too much noise to be sure whether it's a real slowdown, but we should at least increase the timeout from 7h given it seems to be relatively close even on master under usual CI conditions (I saw shorter times than this during the weekend when I adjusted that timeout down from 12h). There's probably also tests we should just skip on the coverage jobs.. like just run the threads_exec stuff once. |
Seems about right then. The main risk with seeing it get a lot faster is likely just that it might be crashing more / failing more tests, and thus losing coverage. Doesn't seem true here. The threads_exec test itself isn't actually slow, it is just has a test that threads can sleep for a minute added recently unnecessarily and then run repeatedly unnecessarily. |
No description provided.