Skip to content

Conversation

@brandtbucher
Copy link
Member

@brandtbucher brandtbucher commented Oct 22, 2019

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 a frozenset 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

@brandtbucher brandtbucher added the performance Performance or resource usage label Oct 22, 2019
@brandtbucher
Copy link
Member Author

@serhiy-storchaka, care to review (since you reviewed the other)?

#endif

if (frozenset) {
PyObject *tmp = newconst;
Copy link
Contributor

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.

Copy link
Member Author

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

@methane
Copy link
Member

methane commented Nov 11, 2019

I think it should be implemented in AST, not in peephole.

@brandtbucher
Copy link
Member Author

brandtbucher commented Nov 11, 2019

@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 co_consts?

@methane
Copy link
Member

methane commented Nov 12, 2019

I don’t think...

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.
For example, tuple creation expression (e.g. (1, 2, 3)) is folded in AST optimizer already.

Tuple folding is remaining in peephole only because some other AST nodes produce BUILD_TUPLE ops.

@brandtbucher
Copy link
Member Author

Ah, I see. It looks like this can actually be done pretty easily in starunpack_helper. I'll have another PR up soon for comparison. Thanks @methane!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting review performance Performance or resource usage

5 participants