From 19b71a619326a62427be517c884913c07c2029c4 Mon Sep 17 00:00:00 2001 From: Donghee Na Date: Mon, 2 Dec 2024 23:14:34 +0900 Subject: [PATCH 1/2] gh-115999: Use light-weight lock for UNPACK_SEQUENCE_LIST --- Include/internal/pycore_opcode_metadata.h | 2 +- Include/internal/pycore_uop_metadata.h | 2 +- Python/bytecodes.c | 13 +++---------- Python/executor_cases.c.h | 19 +++---------------- Python/generated_cases.c.h | 19 +++---------------- 5 files changed, 11 insertions(+), 44 deletions(-) diff --git a/Include/internal/pycore_opcode_metadata.h b/Include/internal/pycore_opcode_metadata.h index d63c8df8ca6690..81dde66a6f26c2 100644 --- a/Include/internal/pycore_opcode_metadata.h +++ b/Include/internal/pycore_opcode_metadata.h @@ -2148,7 +2148,7 @@ const struct opcode_metadata _PyOpcode_opcode_metadata[266] = { [UNARY_NOT] = { true, INSTR_FMT_IX, HAS_PURE_FLAG }, [UNPACK_EX] = { true, INSTR_FMT_IB, HAS_ARG_FLAG | HAS_ERROR_FLAG | HAS_ESCAPES_FLAG }, [UNPACK_SEQUENCE] = { true, INSTR_FMT_IBC, HAS_ARG_FLAG | HAS_ERROR_FLAG | HAS_ESCAPES_FLAG }, - [UNPACK_SEQUENCE_LIST] = { true, INSTR_FMT_IBC, HAS_ARG_FLAG | HAS_DEOPT_FLAG | HAS_ESCAPES_FLAG }, + [UNPACK_SEQUENCE_LIST] = { true, INSTR_FMT_IBC, HAS_ARG_FLAG | HAS_DEOPT_FLAG }, [UNPACK_SEQUENCE_TUPLE] = { true, INSTR_FMT_IBC, HAS_ARG_FLAG | HAS_DEOPT_FLAG }, [UNPACK_SEQUENCE_TWO_TUPLE] = { true, INSTR_FMT_IBC, HAS_ARG_FLAG | HAS_DEOPT_FLAG }, [WITH_EXCEPT_START] = { true, INSTR_FMT_IX, HAS_ERROR_FLAG | HAS_ESCAPES_FLAG }, diff --git a/Include/internal/pycore_uop_metadata.h b/Include/internal/pycore_uop_metadata.h index 1825bb3a5abc80..89fce193f40bd8 100644 --- a/Include/internal/pycore_uop_metadata.h +++ b/Include/internal/pycore_uop_metadata.h @@ -112,7 +112,7 @@ const uint16_t _PyUop_Flags[MAX_UOP_ID+1] = { [_UNPACK_SEQUENCE] = HAS_ARG_FLAG | HAS_ERROR_FLAG | HAS_ESCAPES_FLAG, [_UNPACK_SEQUENCE_TWO_TUPLE] = HAS_ARG_FLAG | HAS_DEOPT_FLAG, [_UNPACK_SEQUENCE_TUPLE] = HAS_ARG_FLAG | HAS_DEOPT_FLAG, - [_UNPACK_SEQUENCE_LIST] = HAS_ARG_FLAG | HAS_DEOPT_FLAG | HAS_ESCAPES_FLAG, + [_UNPACK_SEQUENCE_LIST] = HAS_ARG_FLAG | HAS_DEOPT_FLAG, [_UNPACK_EX] = HAS_ARG_FLAG | HAS_ERROR_FLAG | HAS_ESCAPES_FLAG, [_STORE_ATTR] = HAS_ARG_FLAG | HAS_NAME_FLAG | HAS_ERROR_FLAG | HAS_ESCAPES_FLAG, [_DELETE_ATTR] = HAS_ARG_FLAG | HAS_NAME_FLAG | HAS_ERROR_FLAG | HAS_ESCAPES_FLAG, diff --git a/Python/bytecodes.c b/Python/bytecodes.c index c07ec42ec68f8b..2c2d8320332c42 100644 --- a/Python/bytecodes.c +++ b/Python/bytecodes.c @@ -1438,14 +1438,9 @@ dummy_func( inst(UNPACK_SEQUENCE_LIST, (unused/1, seq -- values[oparg])) { PyObject *seq_o = PyStackRef_AsPyObjectBorrow(seq); DEOPT_IF(!PyList_CheckExact(seq_o)); - #ifdef Py_GIL_DISABLED - PyCriticalSection cs; - PyCriticalSection_Begin(&cs, seq_o); - #endif + LOCK_OBJECT(seq_o); if (PyList_GET_SIZE(seq_o) != oparg) { - #ifdef Py_GIL_DISABLED - PyCriticalSection_End(&cs); - #endif + UNLOCK_OBJECT(seq_o); DEOPT_IF(true); } STAT_INC(UNPACK_SEQUENCE, hit); @@ -1453,9 +1448,7 @@ dummy_func( for (int i = oparg; --i >= 0; ) { *values++ = PyStackRef_FromPyObjectNew(items[i]); } - #ifdef Py_GIL_DISABLED - PyCriticalSection_End(&cs); - #endif + UNLOCK_OBJECT(seq_o); DECREF_INPUTS(); } diff --git a/Python/executor_cases.c.h b/Python/executor_cases.c.h index c91257b06cad11..22edf1afb75919 100644 --- a/Python/executor_cases.c.h +++ b/Python/executor_cases.c.h @@ -1728,18 +1728,9 @@ UOP_STAT_INC(uopcode, miss); JUMP_TO_JUMP_TARGET(); } - #ifdef Py_GIL_DISABLED - PyCriticalSection cs; - _PyFrame_SetStackPointer(frame, stack_pointer); - PyCriticalSection_Begin(&cs, seq_o); - stack_pointer = _PyFrame_GetStackPointer(frame); - #endif + LOCK_OBJECT(seq_o); if (PyList_GET_SIZE(seq_o) != oparg) { - #ifdef Py_GIL_DISABLED - _PyFrame_SetStackPointer(frame, stack_pointer); - PyCriticalSection_End(&cs); - stack_pointer = _PyFrame_GetStackPointer(frame); - #endif + UNLOCK_OBJECT(seq_o); if (true) { UOP_STAT_INC(uopcode, miss); JUMP_TO_JUMP_TARGET(); @@ -1750,11 +1741,7 @@ for (int i = oparg; --i >= 0; ) { *values++ = PyStackRef_FromPyObjectNew(items[i]); } - #ifdef Py_GIL_DISABLED - _PyFrame_SetStackPointer(frame, stack_pointer); - PyCriticalSection_End(&cs); - stack_pointer = _PyFrame_GetStackPointer(frame); - #endif + UNLOCK_OBJECT(seq_o); PyStackRef_CLOSE(seq); stack_pointer += -1 + oparg; assert(WITHIN_STACK_BOUNDS()); diff --git a/Python/generated_cases.c.h b/Python/generated_cases.c.h index 45bcc4242af9d7..ec6d4e409f0e4c 100644 --- a/Python/generated_cases.c.h +++ b/Python/generated_cases.c.h @@ -8040,18 +8040,9 @@ values = &stack_pointer[-1]; PyObject *seq_o = PyStackRef_AsPyObjectBorrow(seq); DEOPT_IF(!PyList_CheckExact(seq_o), UNPACK_SEQUENCE); - #ifdef Py_GIL_DISABLED - PyCriticalSection cs; - _PyFrame_SetStackPointer(frame, stack_pointer); - PyCriticalSection_Begin(&cs, seq_o); - stack_pointer = _PyFrame_GetStackPointer(frame); - #endif + LOCK_OBJECT(seq_o); if (PyList_GET_SIZE(seq_o) != oparg) { - #ifdef Py_GIL_DISABLED - _PyFrame_SetStackPointer(frame, stack_pointer); - PyCriticalSection_End(&cs); - stack_pointer = _PyFrame_GetStackPointer(frame); - #endif + UNLOCK_OBJECT(seq_o); DEOPT_IF(true, UNPACK_SEQUENCE); } STAT_INC(UNPACK_SEQUENCE, hit); @@ -8059,11 +8050,7 @@ for (int i = oparg; --i >= 0; ) { *values++ = PyStackRef_FromPyObjectNew(items[i]); } - #ifdef Py_GIL_DISABLED - _PyFrame_SetStackPointer(frame, stack_pointer); - PyCriticalSection_End(&cs); - stack_pointer = _PyFrame_GetStackPointer(frame); - #endif + UNLOCK_OBJECT(seq_o); PyStackRef_CLOSE(seq); stack_pointer += -1 + oparg; assert(WITHIN_STACK_BOUNDS()); From 9c35049f1e34a9dffbe2b9c1a8f4e13e3e80bb3a Mon Sep 17 00:00:00 2001 From: Donghee Na Date: Mon, 2 Dec 2024 23:47:49 +0900 Subject: [PATCH 2/2] Address code review --- Python/bytecodes.c | 2 +- Python/executor_cases.c.h | 5 ++++- Python/generated_cases.c.h | 2 +- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/Python/bytecodes.c b/Python/bytecodes.c index 2c2d8320332c42..e96674c3502ef1 100644 --- a/Python/bytecodes.c +++ b/Python/bytecodes.c @@ -1438,7 +1438,7 @@ dummy_func( inst(UNPACK_SEQUENCE_LIST, (unused/1, seq -- values[oparg])) { PyObject *seq_o = PyStackRef_AsPyObjectBorrow(seq); DEOPT_IF(!PyList_CheckExact(seq_o)); - LOCK_OBJECT(seq_o); + DEOPT_IF(!LOCK_OBJECT(seq_o)); if (PyList_GET_SIZE(seq_o) != oparg) { UNLOCK_OBJECT(seq_o); DEOPT_IF(true); diff --git a/Python/executor_cases.c.h b/Python/executor_cases.c.h index 22edf1afb75919..580814657608db 100644 --- a/Python/executor_cases.c.h +++ b/Python/executor_cases.c.h @@ -1728,7 +1728,10 @@ UOP_STAT_INC(uopcode, miss); JUMP_TO_JUMP_TARGET(); } - LOCK_OBJECT(seq_o); + if (!LOCK_OBJECT(seq_o)) { + UOP_STAT_INC(uopcode, miss); + JUMP_TO_JUMP_TARGET(); + } if (PyList_GET_SIZE(seq_o) != oparg) { UNLOCK_OBJECT(seq_o); if (true) { diff --git a/Python/generated_cases.c.h b/Python/generated_cases.c.h index ec6d4e409f0e4c..e1f951558de7da 100644 --- a/Python/generated_cases.c.h +++ b/Python/generated_cases.c.h @@ -8040,7 +8040,7 @@ values = &stack_pointer[-1]; PyObject *seq_o = PyStackRef_AsPyObjectBorrow(seq); DEOPT_IF(!PyList_CheckExact(seq_o), UNPACK_SEQUENCE); - LOCK_OBJECT(seq_o); + DEOPT_IF(!LOCK_OBJECT(seq_o), UNPACK_SEQUENCE); if (PyList_GET_SIZE(seq_o) != oparg) { UNLOCK_OBJECT(seq_o); DEOPT_IF(true, UNPACK_SEQUENCE);