GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — GH-116017: Get rid of `_COLD_EXIT` by brandtbucher · Pull Request #120960 · python/cpython

Conversation

@brandtbucher

@brandtbucher brandtbucher commented Jun 24, 2024 •
edited by bedevere-app Bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

The cold exits use quite a bit of memory on JIT builds, and complicate memory allocation improvements (since they are the only immortal, shared-by-all-interpreters, unlinked, un-invalidatable executors). They also introduce a very subtle back-and-forth dance between executors when warming up side exits, and represent a significant proportion of the uops executed.

This:

  • Removes them and puts all of their logic into the _EXIT_TRACE instruction (previously this logic was shared between the exit_to_trace label (only used by _EXIT_TRACE) and _COLD_EXIT...
  • ...and also changes _EXIT_TRACE to use an oparg instead of an exit_index...
  • ...which allows us to simplify the tier two instruction format.

Separately, I added some missing stats to _DYNAMIC_EXIT.

This results in a 34.7% reduction in traces executed, and a 1.6% reduction in uops executed (both corresponding to 3.7 billion fewer _COLD_EXITS). Per platform:

  • aarch64-apple-darwin: 3% faster, 9% less memory
  • aarch64-unknown-linux-gnu: 9% faster, 2% less memory
  • i686-pc-windows-msvc: 3% faster
  • x86_64-pc-windows-msvc: 3% faster
  • x86_64-unknown-linux-gnu: 1% faster, 2% less memory
  • Issue: Compiling tiny traces wastes lots of memory #116017

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
brandtbucher added 2 commits June 24, 2024 08:36
@brandtbucher
Get rid of cold exits
caf4b4b
@brandtbucher
blurb add
fb4e43c
@brandtbucher brandtbucher added performance Performance or resource usage interpreter-core (Objects, Python, Grammar, and Parser dirs) labels Jun 24, 2024
@brandtbucher brandtbucher self-assigned this Jun 24, 2024
@brandtbucher
brandtbucher requested review from ericsnowcurrently and markshannon as code owners June 24, 2024 15:59
@bedevere-app bedevere-app Bot mentioned this pull request Jun 24, 2024
Compiling tiny traces wastes lots of memory #116017
Open
@bedevere-app bedevere-app Bot added the awaiting core review label Jun 24, 2024
@Fidget-Spinner

Fidget-Spinner commented Jun 24, 2024

Copy link
Copy Markdown
Member

Maybe a question for @mdboom instead of you: why are the figures on the time plot different than the figures given in geometric mean? E.g. darwin arm64 says "Geometric mean: 1.00x faster" but the figure shows 1.027x" or roughly 3%?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@brandtbucher

brandtbucher commented Jun 24, 2024

Copy link
Copy Markdown
Member Author

I think the number in the README overview of the results is the "HPT" calculation, which (I believe) weights by "noise". You can see that those particular results were pulled upwards by the startup benchmarks, which have a higher standard deviation than most of the others.

But yeah, @mdboom can probably explain better.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mdboom

mdboom commented Jun 24, 2024

Copy link
Copy Markdown
Contributor

The plot takes the mean across all of the distributions of all of the benchmarks. So it takes into account that the distributions of all of the benchmarks aren't necessarily normally distributed.

The geometric mean value (from pyperf) is the nth root of multiplying all of the means of all the benchmarks together.

Fidget-Spinner reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

markshannon
markshannon reviewed Jun 25, 2024
View reviewed changes

@markshannon markshannon left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

One minor nit, otherwise it looks good.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Python/bytecodes.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

brandtbucher added 2 commits July 1, 2024 11:27
@brandtbucher
Feedback from code review
3bcc395
@brandtbucher
Catch up with main
94c4afb
@brandtbucher
brandtbucher merged commit 33903c5 into python:main Jul 1, 2024
@bedevere-app bedevere-app Bot removed the awaiting core review label Jul 1, 2024
Akasurde pushed a commit to Akasurde/cpython that referenced this pull request Jul 3, 2024
@brandtbucher @Akasurde
pythonGH-116017: Get rid of _COLD_EXITs (pythonGH-120960)
d7dedc3
estyxx pushed a commit to estyxx/cpython that referenced this pull request Jul 17, 2024
@brandtbucher @estyxx
pythonGH-116017: Get rid of _COLD_EXITs (pythonGH-120960)
6fcc55e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@markshannon markshannon markshannon left review comments
@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently ericsnowcurrently is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@brandtbucher brandtbucher

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@brandtbucher @Fidget-Spinner @mdboom @markshannon
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.