Skip to content

Conversation

BobTheBuidler
Copy link
Contributor

@BobTheBuidler BobTheBuidler commented Aug 14, 2025

I realized that any time a user has a kwarg-only call expression like fn(abc=123, ...) in their compiled code, and func is not a native function, a new empty tuple is created every time

This 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.

@BobTheBuidler BobTheBuidler marked this pull request as draft August 14, 2025 06:24
@JukkaL
Copy link
Collaborator

JukkaL commented Aug 14, 2025

In 3.13 and later, we could use Py_GetConstant(Py_CONSTANT_EMPTY_TUPLE);. However, having a static variable with the value would be faster, as we'd save at least a function call, and it will work on older Python versions as well, so it's reasonable to cache it in mypyc.

@BobTheBuidler BobTheBuidler force-pushed the empty-tuple-constant branch 2 times, most recently from 02cd112 to fecc89c Compare August 25, 2025 13:55
@BobTheBuidler BobTheBuidler marked this pull request as ready for review August 25, 2025 14:09
PyErr_SetString(PyExc_RuntimeError, "Failed to initialize __mypyc_empty_tuple__");
return;
}
Py_INCREF(__mypyc_empty_tuple__);
Copy link
Collaborator

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__.

@BobTheBuidler
Copy link
Contributor Author

Thanks for your feedbacks, I've updated the PR accordingly.

@BobTheBuidler BobTheBuidler requested a review from JukkaL August 26, 2025 19:31
Copy link
Collaborator

@JukkaL JukkaL left a 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)
Copy link
Collaborator

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.

Copy link
Contributor Author

@BobTheBuidler BobTheBuidler Aug 27, 2025

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

Copy link
Collaborator

@JukkaL JukkaL Aug 27, 2025

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?

Copy link
Contributor Author

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.

Copy link
Collaborator

@JukkaL JukkaL left a 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!

@JukkaL JukkaL merged commit e0ce3e3 into python:master Aug 27, 2025
13 checks passed
@BobTheBuidler BobTheBuidler deleted the empty-tuple-constant branch August 27, 2025 17:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants