Repository navigation
[mypyc] feat: support RTuple in sequence_from_generator_preallocate_helper - #19931
JukkaL merged 30 commits into
Conversation
Sorry, something went wrong.
…Buidler/mypy into preallocate-helper-rtuple
…Buidler/mypy into preallocate-helper-rtuple
for more information, see https://pre-commit.ci
…Buidler/mypy into preallocate-helper-rtuple
for more information, see https://pre-commit.ci
…Buidler/mypy into preallocate-helper-rtuple
| def gen_condition(self) -> None: | ||
| builder = self.builder | ||
| line = self.line | ||
| # TODO: Don't reload the length each time when iterating an immutable sequence? |
There was a problem hiding this comment.
The PR implementing this TODO was merged
Sorry, something went wrong.
|
|
||
| items = [ | ||
| builder.add( | ||
| LoadLiteral(typ.value, object_rprimitive) |
There was a problem hiding this comment.
not really sure why no LoadLiterals are showing up in these 2 IR tests. am I using the get_proper_types incorrectly?
Sorry, something went wrong.
JukkaL
left a comment
There was a problem hiding this comment.
Have you tried measuring the impact using a microbenchmark?
Sorry, something went wrong.
| r2 = box(tuple[int, int, int], source) | ||
| r3 = PyObject_GetIter(r2) | ||
| r1 = box(tuple[int, int, int], source) | ||
| r2 = PyObject_Size(r1) |
There was a problem hiding this comment.
The length is statically known, and there's also a specialized length op for variable-length tuples (var_object_size). Would it be hard to avoid a generic length op here? (This is optional for this PR.)
Sorry, something went wrong.
There was a problem hiding this comment.
#19929 takes care of this by adding support for RTuples to the get_expr_length which supports not only this case but also RTuple length when doing recursive length checks. This PR focuses solely on the preallocate_helper for ease of review.
Sorry, something went wrong.
There was a problem hiding this comment.
I'm also working on a more optimized handler for this case in #19518 but this PR is okay as an intermediate step which also provides a place to hook in that separate optimization (which also covers other cases unrelated to this PR)
Sorry, something went wrong.
I did not, I just took the IR changes for granted for this one The boxing of the ints is ugly but already takes place in the existing iterator implementation, just internally so wasnt showing on the IR previously And this implementation allows us to skip the allocation of the iterator and all of the exception checking in the ForGenerator gen_condition, plus to preallocate the output tuple/list |
Sorry, something went wrong.
This PR extends sequence_from_generator_preallocate_helper to work with RTuple types. This is ready for review.