GH-105848: Get rid of `KW_NAMES`#105849

brandtbucher · 2023-06-16T06:10:47Z

Instead, optionally push tuple of keyword names before every CALL (using the low bit of the oparg as a flag to indicate their presence).

Issue: Clean up the CALL sequence #105848

📚 Documentation preview 📚: https://cpython-previews--105849.org.readthedocs.build/

carljm

Code LGTM. Definitely happy to see KW_NAMES and its side-channel local variable go away.

It's not clear to me that it wouldn't be simpler to just do the conditional version with oparg flag right away in this PR (you've added a lot of PUSH_NULL that are going to be removed shortly, and I wouldn't want this intermediate state to persist without measuring the code size and perf impact), but since you've already done it this way, seems OK.

carljm · 2023-06-16T17:50:57Z

Doc/library/dis.rst


 .. versionadded:: 3.11

+ .. versionchanged:: 3.12


I doubt this is really a candidate for 3.12 backport?
Suggested change
.. versionchanged:: 3.12
.. versionchanged:: 3.13

carljm · 2023-06-16T17:55:52Z

Python/bytecodes.c


-inst(CALL_PY_EXACT_ARGS, (unused/1, func_version/2, method, callable, args[oparg] --unused)){
+inst(CALL_PY_EXACT_ARGS, (unused/1, func_version/2, method, callable, args[oparg], kwnames--unused)){
 assert(kwnames==NULL);


Is this assert is worth having kwnames instead of unused in the instruction signature? I'm guessing compilers will be smart enough to eliminate the unused load from stack when asserts are not enabled...

You're right, probably not worth keeping around. The new assert can just check the oparg flag instead.

carljm · 2023-06-16T17:56:31Z

Python/bytecodes.c


-inst(CALL_PY_WITH_DEFAULTS, (unused/1, func_version/2, method, callable, args[oparg] --unused)){
+inst(CALL_PY_WITH_DEFAULTS, (unused/1, func_version/2, method, callable, args[oparg], kwnames--unused)){
 assert(kwnames==NULL);


same question as above, and several more places below as well

brandtbucher · 2023-06-16T18:11:09Z

It's not clear to me that it wouldn't be simpler to just do the conditional version with oparg flag right away in this PR (you've added a lot of PUSH_NULL that are going to be removed shortly, and I wouldn't want this intermediate state to persist without measuring the code size and perf impact), but since you've already done it this way, seems OK.

I originally benchmarked this and it was "0% faster", but I re-ran the benchmarks after merging in main and it came back 1% slower. So while it's probably in the noise, I agree it's probably just cleaner to do the oparg flag here since that is our end goal anyways, and it should make the perf impact less iffy.

I'll have some new commits with the flag up later today.

brandtbucher · 2023-06-16T21:13:53Z

Rerunning the benchmarks...

brandtbucher · 2023-06-18T14:46:55Z

Hm... still 1% slower. Maybe because of the new branches and bitwise operations calls have now?

brandtbucher · 2023-06-20T21:17:29Z

Given the disappointing performance results, I'm going to close this. It's probably still worth exploring in the future, but isn't really blocking anything we have in the pipeline right now.

gvanrossum · 2023-08-21T16:21:42Z

Not having this is holding back converting some Tier 1 CALL specializations to Tier 2, because the Tier 2 interpreter doesn't have access to the kwnames variable. Moving that onto the stack seems the obvious solution (as opposed to passing the address of the kwnames variable around, which seems horrible).

gvanrossum · 2023-08-21T16:22:24Z

(Reopening as a reminder.)

markshannon · 2023-08-21T16:33:33Z

Given that we hold a strong reference to the code object, we can safely use a borrowed reference to the kwnames tuple.
Using a borrowed reference saves a lot of branches removing the Py_XDECREF to clean up the kwnames.

gvanrossum · 2023-08-25T18:16:49Z

I have a feeling it would be easier to start over than to attempt to merge main into this PR. @brandtbucher Your thoughts? This is the next frontier in translating remaining CALL specializations to Tier 2.

brandtbucher · 2023-08-25T18:19:25Z

My plan is to start over (I can start on this today). Do we have a preference between extra PUSH_NULL instructions vs stealing a bit from CALL's oparg? I'm slightly leaning towards the former, since it's much simpler and they both seem to be about the same, performance-wise.

brandtbucher · 2023-08-25T18:21:06Z

Re: borrowed references, that seems like something we could bolt onto this after (with a new LOAD_CONST_BORROWED opcode or similar).

gvanrossum · 2023-08-25T18:23:13Z

My plan is to start over (I can start on this today). Do we have a preference between extra PUSH_NULL instructions vs stealing a bit from CALL's oparg? I'm slightly leaning towards the former, since it's much simpler and they both seem to be about the same, performance-wise.

Excellent. I'd vote for extra PUSH_NULLs -- in Tier 2 that'd lead to less work (no need to check a bit in oparg and shift to get arg count).

brandtbucher · 2023-08-25T21:50:02Z

Closing in favor of #108496 (updated version).

brandtbucher added 7 commits June 10, 2023 17:36

Replace KW_NAMES with LOAD_CONST/PUSH_NULL
28adc90

Catch up with main
3aff105

Bump magic number
fc8176e

blurb add
dbda923

Fix up the dis docs for CALL
8c414e5

Catch up with main
abfd56a

make patchcheck
538633f

brandtbucher requested review from brettcannon, ericsnowcurrently, iritkatriel, markshannon, ncoghlan and warsaw as code owners June 16, 2023 06:10

bedevere-bot added the awaiting core review label Jun 16, 2023

bedevere-bot mentioned this pull request Jun 16, 2023
Clean up the CALL sequence #105848
Closed

brandtbucher added 6 commits June 15, 2023 23:30

Fix doctests
f9a9aab

Catch up with main
a1640c7

Use the low bit of CALL for kwnames
8da9d14

Fix blurb
b022837

fixup
408c615

Remove bad blurb
47205b2

carljm reviewed Jun 16, 2023
View reviewed changes

brandtbucher marked this pull request as draft June 16, 2023 18:14

bedevere-bot removed the awaiting core review label Jun 16, 2023

brandtbucher marked this pull request as ready for review June 16, 2023 21:13

bedevere-bot added the awaiting core review label Jun 16, 2023

brettcannon removed their request for review June 16, 2023 22:27

brandtbucher closed this Jun 20, 2023

gvanrossum reopened this Aug 21, 2023

brandtbucher mentioned this pull request Aug 25, 2023
GH-105848: Get rid of KW_NAMES (again) #108496
Closed

brandtbucher closed this Aug 25, 2023

Uh oh!

GH-105848: Get rid of KW_NAMES#105849

GH-105848: Get rid of KW_NAMES#105849

Uh oh!

Conversation

brandtbucher commented Jun 16, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carljm left a comment

Choose a reason for hiding this comment

Uh oh!

carljmJun 16, 2023

Choose a reason for hiding this comment

Uh oh!

brandtbucherJun 16, 2023

Choose a reason for hiding this comment

Uh oh!

carljmJun 16, 2023

Choose a reason for hiding this comment

Uh oh!

brandtbucherJun 16, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carljmJun 16, 2023

Choose a reason for hiding this comment

Uh oh!

brandtbucher commented Jun 16, 2023

Uh oh!

brandtbucher commented Jun 16, 2023

Uh oh!

brandtbucher commented Jun 18, 2023

Uh oh!

brandtbucher commented Jun 20, 2023

Uh oh!

gvanrossum commented Aug 21, 2023

Uh oh!

gvanrossum commented Aug 21, 2023

Uh oh!

markshannon commented Aug 21, 2023

Uh oh!

gvanrossum commented Aug 25, 2023

Uh oh!

brandtbucher commented Aug 25, 2023

Uh oh!

brandtbucher commented Aug 25, 2023

Uh oh!

gvanrossum commented Aug 25, 2023

Uh oh!

brandtbucher commented Aug 25, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

GH-105848: Get rid of `KW_NAMES`#105849

GH-105848: Get rid of `KW_NAMES`#105849

brandtbucher commented Jun 16, 2023•
edited
Loading

brandtbucherJun 16, 2023•
edited
Loading