| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
I'll get to reviewing in an hour or so. At a glance:
|
Sorry, something went wrong.
There was a problem hiding this comment.
Some comments. I'll yield to you on whether or not it's worth trying to do this locklessly :)
Sorry, something went wrong.
Let's circle back to this later. My initial thought is that backporting would make sense. What do you mean about making debuggers unhappy?
Perhaps. I don't anticipate there being enough simultaneous attempts that the overhead of any contention would matter.
I wanted to start off with this. I'm not convinced that an actual freelist of thread states is worth it. Conceptually it made sense as a solution for the pre-allocated thread state. I debated on having a dedicated field for just that one vs. do the freelist thing. Clearly I went with the latter, but now I'm thinking the illusion of a freelist isn't the best thing. I may switch it back. |
Sorry, something went wrong.
Some low-level debuggers and profilers (something like PyStack, py-spy, and probably some others that I don't know about) might get a nasty surprise if we change where the main thread is stored. I don't think it's that important, as they're already aware of the maintenance burden that comes with relying on private implementation details, but it's not too convincing for a backport. (It's probably easier for downstream if we backport GH-126915, and then just put this on main for going forward, but I'm definitely biased 😄)
I'm not too worried about the subinterpreter case, but instead for just normal multithreading under one interpreter. This will add additional overhead to calling something like PyGILState_Ensure with a lot of threads, and users might not be happy with how it scales. |
Sorry, something went wrong.
There was a problem hiding this comment.
Ok, it looks like you've removed the freelist now. (I would change the title for the commit message.)
This looks pretty good, and I think this is small enough to not worry about giving debuggers too much of a headache. I'll do one final pass a little later today once you've figured out what you want to do about resets and then let's merge :)
Sorry, something went wrong.
It is still found at PyInterpreterState._initial_thread, so it shouldn't be a problem.
I'd be surprised if lock/atomic contention here were more than insignificant relative to any of the other operations at play. Also keep in mind that PyMutex is mostly a user-space lock, so some of the overhead of contention is reduced (relative to a kernel-provided lock). |
Sorry, something went wrong.
I noticed! I like this fix better than mine with the extra field, it's clever. In fact, we could reuse the preallocated field as a makeshift freelist for later; heap-allocated thread states could also get stored in that field, and then alloc_threadstate will use them just fine.
Yeah, contention on those functions is probably not great right now anyways because of the HEAD_LOCK. I'm more talking generally, and trying to think ahead about what we should do in the future. |
Sorry, something went wrong.
There was a problem hiding this comment.
LGTM
Sorry, something went wrong.
|
I'll add the backport to 3.13 label once gh-126995 is merged. Otherwise sorting out the ABI data file becomes a much bigger pain. |
Sorry, something went wrong.
⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️Hi! The buildbot aarch64 Android 3.x has failed when building commit 1c0a104. What do you need to do:
You can take a look at the buildbot page here: https://buildbot.python.org/#/builders/1594/builds/622 Failed tests:
Failed subtests:
Summary of the results of the build (if available): == Click to see traceback logsTraceback (most recent call last):
File "/data/user/0/org.python.testbed/files/python/lib/python3.14/test/test_os.py", line 4499, in test_timerfd_ns_select
self.assertEqual(self.read_count_signaled(fd), 1)
~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: 2 != 1
Traceback (most recent call last):
File "/data/user/0/org.python.testbed/files/python/lib/python3.14/test/test_os.py", line 4334, in test_timerfd_select
self.assertEqual(self.read_count_signaled(fd), 1)
~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: 2 != 1
|
Sorry, something went wrong.
|
In hindsight, we probably should have ran buildbots before merging. I'm not sure that's related though. |
Sorry, something went wrong.
|
I'm fairly sure it's unrelated. |
Sorry, something went wrong.
…yInterpreterState Field (pythongh-126989) This approach eliminates the originally reported race. It also gets rid of the deadlock reported in pythongh-96071, so we can remove the workaround added then.
…PyInterpreterState Field (gh-127114) This approach eliminates the originally reported race. It also gets rid of the deadlock reported in gh-96071, so we can remove the workaround added then. This is mostly a cherry-pick of 1c0a104 (AKA gh-126989). The difference is we add PyInterpreterState.threads_preallocated at the end of PyInterpreterState, instead of adding PyInterpreterState.threads.preallocated. That avoids ABI disruption.
…yInterpreterState Field (pythongh-126989) This approach eliminates the originally reported race. It also gets rid of the deadlock reported in pythongh-96071, so we can remove the workaround added then.
| Back | FazBrowse Home | New Git URL |
This approach eliminates the originally reported race. It also gets rid of the deadlock reported in gh-96071, so we can remove the workaround added then.