-
Notifications
You must be signed in to change notification settings - Fork 19
Multithreaded import scanning #236
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
CodSpeed Instrumentation Performance ReportMerging #236 will degrade performances by 80.31%Comparing Summary
Benchmarks breakdown
|
1d90a2a to
4fc6090
Compare
50f575b to
89359d4
Compare
89359d4 to
f4a669a
Compare
f4a669a to
f764d6c
Compare
| exclude_type_checking_imports, | ||
| ); | ||
| let imports_by_module_result = py.allow_threads(|| { | ||
| scan_for_imports_no_py( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Temporarily releases the GIL, thus allowing other Python threads to run
Which other python threads are running?
Are you sure allow_threads is needed? In the context of this commit it makes sense, but after the next commit, once joblib is removed, I'm unsure if it's still needed.
🐼 I think allow_threads is deprecating in favor of detach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted re. detach - I'll update when I update pyo3.
This was the crucial line I had to add to get things working - without it, everything was much slower - it didn't seem to be operating in parallel.
From https://pyo3.rs/main/parallelism.html
You should always call detach in situations that spawn worker threads, but especially so in cases where worker threads need to acquire the GIL, to prevent deadlocks.
(emphasis mine)
FWIW the explanation I got from Gemini of why this is was this:
Rayon's parallel iterators release the Global Interpreter Lock (GIL) internally, but pyo3 needs to be aware of this to ensure the Rust code can safely run in a separate thread without holding the GIL. The allow_threads block handles this by temporarily releasing the GIL, allowing Rayon to create its thread pool and execute the parallel workload. Without allow_threads, Rayon's thread spawn attempts would be blocked by the GIL, leading to deadlocks or other unexpected behavior.
| ] | ||
| requires-python = ">=3.9" | ||
| dependencies = [ | ||
| "joblib>=1.3.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
Peter554
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
I'm a but unsure if allow_threads is needed #236 (comment), but that's only minor
Prior to this PR, we used Python's joblib library to scan imports in parallel.
This PR, enabled by the refactor of #237, moves the parallelism to Rust-based multithreading instead of doing multiprocessing with Python.
The benchmarks say it'll make it much slower, but I'm not seeing that in practice: running this on a very large graph, uncached, seems to speed it up from ~11s to ~8s, locally (compared with Grimp's latest unyanked release, 3.9). With a fully populated cache it's about the same, which is what we'd expect since this change is limited to the uncached path.
This is good news because I had to yank the Grimp 3.10 release due to it performing unexpectedly poorly on an uncached graph - this should allow us to do another release.