-
-
Notifications
You must be signed in to change notification settings - Fork 31.8k
bpo-38328: Speed up the creation time of constant set literals. #16878
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
@serhiy-storchaka, care to review (since you reviewed the other)? |
@@ -161,6 +162,15 @@ fold_tuple_on_constants(_Py_CODEUNIT *codestr, Py_ssize_t codelen, | |||
} | |||
#endif | |||
|
|||
if (frozenset) { | |||
PyObject *tmp = newconst; |
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 pretty sure Py_SETREF()
should be used here instead of this temporary variable.
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.
Is Py_SETREF(newconst, PyFrozenSet_New(newconst))
really clearer and safer? I can add it if you think so, but I personally don’t...
I think it should be implemented in AST, not in peephole. |
@methane I don’t think that there’s any precedent for adding nodes in the AST optimizer; just changing or removing existing ones. This change (and the other one for lists, which has already been approved by @serhiy-storchaka) would essentially require making a new constant, nesting that inside of a new star unpacking node, and nesting that inside of the final container literal node. The peephole optimizer already has the machinery for folding tuples, which can be trivially modified to work for lists and sets in this manner. Is there any reason you think it should be moved, other than the possibility of leaving unused constants in |
It may be done in the compiler instead of AST-optimizer. Even though some code are leaved in peephole, we had moved most optimizer to AST. Tuple folding is remaining in peephole only because some other AST nodes produce BUILD_TUPLE ops. |
Ah, I see. It looks like this can actually be done pretty easily in |
This is basically the same patch as #16498, except for sets. It's nearly identical, except that it adds a new argument to
fold_tuple_on_constants
to create afrozenset
instead.This one can probably be merged first, since the other PR is waiting for a list overallocation issue to be resolved.
https://bugs.python.org/issue38328