FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

GH-118093: Handle some polymorphism before requiring progress in tier two by brandtbucher · Pull Request #122843 · python/cpython · GitHub

/ cpython Public

GH-118093: Handle some polymorphism before requiring progress in tier two - #122843

Merged
brandtbucher merged 11 commits into
python:mainfrom
brandtbucher:no-progress-needed
Aug 12, 2024
Merged

GH-118093: Handle some polymorphism before requiring progress in tier two#122843
brandtbucher merged 11 commits into
python:mainfrom
brandtbucher:no-progress-needed

Conversation

brandtbucher commented Aug 8, 2024
edited
Loading

Copy link
Copy Markdown
Member

Despite our best intentions, we currently don't handle polymorphism at all in tier two, since our current forward progress requirement means that we need to deopt the first instruction of every new trace.

This changes our trace stitching for side exits to not require progress until a certain tree depth is reached (currently four). It also changes the optimizer to rejoin with any traces encountered during projection, since this does a better job of keeping us on trace in loops (we can take four side exits per iteration before requiring progress) and keeps the amount of tier two code from exploding.

As a simple microbenchmark, this goes from ~10% slower when run with the JIT enabled to ~25% faster after this change.

class A:
    def f(self):
        return True

class B:
    def f(self):
        return False

def main():
    a, b = A(), B()
    for _ in range(100_000_000):
        a, b = b, a
        a.f()

main()

Overall, the benchmarks are 0.6% faster, including nice improvements on all of our interpreter-heavy benchmarks. The stats show that we're actually executing more traces and a bit (~1%) more tier one code now, but that's expected since side exit chains require up to 4x as much "warming up" as before.

brandtbucher added performance Performance or resource usage interpreter-core (Objects, Python, Grammar, and Parser dirs) labels Aug 8, 2024
brandtbucher self-assigned this Aug 8, 2024
Comment thread Include/internal/pycore_optimizer.h Outdated
_PyBloomFilter bloom;
_PyExecutorLinkListNode links;
PyCodeObject *code; // Weak (NULL if no corresponding ENTER_EXECUTOR).
int chain_depth;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Since this is only 2 bits of information, and valid and linked are only 1 bit each, could we pack them together?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I packed these three into bitfields of a uint16_t.

Comment thread Python/optimizer.c Outdated
_Py_CODEUNIT *instr,
_PyExecutorObject **exec,
int Py_UNUSED(stack_entries))
int Py_UNUSED(stack_entries), bool Py_UNUSED(progress_needed))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Put progress_needed on its own line?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Fixed here and elsewhere.

Comment thread Python/optimizer.c Outdated
_PyStackRef *stack_pointer, _PyExecutorObject **executor_ptr)
_PyStackRef *stack_pointer, _PyExecutorObject **executor_ptr, int chain_depth)
{
// The first instruction in a chain and the MAX_CHAIN_DEPTH'th instruction

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Do you mean "the first instruction in the last executor in a chain"?
I read this as meaning that the first and fourth instructions must make progress.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I replaced both "instruction"s with "executor"s.

Comment thread Python/optimizer.c
goto done;
}
}
if (opcode == ENTER_EXECUTOR) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Why are we are done?
If we are wanting specialized traces, they may overlap.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I've added a comment justifying the decision.

brandtbucher merged commit 9621a7d into python:main Aug 12, 2024
blhsing pushed a commit to blhsing/cpython that referenced this pull request Aug 22, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

interpreter-core (Objects, Python, Grammar, and Parser dirs) performance Performance or resource usage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants


Back | FazBrowse Home | New Git URL