gh-115999: Specialize LOAD_SUPER_ATTR in free-threaded builds
#127128
Conversation
Use existing helpers to atomically modify the bytecode. Add unit tests to ensure specializing is happening as expected. Add test_specialize.py that can be used with ThreadSanitizer to detect data races.
There was a problem hiding this comment.
This looks pretty good to me, but is a significant regression on the free-threaded builds. I think the regression is coming from the critical section that is acquired in BEGIN_TYPE_LOCK before calling super_lookup_lock_held. It looks like some of the functions called by super_lookup_lock_held already acquire the type lock when necessary (e.g. _super_lookup_descr); we are forced to release and reacquire the critical section each time these functions are called. Do we need to hold the type lock before calling super_lookup_lock_held?
Edit: For completeness, this looks perf neutral on default builds.
I see. In that case, it seems |
It seems `do_super_lookup()` and `supercheck()` are safe to call and so `_PySuper_Lookup()` doesn't need a lock for the type object. This avoids a performance regression in FT builds.
|
This is the worst regression on the benchmark suite:
A new benchmark run is in-progress and it looks like that regression is gone (which would be expected since the code hasn't changed). |
|
New benchmark results look good. Performance is neutral / slightly improved overall; the benchmarks that we expect this to help (e.g. I think you need to fix the merge conflicts before CI will run tests. |
…pythongh-127128) Use existing helpers to atomically modify the bytecode. Add unit tests to ensure specializing is happening as expected. Add test_specialize.py that can be used with ThreadSanitizer to detect data races. Fix thread safety issue with cell_set_contents().
…pythongh-127128) Use existing helpers to atomically modify the bytecode. Add unit tests to ensure specializing is happening as expected. Add test_specialize.py that can be used with ThreadSanitizer to detect data races. Fix thread safety issue with cell_set_contents().
Use existing helpers to atomically modify the bytecode. Fix thread safety issue with
cell_set_contents(). Add unit tests to ensure specializing is happening as expected.Note: this needs gh-127272 to be merged as well in order to avoid thread safety issues.
--disable-gilbuilds #115999