Compilers were unable to prove that the value at this address will not change, and they do not keep the previously loaded value live across the call; as a result, they reload it both in the or rmw operation and when checking two mutex.
To avoid reloading the value multiple times, we store it once in a register; this prevents the compilers from performing unnecessary load operations, allowing them to simply read the value and perform the OR operation on it. Of course, this puts additional pressure on the registers (in this case, callee-saved), but since they are callee-saved anyway, calls to other functions would save our registers regardless, we simply store our cached critical_section value in them; additionally, the extra push operations shouldn’t be a problem, since they likely fit into the same cache line, so when flushing values, this can be a single cache line flush. Also, here we only pay the cost of the extra registers once, at the beginning of the function. The previous version, on the other hand, re-reads the values each time, so the number of load operations is proportional to the number of critical_sections we iterate through.
Configured with:
../configure CC=clang CXX=clang++ \
--disable-gil --enable-optimizations
Built with:
make
before:
void
_PyCriticalSection_SuspendAll(PyThreadState *tstate)
{
#ifdef Py_GIL_DISABLED
uintptr_t *tagptr = &tstate->critical_section;
while (_PyCriticalSection_IsActive(*tagptr)) {
PyCriticalSection *c = untag_critical_section(*tagptr);
if (c->_cs_mutex) {
PyMutex_Unlock(c->_cs_mutex);
if ((*tagptr & _Py_CRITICAL_SECTION_TWO_MUTEXES)) {
PyCriticalSection2 *c2 = (PyCriticalSection2 *)c;
if (c2->_cs_mutex2) {
PyMutex_Unlock(c2->_cs_mutex2);
}
}
}
*tagptr |= _Py_CRITICAL_SECTION_INACTIVE;
tagptr = &c->_cs_prev;
}
#endif
}x86_64
clang version 22.1.8
gcc (GCC) 16.2.1 have same problem.
before:
critical_section.o: file format elf64-x86-64
Disassembly of section .text:
0000000000000150 <_PyCriticalSection_SuspendAll>:
150: 55 push rbp
151: 41 56 push r14
153: 53 push rbx
154: 48 8b 9f b0 00 00 00 mov rbx,QWORD PTR [rdi+0xb0]
15b: 48 85 db test rbx,rbx
15e: 74 40 je 1a0 <_PyCriticalSection_SuspendAll+0x50>
160: 89 d8 mov eax,ebx
162: 83 e0 01 and eax,0x1
165: 75 39 jne 1a0 <_PyCriticalSection_SuspendAll+0x50>
167: 48 83 e3 fc and rbx,0xfffffffffffffffc // We set 2 bits of the original `critical_section` to zero
...
178: f0 0f b0 11 lock cmpxchg BYTE PTR [rcx],dl
17c: 75 61 jne 1df <_PyCriticalSection_SuspendAll+0x8f>
17e: f6 87 b0 00 00 00 02 test BYTE PTR [rdi+0xb0],0x2 // Since we lost the bottom 2 bits of `critical_section*`, we perform another load to check the value in bit 1
185: 75 6f jne 1f6 <_PyCriticalSection_SuspendAll+0xa6>
187: 80 8f b0 00 00 00 01 or BYTE PTR [rdi+0xb0],0x1 // We're spending uops on a load to perform an OR operation, when we could have stored the value earlier in a register—say, r8.
18e: 48 8b 03 mov rax,QWORD PTR [rbx]
...
1a4: c3 ret
1a5: 31 ed xor ebp,ebp
1a7: 49 89 c6 mov r14,rax
1aa: 49 83 e6 fc and r14,0xfffffffffffffffc
1ae: 49 8b 7e 08 mov rdi,QWORD PTR [r14+0x8]
1b2: 48 85 ff test rdi,rdi
1b5: 74 0e je 1c5 <_PyCriticalSection_SuspendAll+0x75>
1b7: b0 01 mov al,0x1
1b9: f0 40 0f b0 2f lock cmpxchg BYTE PTR [rdi],bpl
1be: 75 2f jne 1ef <_PyCriticalSection_SuspendAll+0x9f>
1c0: f6 03 02 test BYTE PTR [rbx],0x2 // load
1c3: 75 5b jne 220 <_PyCriticalSection_SuspendAll+0xd0>
1c5: 80 0b 01 or BYTE PTR [rbx],0x1 // load
...
after:
void
_PyCriticalSection_SuspendAll(PyThreadState *tstate)
{
#ifdef Py_GIL_DISABLED
uintptr_t* tagptr = &tstate->critical_section;
uintptr_t critical_section = *tagptr;
while (_PyCriticalSection_IsActive(critical_section)) {
PyCriticalSection *c = untag_critical_section(critical_section);
if (c->_cs_mutex) {
PyMutex_Unlock(c->_cs_mutex);
if ((critical_section & _Py_CRITICAL_SECTION_TWO_MUTEXES)) {
PyCriticalSection2 *c2 = (PyCriticalSection2 *)c;
if (c2->_cs_mutex2) {
PyMutex_Unlock(c2->_cs_mutex2);
}
}
}
*tagptr = critical_section | _Py_CRITICAL_SECTION_INACTIVE;
tagptr = &c->_cs_prev;
critical_section = c->_cs_prev;
}
#endif
}after:
critical_section.o: file format elf64-x86-64
Disassembly of section .text:
0000000000000150 <_PyCriticalSection_SuspendAll>:
150: push rbp
151: push r15
153: push r14
155: push rbx
156: push rax
157: mov r14,QWORD PTR [rdi+0xb0]
15e: test r14,r14
161: je 1ad <_PyCriticalSection_SuspendAll+0x5d>
163: mov eax,r14d
166: and eax,0x1
169: jne 1ad <_PyCriticalSection_SuspendAll+0x5d>
16b: mov rbx,r14
16e: and rbx,0xfffffffffffffffc
172: mov rcx,QWORD PTR [rbx+0x8]
176: test rcx,rcx
179: je 18f <_PyCriticalSection_SuspendAll+0x3f>
17b: xor edx,edx
17d: mov al,0x1
17f: lock cmpxchg BYTE PTR [rcx],dl
183: jne 1f9 <_PyCriticalSection_SuspendAll+0xa9>
185: test r14b,0x2
189: jne 213 <_PyCriticalSection_SuspendAll+0xc3>
18f: or r14,0x1 // no load
193: mov QWORD PTR [rdi+0xb0],r14
...
1b8: xor ebp,ebp
1ba: mov r15,r14
1bd: and r15,0xfffffffffffffffc
1c1: mov rdi,QWORD PTR [r15+0x8]
1c5: test rdi,rdi
1c8: je 1d9 <_PyCriticalSection_SuspendAll+0x89>
1ca: mov al,0x1
1cc: lock cmpxchg BYTE PTR [rdi],bpl
1d1: jne 20c <_PyCriticalSection_SuspendAll+0xbc>
1d3: test r14b,0x2
1d7: jne 241 <_PyCriticalSection_SuspendAll+0xf1>
1d9: or r14,0x1
1dd: mov QWORD PTR [rbx],r14
...
This is not expected to be a hot path, but the change removes redundant loads and memory read-modify-write operations from each iteration. I would appreciate feedback on whether this trade-off is worthwhile.
Compilers were unable to prove that the value at this address will not change, and they do not keep the previously loaded value live across the call; as a result, they reload it both in the
orrmw operation and when checkingtwo mutex.To avoid reloading the value multiple times, we store it once in a register; this prevents the compilers from performing unnecessary load operations, allowing them to simply read the value and perform the OR operation on it. Of course, this puts additional pressure on the registers (in this case, callee-saved), but since they are callee-saved anyway, calls to other functions would save our registers regardless, we simply store our cached
critical_sectionvalue in them; additionally, the extrapushoperations shouldn’t be a problem, since they likely fit into the same cache line, so when flushing values, this can be a single cache line flush. Also, here we only pay the cost of the extra registers once, at the beginning of the function. The previous version, on the other hand, re-reads the values each time, so the number ofloadoperations is proportional to the number ofcritical_sections we iterate through.before:
x86_64
clang version 22.1.8
gcc (GCC) 16.2.1 have same problem.
before:
after:
after:
This is not expected to be a hot path, but the change removes redundant loads and memory read-modify-write operations from each iteration. I would appreciate feedback on whether this trade-off is worthwhile.