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

bpo-38310: Predict BUILD_MAP_UNPACK_WITH_CALL -> CALL_FUNCTION_EX. #16467

Merged
merged 2 commits into from
Sep 29, 2019

Conversation

brandtbucher
Copy link
Member

@brandtbucher brandtbucher commented Sep 28, 2019

This patch adds four one new opcode prediction:

BUILD_MAP_UNPACK_WITH_CALL -> CALL_FUNCTION_EX:

  • Emitted whenever more than one map of **kwargs is unpacked into a call.
  • Pair always occurs together.

LOAD_BUILD_CLASS -> LOAD_CONST:

  • Emitted whenever a class is defined without the use of cell vars.
  • In my analysis, occurs ~93% of the time.

LOAD_BUILD_CLASS -> LOAD_CLOSURE:

  • Emitted whenever a class is defined with the use of cell vars.
  • Occurs the other ~7% of the time

LOAD_ASSERTION_ERROR -> RAISE_VARARGS:

  • Emitted whenever the one-argument form of assert is used.
  • Occurs ~91% of the time.

https://bugs.python.org/issue38310

@brandtbucher brandtbucher added the performance Performance or resource usage label Sep 28, 2019
@rhettinger rhettinger self-assigned this Sep 28, 2019
Copy link
Contributor

@rhettinger rhettinger left a comment

Choose a reason for hiding this comment

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

Let's limit this to just "BUILD_MAP_UNPACK_WITH_CALL -> CALL_FUNCTION_EX". Since the has a 100% prediction rate and and because it can occur in performance sensitive code.

The others are low payoff and possibly negative payoff in some code (due to the added cost in the mispredicted state). BUILD_CLASS is almost never part of performance sensitive code (we tend to build a class once and use it many time). Also, asserts are rarely part of performance sensitive code (either we turn them off with -O or hoist them out of loop) and actually failing with a LOAD_ASSERTION_ERROR is literally the exceptional case. Further the prediction rate is highly dependent on the particular code snippet in question.

@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

And if you don't make the requested changes, you will be put in the comfy chair!

@brandtbucher brandtbucher force-pushed the predictions branch 2 times, most recently from 452edaa to ee7c100 Compare September 28, 2019 22:52

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@brandtbucher brandtbucher changed the title bpo-38310: Opcode predictions for asserts, class definitions, and some calls. bpo-38310: Predict BUILD_MAP_UNPACK_WITH_CALL -> CALL_FUNCTION_EX. Sep 28, 2019
@brandtbucher
Copy link
Member Author

brandtbucher commented Sep 28, 2019

Thank @rhettinger for your feedback. I agree with your thought process regarding these, and have removed them.

On the topic of predictability, DUP_TOP_TWO -> BINARY_SUBSCR is a 100% predictable combination as well, emitted by a[b] += c (for any in-place op). I omitted it because it had been discussed already, over three years ago.

I'm wondering if your thoughts have changed regarding this pairing. It comes up almost two hundred times in the standard library, often in deeply-nested loops.

Yes, the pairing is coincidental (not conceptual). But it still seems like a cheap boost to add here.

@rhettinger
Copy link
Contributor

The reasoning about DUP_TOP_TWO -> BINARY_SUBSCR still stands. It would only take a minor change to the compiler to cause significant mispredictions. It is a performance bug waiting to happen. FWIW, the prediction opcodes matter less than they used to because they're turned-off by default on our most popular builds.

@brandtbucher
Copy link
Member Author

Thanks @rhettinger!

@brandtbucher brandtbucher deleted the predictions branch September 29, 2019 05:23
jacobneiltaylor pushed a commit to jacobneiltaylor/cpython that referenced this pull request Dec 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Performance or resource usage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants