-
-
Notifications
You must be signed in to change notification settings - Fork 3k
[mypyc] feat: support RTuple in sequence_from_generator_preallocate_helper
#19931
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[mypyc] feat: support RTuple in sequence_from_generator_preallocate_helper
#19931
Conversation
…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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR implementing this TODO was merged
|
||
items = [ | ||
builder.add( | ||
LoadLiteral(typ.value, object_rprimitive) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not really sure why no LoadLiterals are showing up in these 2 IR tests. am I using the get_proper_types incorrectly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you tried measuring the impact using a microbenchmark?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
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 |
This PR extends
sequence_from_generator_preallocate_helper
to work with RTuple types. This is ready for review.