Skip to content
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

gh-86620: Optimize constant slice creation #23496

Closed
wants to merge 1 commit into from

Conversation

isidentical
Copy link
Sponsor Member

@isidentical isidentical commented Nov 24, 2020

Doc/whatsnew/3.10.rst Outdated Show resolved Hide resolved
@markshannon
Copy link
Member

Why do this in the AST optimizer, rather than the CFG optimizer?
Can this convert tuples containing slices, as used in numpy, into constants? . E.g. [:, 1]

@isidentical
Copy link
Sponsor Member Author

isidentical commented Nov 25, 2020

Why do this in the AST optimizer, rather than the CFG optimizer?

How is this different than any kind of constant folding that we do in the AST optimizer? The only reason to move that I can think of is, having CFG to cover more cases than the AST optimizer (like the stuff compiler generates independently from the AST node) but I do not think it is a valid argument for slices (I can't think of any code path that the AST optimizer would miss).

@markshannon
Copy link
Member

markshannon commented Nov 25, 2020

That wasn't a "please justify yourself" question, just an "I'm curious" question 🙂
I'm wondering whether we should remove constant folding from the CFG optimizer, if the AST optimizer does it anyway.

It seems that neither optimizer turns (0 if True else 1, None) into a constant (yet).

@markshannon
Copy link
Member

A quick bit of experimentation shows that the CFG optimizer folds default values and annotations, which aren't handled by the AST optimizer.

Copy link
Member

@markshannon markshannon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@isidentical
Copy link
Sponsor Member Author

A quick bit of experimentation shows that the CFG optimizer folds default values and annotations, which aren't handled by the AST optimizer.

How can the CFG optimizer can do extra work on annotations? (Since PEP 563 is enabled by default, they are all strings)? For the default values, I believe this code path should optimize the defaults away, but if there is something wrong with it let's open an issue and discuss.

cpython/Python/ast_opt.c

Lines 620 to 621 in b9127dd

CALL_SEQ(astfold_expr, expr, node_->kw_defaults);
CALL_SEQ(astfold_expr, expr, node_->defaults);

@markshannon
Copy link
Member

Try it for yourself. Comment out fold_tuple_on_constants in compile.c

@isidentical
Copy link
Sponsor Member Author

Try it for yourself. Comment out fold_tuple_on_constants in compile.c

It works fine for defaults?

 $ ./python -m dis
def foo(a = (1,2,3)): pass
  1           0 LOAD_CONST               0 ((1, 2, 3))
              2 BUILD_TUPLE              1
              4 LOAD_CONST               1 (<code object foo at 0x7fde5581e520, file "<stdin>", line 1>)
              6 LOAD_CONST               2 ('foo')
              8 MAKE_FUNCTION            1 (defaults)
             10 STORE_NAME               0 (foo)
             12 LOAD_CONST               3 (None)
             14 RETURN_VALUE

Disassembly of <code object foo at 0x7fde5581e520, file "<stdin>", line 1>:
  1           0 LOAD_CONST               0 (None)
              2 RETURN_VALUE

@markshannon
Copy link
Member

Try running the test suite, and look at the failures. I think it only happens in nested functions with defaults. Not sure why.

@isidentical
Copy link
Sponsor Member Author

Ah, I see. It is not related to the constant folding that I was speaking of (like folding a single default's value a = (1, 2, 3)) but rather the tuple of all default values a=1, b=2 => (1, 2). AFAIK this and some other cases where the compiler generates tuples (via BUILD_TUPLE) but the cases aren't exactly optimizable on the AST is the reason that we do this after the CFG is emitted (BUILD_TUPLE search on Python/compile.c highlights some of these, especially the ones that represents the signatures of function objects).

@github-actions
Copy link

github-actions bot commented Jan 1, 2021

This PR is stale because it has been open for 30 days with no activity.

@github-actions github-actions bot added the stale Stale PR or inactive for long period of time. label Jan 1, 2021
@tiran tiran removed their request for review April 17, 2021 21:15
@github-actions github-actions bot removed the stale Stale PR or inactive for long period of time. label Aug 1, 2022
@iritkatriel
Copy link
Member

@isidentical This has merge conflicts now.

@iritkatriel iritkatriel added pending The issue will be closed if no feedback is provided and removed awaiting merge labels Nov 5, 2022
@arhadthedev arhadthedev changed the title bpo-42454: Optimize constant slice creation gh-23496: Optimize constant slice creation May 5, 2023
@arhadthedev arhadthedev changed the title gh-23496: Optimize constant slice creation gh-86620: Optimize constant slice creation May 5, 2023
@arhadthedev
Copy link
Member

Should this PR and the associated issue be closed as totally abandoned? The conflicts are not resolved for at least six month.

@markshannon
Copy link
Member

Yes.
@isidentical free free to reopen if you want to.

FTR, we plan to add a new instruction LOAD_COMMON_CONSTANT to handle None, True, False, etc.
We could add [:], [::-1] and other common slice constants to that set.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting core review pending The issue will be closed if no feedback is provided performance Performance or resource usage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants