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-90997: Improve inline cache performance for MSVC by brandtbucher · Pull Request #96781 · python/cpython

Conversation

@brandtbucher

@brandtbucher brandtbucher commented Sep 12, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

This Compiler Explorer snippet suggests that MSVC doesn't optimize away our cache read/write utilities the way that Clang and GCC do. However, it also shows that replacing our current shifting implementations with memcpy calls does exactly what we want, in a standard-defined way, on all three compilers. I think the memcpy version is a bit nicer, especially since we don't need to maintain two differently-endian versions of the same code.

I'm curious if this moves the benchmarks at all, but I don't have a good Windows pyperformance setup figured out yet. So maybe somebody else could help me out with some numbers on this? (@gvanrossum, I think I remember that you were able to get kinda-stable MSVC numbers a while back?)

  • Issue: Inline bytecode caches #90997

Sorry, something went wrong.

Uh oh!

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

All reactions
brandtbucher added 2 commits September 12, 2022 15:13
@brandtbucher
Use memcpy for cache reads/writes
eaa91ed
@brandtbucher
blurb add
ae8dfea
@brandtbucher brandtbucher added performance Performance or resource usage interpreter-core (Objects, Python, Grammar, and Parser dirs) labels Sep 12, 2022
@brandtbucher brandtbucher self-assigned this Sep 12, 2022
@bedevere-bot bedevere-bot added the awaiting core review label Sep 12, 2022
@brandtbucher
brandtbucher requested a review from markshannon September 12, 2022 22:38
@gvanrossum

gvanrossum commented Sep 12, 2022

Copy link
Copy Markdown
Member

I'm reluctant to spend time getting stable benchmarks for this on Windows, last time it took me an afternoon to run the benchmarks several times with and without the patches. Let's wait until we have a Windows benchmarking machine, please?

brandtbucher 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 Sep 13, 2022
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.

The changes in MSVC output are a clear improvement, and seems to have a positive effect elsewhere.
Even on RISC-V which doesn't support unaligned access, the code is a bit better: https://godbolt.org/z/Ydx5roTa7

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread Include/internal/pycore_code.h Resolved

Uh oh!

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

@brandtbucher
Update comment
5985a4a
@brandtbucher

brandtbucher commented Sep 14, 2022

Copy link
Copy Markdown
Member Author

Looks like even GCC benefits on 64-bit ARM: https://godbolt.org/z/MTj3anj4c

All reactions

Sorry, something went wrong.

Uh oh!

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

@brandtbucher brandtbucher added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Sep 14, 2022
@bedevere-bot

bedevere-bot commented Sep 14, 2022

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @brandtbucher for commit 5985a4a 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

All reactions

Sorry, something went wrong.

Uh oh!

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

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Sep 14, 2022
@lpereira

lpereira commented Sep 14, 2022

Copy link
Copy Markdown
Contributor

LGTM

All reactions

Sorry, something went wrong.

Uh oh!

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

@brandtbucher
brandtbucher merged commit a83fdf2 into python:main Sep 15, 2022
@bedevere-bot bedevere-bot removed the awaiting core review label Sep 15, 2022
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

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.

5 participants

@brandtbucher @gvanrossum @bedevere-bot @lpereira @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.