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

GH-91719: Make MSVC generate somewhat faster switch code by gvanrossum · Pull Request #91718 · python/cpython · GitHub

Repository navigation

GH-91719: Make MSVC generate somewhat faster switch code - #91718

Merged
gvanrossum merged 4 commits into
python:mainfrom
gvanrossum:dummy-cases
Apr 21, 2022
Merged

gvanrossum merged 4 commits into
python:mainfrom
gvanrossum:dummy-cases

Conversation

Copy link
Copy Markdown
Member

Apparently a switch on an 8-bit quantity where all cases are
present generates a more efficient jump (doing only one indexed
memory load instead of two).

See faster-cpython/ideas#321 (comment)

Apparently a switch on an 8-bit quantity where all cases are
present generates a more efficient jump (doing only one indexed
memory load instead of two).

See faster-cpython/ideas#321 (comment)
gvanrossum requested a review from markshannon as a code owner April 20, 2022 01:28
gvanrossum changed the title Make MSVC generate somewhat faster switch code GH-91719: Make MSVC generate somewhat faster switch code Apr 20, 2022

Copy link
Copy Markdown
Member

Would it make more sense to redefine opcode to be a uint8_t, rather than casting it?

We should probably make use_tracing an 8 bit unsigned integer as well.

Copy link
Copy Markdown
Member Author

Would it make more sense to redefine opcode to be a uint8_t, rather than casting it?

Yeah, I had considered that, it makes sense. I'll confirm that it has the same effect.

We should probably make use_tracing an 8 bit unsigned integer as well.

I don't see why -- it's not used in a similar switch AFAICT, and it's not cramped for space in its struct. I assume for most other operations the cost of loading an int and loading a byte is effectively the same, since the CPU has to load a whole cache line (32 or 64 bytes) anyway.

Copy link
Copy Markdown
Member Author

I don't believe this needs a news blurb.

Copy link
Copy Markdown
Member

I don't see why -- it's not used in a similar switch AFAICT, and it's not cramped for space in its struct. I assume for most other operations the cost of loading an int and loading a byte is effectively the same, since the CPU has to load a whole cache line (32 or 64 bytes) anyway.

The dispatch sequence includes opcode |= cframe.use_tracing.
If cframe.use_tracing is a 32 bit int, then the compiler needs to add a cast.
If it is the same type as opcode, it does not.

Copy link
Copy Markdown
Member Author

The dispatch sequence includes opcode |= cframe.use_tracing.
If cframe.use_tracing is a 32 bit int, then the compiler needs to add a cast.
If it is the same type as opcode, it does not.

Okay, I'll make that change.

Copy link
Copy Markdown
Member Author

@markshannon, please re-review. I confirmed that the switch still uses a single indirection (goto *(base + offset_table[opcode])). I also found that the opcode |= use_tracing is a single instruction (but maybe it always was one?).

Comment thread Python/ceval.c Outdated
Comment thread Tools/scripts/generate_opcode_h.py Outdated

Copy link
Copy Markdown
Member

Looks good to me

Copy link
Copy Markdown
Member

Oh wow, that's a simple and clever optimization! Great that it helps MSVC to optimize Python on Windows!

Copy link
Copy Markdown
Member

Follow-up fo clean the public API: #91906

gvanrossum deleted the dummy-cases branch August 7, 2022 16:01
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants


Back | FazBrowse Home | New Git URL