gh-119477: Analyze through BUILD_TUPLE and BINARY_SUBSCR_TUPLE_INT#119478
gh-119477: Analyze through BUILD_TUPLE and BINARY_SUBSCR_TUPLE_INT#119478tekknolagi wants to merge 2 commits intopython:mainfrom
Conversation
Make an symbolic tuple arena so we can allocate arrays of pointers to other symbols.
tekknolagi
changed the title
tekknolagi
requested review from
Fidget-Spinner,
gvanrossum and
markshannon
as code owners
markshannon
left a comment
markshannon
left a comment
There was a problem hiding this comment.
This increases the size of the stack allocated arena considerably, maybe causing stack overflows.
I've suggested how to address this in the comments.
FYI, We plan to merge BINARY_SUBSCR into BINARY_OP soon.
| int flags; // 0 bits: Top; 2 or more bits: Bottom | ||
| PyTypeObject *typ; // Borrowed reference | ||
| PyObject *const_val; // Owned reference (!) | ||
| int tuple_count; |
There was a problem hiding this comment.
Can you move this after the flags, and it doesn't need 32 bits.
| PyTypeObject *typ; // Borrowed reference | ||
| PyObject *const_val; // Owned reference (!) | ||
| int tuple_count; | ||
| struct _Py_UopsSymbol **tuple_val; |
There was a problem hiding this comment.
Can you use indexes, rather than pointers? We can fit 6 10-bit indexes into a 64 bit int.
We really don't care about tuples with len() > 6 and we are already using too much stack memory for these data structures.
Would it be possible to use a union, and overlap this field with const_val?
| _Py_UopsSymbol arena[TY_ARENA_SIZE]; | ||
| } ty_arena; | ||
|
|
||
| typedef struct tup_arena { |
There was a problem hiding this comment.
This shouldn't be necessary, if we use indexes into the arena, instead of pointers.
| extern _Py_UopsSymbol *_Py_uop_sym_new_type( | ||
| _Py_UOpsContext *ctx, PyTypeObject *typ); | ||
| extern _Py_UopsSymbol *_Py_uop_sym_new_tuple(_Py_UOpsContext *ctx, int count); | ||
| extern _Py_UopsSymbol *_Py_uop_sym_tuple_at(_Py_UopsSymbol *sym, int idx); |
There was a problem hiding this comment.
Can you use get_item, not at, for consistency with the rest of the code base?
Can you add _Py_uop_sym_tuple_set_item as well?
| assert(tup != NULL); | ||
| assert(!ctx->out_of_space); | ||
| for (int i = 0; i < oparg; i++) { | ||
| tup->tuple_val[i] = values[i]; |
There was a problem hiding this comment.
Use sym_tuple_set_item(tup, i, values[i]) for better encapsulation.
| op(_BINARY_SUBSCR_TUPLE_INT, (tuple, sub -- res)) { | ||
| if (sym_has_type(tuple) && sym_matches_type(tuple, &PyTuple_Type) && sym_is_const(sub) ) { | ||
| PyObject* value = sym_get_const(sub); | ||
| Py_ssize_t index = ((PyLongObject*)value)->long_value.ob_digit[0]; |
There was a problem hiding this comment.
There is no guarantee that value is an int.
| } | ||
| } | ||
|
|
||
| op(_BINARY_SUBSCR_TUPLE_INT, (tuple, sub -- res)) { |
There was a problem hiding this comment.
This optimization applies to all BINARY_SUBSCR variants, although it is much more likely to apply here.
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
|
@tekknolagi do you plan to continue working on this? |
|
This PR is stale because it has been open for 30 days with no activity. |
|
This is done, thanks everyone! |
3 participants
Make an symbolic tuple arena so we can allocate arrays of pointers to
other symbols.
Co-authored-by: @Fidget-Spinner