-
-
Notifications
You must be signed in to change notification settings - Fork 3k
[mypyc] feat: __mypyc_empty_tuple__
constant
#19654
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
Conversation
In 3.13 and later, we could use |
02cd112
to
fecc89c
Compare
mypyc/lib-rt/init.c
Outdated
PyErr_SetString(PyExc_RuntimeError, "Failed to initialize __mypyc_empty_tuple__"); | ||
return; | ||
} | ||
Py_INCREF(__mypyc_empty_tuple__); |
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.
No need to do an incref, since PyTuple_New
returns a new reference that gets implicitly transferred to __mypyc_empty_tuple__
.
Thanks for your feedbacks, I've updated the PR accordingly. |
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.
Found one more thing.
size: Value = Integer(len(items), c_pyssize_t_rprimitive) | ||
return self.call_c(new_tuple_op, [size] + items, line) | ||
else: | ||
return self.call_c(load_empty_tuple_constant_op, [], line) |
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.
Also use the new primitive in new_tuple_with_length
below if length == 0.
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 not sure how to implement this logic at compile time because in this location, length
is a Value not a known integer
we could do a boolean op to check, but then we're just recreating what already exists in PyTuple_New and making our IR longer to do it
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.
You are correct, please disregard my above comment.
The new function could be used in mypyc/codegen/emit.py
on this line when generating C for a box operation: self.emit_line(f"{declaration}{dest} = PyTuple_New({len(typ.types)});")
. If length is zero, we could use the new primitive?
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.
looks correct. We can also skip the subsequent NULL check. I've implemented this and the tests are running, I may need to update the IR test definitons.
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.
Thanks for the updates, looks good now!
I realized that any time a user has a kwarg-only call expression like
fn(abc=123, ...)
in their compiled code, andfunc
is not a native function, a new empty tuple is created every timeThis is not really necessary, we can just hold the same empty tuple in memory as a constant and pass it around. It's immutable, and that's already what we're already doing, since
tuple() is tuple()
but our current method involves more steps.This should slightly improve the speed of kwarg-only python func calling.