Repository navigation
gh-119477: Analyze through BUILD_TUPLE and BINARY_SUBSCR_TUPLE_INT - #119478
tekknolagi wants to merge 2 commits into
Conversation
Sorry, something went wrong.
Make an symbolic tuple arena so we can allocate arrays of pointers to other symbols.
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.
Sorry, something went wrong.
| 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.
Sorry, something went wrong.
| 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?
Sorry, something went wrong.
| _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.
Sorry, something went wrong.
| 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?
Sorry, something went wrong.
| 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.
Sorry, something went wrong.
| 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.
Sorry, something went wrong.
| } | ||
| } | ||
|
|
||
| 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.
Sorry, something went wrong.
|
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 I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request. |
Sorry, something went wrong.
|
@tekknolagi do you plan to continue working on this? |
Sorry, something went wrong.
|
This PR is stale because it has been open for 30 days with no activity. |
Sorry, something went wrong.
|
This is done, thanks everyone! |
Sorry, something went wrong.
Make an symbolic tuple arena so we can allocate arrays of pointers to
other symbols.
Co-authored-by: @Fidget-Spinner