Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

gh-119258: Eliminate Type Guards in Tier 2 Optimizer#119259

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Closed

Conversation

@saulshanabrook
Copy link
Contributor

@saulshanabrooksaulshanabrook commentedMay 20, 2024•
edited by bedevere-appbot
Loading

This PR eliminates some unnecessary calls to_GUARD_TYPE_VERSION in the tier 2 interpreter, closing#119258.

It does it by symbolically tracking the version of the type of each object and eliminating the type guard if the checked version is the same as the tracked one.

It also has to verify that there were no operations that might have escaped since we recorded the type version. So we store the last instruction offset that could lead to an escape (and possibly change the type version), and also store at which instruction offset we stored the version. We can compare these when optimizing, to make sure to only remove the bytecode if we haven't escaped after recording the version.

Work on this PR was done at the PyCon sprints with@dpdani and with help from@Fidget-Spinner and@markshannon

Development

In order to try out this commit, you have to configure it with the experimental jit:

./configure --with-pydebug --enable-experimental-jit=interpreter

Then whenever you change the cases you have to runmake regen-cases and thenmake -j.

Finally, to just run one test case, you can use the--match:

./python.exe -mtest -v test_capi.test_opt --match test_guard_type_version_removed./python.exe -mtest -v test_capi.test_opt --match test_guard_type_version_not_removed

@ghost
Copy link

ghost commentedMay 20, 2024•
edited by ghost
Loading

All commit authors signed the Contributor License Agreement.
CLA signed

@saulshanabrook
Copy link
ContributorAuthor

@brandtbucher can you run the benchmarks on this PR?

brandtbucher reacted with thumbs up emoji

@brandtbucher
Copy link
Member

Just started the job (including stats). Should be 2-3 hours!

saulshanabrook reacted with heart emoji

@brandtbucher
Copy link
Member

brandtbucher commentedMay 21, 2024•
edited
Loading

Results are in!

Performance impact isnegligible with the JIT enabled. However, thestats are more interesting: if you go to the "Optimization (Tier 2) stats" section, you'll see that we're executing 1.7B fewer_GUARD_TYPE_VERSION instructions (a reduction of 25%).

I haven't dug much deeper than that, but the stats also seem to indicate that we're executing around 10% fewer traces (and micro-ops) in general. It isn't clear why this PR would cause that to happen... it could just be noise in the run, or possibly some second-order interaction due to the change.

Either way, I think the takeaway is: this seems to work correctly, but type version checks aren't really a significant source of overhead. I think it may still be worth doing, especially if we can use a similar approach to remove other checks too.

@markshannon, any thoughts here?

@mdboom
Copy link
Contributor

we're executing around 10% fewer traces (and micro-ops) in general. It isn't clear why this PR would cause that to happen...

I did notice that this PR breaks the hexiom and mdp benchmarks (they run on the base, but not with the changes in this PR). This kind of thing is totally expected for a big experimental change like this, but it might provide some clues about how it's not performing as expected. Thelogs just say "benchmark died", which usually means a segfault or something else that wouldn't produce a Python traceback.

@brandtbucher
Copy link
Member

Interesting! We’ll take a look today.

@saulshanabrook
Copy link
ContributorAuthor

@mdboom thank you for flagging this! I was able to reproduce it locally with these two test files.

I notice it also segfaults on the new one#119365 so I will look into why and try to add a test case for it as well to the test suite in the new PR.

@saulshanabrook
Copy link
ContributorAuthor

We seemed to fix this bug:88e2e59

It was very weird and likely a bug in main that was only highlighted with these changes.

mdboom reacted with hooray emoji

@saulshanabrook
Copy link
ContributorAuthor

I am closing this PR since it seems like we want to use the approach in#119365 to use watchers instead, which allows us to remove more guards and also opens up some future improvements that rely on type watching.

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

Reviewers

@Fidget-SpinnerFidget-SpinnerAwaiting requested review from Fidget-SpinnerFidget-Spinner is a code owner

@markshannonmarkshannonAwaiting requested review from markshannonmarkshannon is a code owner

@gvanrossumgvanrossumAwaiting requested review from gvanrossum

@brandtbucherbrandtbucherAwaiting requested review from brandtbucher

@tomasr8tomasr8Awaiting requested review from tomasr8tomasr8 will be requested when the pull request is marked ready for reviewtomasr8 is a code owner

Assignees

No one assigned

Labels

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@saulshanabrook@brandtbucher@mdboom@dpdani
[8]ページ先頭
©2009-2025 Movatter.jp