gh-115999: Add free-threaded specialization for CONTAINS_OP#126450
gh-115999: Add free-threaded specialization for CONTAINS_OP#126450corona10 merged 2 commits intopython:mainpython/cpython:mainfrom corona10:gh-115999-contain-opcorona10/cpython:gh-115999-contain-opCopy head branch name to clipboard
Conversation
|
I just followed up on the change #126410. The specialization mechanism of this opcode is the same with COMPARE_OP IIUC, so it should be added too. |
mpage
left a comment
There was a problem hiding this comment.
Thanks! Would you please add a short explanation to either the pull request description or commit message outlining why the specialization logic + specialized instructions are thread-safe? I think what's here is correct, but it would be helpful for someone unfamiliar with this code, or if it turns out that our assumptions are invalid.
In this case, I think something like the following would be fine:
- The specialization logic determines the appropriate specialization using only the operand's type, which is safe to read non-atomically (changing it requires stopping the world). We are guaranteed that the type will not change in between when it is checked and when we specialize the bytecode because the types involved are immutable (you cannot assign to
__class__for exact instances ofdict,set, orfrozenset). The bytecode is mutated atomically using helpers. - The specialized instructions rely on the operand type not changing in between the
DEOPT_IFchecks and the calls to the appropriate type-specific helpers (e.g._PySet_Contains). This is a correctness requirement in the default builds and there are no changes to the opcodes in the free-threaded builds that would invalidate this.
markshannon
left a comment
There was a problem hiding this comment.
In future, can you please leave sufficient time to review PRs before merging.
Less than 24 hours is not enough. This PR was opened 5pm and merged at 4am UK time.
| got = self.get_disassembly(co, adaptive=True) | ||
| self.do_disassembly_compare(got, call_quicken) | ||
|
|
||
| @cpython_only |
There was a problem hiding this comment.
This is not a test for the dis module. It should be moved to the appropriate place, probably test_opcache.
Also, please make this a behavioral test.
In other words, tests that the behavior is the same before and after specialization.
Ah, sorry, I was too hurry. |
…thongh-126450) - The specialization logic determines the appropriate specialization using only the operand's type, which is safe to read non-atomically (changing it requires stopping the world). We are guaranteed that the type will not change in between when it is checked and when we specialize the bytecode because the types involved are immutable (you cannot assign to `__class__` for exact instances of `dict`, `set`, or `frozenset`). The bytecode is mutated atomically using helpers. - The specialized instructions rely on the operand type not changing in between the `DEOPT_IF` checks and the calls to the appropriate type-specific helpers (e.g. `_PySet_Contains`). This is a correctness requirement in the default builds and there are no changes to the opcodes in the free-threaded builds that would invalidate this.
…thongh-126450) - The specialization logic determines the appropriate specialization using only the operand's type, which is safe to read non-atomically (changing it requires stopping the world). We are guaranteed that the type will not change in between when it is checked and when we specialize the bytecode because the types involved are immutable (you cannot assign to `__class__` for exact instances of `dict`, `set`, or `frozenset`). The bytecode is mutated atomically using helpers. - The specialized instructions rely on the operand type not changing in between the `DEOPT_IF` checks and the calls to the appropriate type-specific helpers (e.g. `_PySet_Contains`). This is a correctness requirement in the default builds and there are no changes to the opcodes in the free-threaded builds that would invalidate this.
--disable-gilbuilds #115999