GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||
GeezerVison is showing github.com — gh-158031: Keep the iter sentinel alive during comparison by lul-cas · Pull Request #158046 · python/cpython Conversation
|
|
This introduces contention on the FT build. How does it affect performance? PRs touching builtins are delicate and should not just be generated through an LLM. Anyone submitting such PRs should have a deeper understanding of the interpreter and the consequences so please keep in mind that we rather have core devs tackle such issues. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Fair point on the free threaded build. I dont have a benchmark so I cant tell you how the extra reference behaves under contention I've checked the bug itself. The comparison was using a borrowed sentinel and a reentrant "eq" could free it before the comparison finished. With this extra reference, the tests for that case passed and the object stays alive until the comparison is done I agree that builtins are delicate and that this is the kind of change you'd rather have a core dev to handle. I'm happy to adjust it, or leave it, whichever you prefer. I may have underestimated that.. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Please do so then. We usually expect a PR that changes performance critical code to justify the change (the change is correct but the impact is the hard question) |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
I ran next() on iter(callable, sentinel) on the free-threaded build, macOS 26.5 arm64, with --disable-gil --enable-optimizations --with-lto. The sentinel is a fresh object(), so the refcount actually changes. Just one thread. main: 26.4 ns +/- 0.3 ns Little slower. Around 2.7 ns per "next()". The callable only returns 0, so this is pretty much the worst case. The extra incref/decref is a big chunk of a tiny loop. I didnt try several threads sharing the same sentinel.. Maybe worth looking for another way to fix this? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
The other thing we can do is: do nothing. Sometimes it is not worth it just for purity as such cases are rare. It slows down every code path for theoretical issues. OTOH, the sentinel form is also rarer. We can document this as a real crasher. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Ok, that makes sense. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
uaf of it_sentinel in callable_iterator.__next__
There was an error while loading. Please reload this page.