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

@@ -1387,6 +1390,10 @@ iterations of the loop.

   .. 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

            Py_DECREF(callable);
            GO_TO_INSTRUCTION(CALL_PY_EXACT_ARGS);
        }

-        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

            SKIP_OVER(INLINE_CACHE_ENTRIES_CALL);
            frame->return_offset = 0;
            DISPATCH_INLINED(new_frame);
        }

-        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, ncoghlan, warsaw, markshannon and iritkatriel 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

brandtbucher commented Jun 16, 2023 •

edited

carljm left a comment

carljm Jun 16, 2023

brandtbucher Jun 16, 2023

carljm Jun 16, 2023

brandtbucher Jun 16, 2023 •

edited

carljm Jun 16, 2023

brandtbucher commented Jun 16, 2023

brandtbucher commented Jun 16, 2023

brandtbucher commented Jun 18, 2023

brandtbucher commented Jun 20, 2023

gvanrossum commented Aug 21, 2023

gvanrossum commented Aug 21, 2023

markshannon commented Aug 21, 2023

gvanrossum commented Aug 25, 2023

brandtbucher commented Aug 25, 2023

brandtbucher commented Aug 25, 2023

gvanrossum commented Aug 25, 2023

brandtbucher commented Aug 25, 2023

		@@ -1387,6 +1390,10 @@ iterations of the loop.

		.. versionadded:: 3.11

		.. versionchanged:: 3.12

GH-105848: Get rid of KW_NAMES #105849

GH-105848: Get rid of KW_NAMES #105849

Conversation

brandtbucher commented Jun 16, 2023 • edited

carljm left a comment

Choose a reason for hiding this comment

carljm Jun 16, 2023

Choose a reason for hiding this comment

brandtbucher Jun 16, 2023

Choose a reason for hiding this comment

carljm Jun 16, 2023

Choose a reason for hiding this comment

brandtbucher Jun 16, 2023 • edited

Choose a reason for hiding this comment

carljm Jun 16, 2023

Choose a reason for hiding this comment

brandtbucher commented Jun 16, 2023

brandtbucher commented Jun 16, 2023

brandtbucher commented Jun 18, 2023

brandtbucher commented Jun 20, 2023

gvanrossum commented Aug 21, 2023

gvanrossum commented Aug 21, 2023

markshannon commented Aug 21, 2023

gvanrossum commented Aug 25, 2023

brandtbucher commented Aug 25, 2023

brandtbucher commented Aug 25, 2023

gvanrossum commented Aug 25, 2023

brandtbucher commented Aug 25, 2023

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

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

brandtbucher commented Jun 16, 2023 •

edited

brandtbucher Jun 16, 2023 •

edited