GH-105848: Replace `KW_NAMES` + `CALL` with `LOAD_CONST` + `CALL_KW` by brandtbucher · Pull Request #109300 · python/cpython

brandtbucher · 2023-09-12T01:01:59Z

Get rid of the magic kwnames side-channel and split out calls with a tuple of keyword names on the stack (CALL_KW) from those with no keyword arguments (the existing CALL family). Stats on an earlier version of this branch suggested that specializing CALL_KW just isn't worth it.

Other cleanup:

There was some incorrect code in mark_stacks that was missed in GH-105848: Simplify the arrangement of CALL's stack #107788 (some docs, too).
Any CALL specialization with the name CALL_NO_KW_* has the NO_KW part dropped, since that should be obvious with the new split.

The new CALL_KW instruction is mostly copied and lightly modified from the existing CALL instruction. They can probably share code using uops, but that's a project for another day.

Issue: Clean up the CALL sequence #105848

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

bedevere-bot · 2023-09-12T01:41:42Z

🤖 New build scheduled with the buildbot fleet by @brandtbucher for commit 53917a5 🤖

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

bedevere-bot · 2023-09-12T01:41:42Z

🤖 New build scheduled with the buildbot fleet by @brandtbucher for commit 53917a5 🤖

If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again.

brandtbucher · 2023-09-12T06:07:51Z

1% faster.

markshannon · 2023-09-12T08:36:12Z

Can you add CALL_KW to the list of specializable instructions that we don't specialize for the stats. https://github.com/python/cpython/blob/main/Python/specialize.c#L135

gvanrossum

I just love seeing this! Mostly some docs nits.

gvanrossum · 2023-09-12T16:05:17Z

+arguments. Also, fix a possible crash when jumping over method calls in a
+debugger.


Is the crash fix inseparable from the new opcode? Is there no issue for it?

It's just bugs in mark_stacks left over from #107788. So same issue number, unrelated to the new opcode though.

gvanrossum · 2023-09-12T16:05:47Z

 correct name, the bytecode pushes the unbound method and ``STACK[-1]``.
 ``STACK[-1]`` will be used as the first argument (``self``) by :opcode:`CALL`
- when calling the unbound method. Otherwise, ``NULL`` and the object returned by
+or :opcode:`CALL_KW` when calling the unbound method. Otherwise, ``NULL`` and the object returned by


Break long line
Suggested change
or :opcode:`CALL_KW` when calling the unbound method. Otherwise, ``NULL`` and the object returned by
or :opcode:`CALL_KW` when calling the unbound method.
Otherwise, ``NULL`` and the object returned by

gvanrossum · 2023-09-12T16:06:45Z

 * The callable
- * The positional arguments
- * The named arguments
+ * ``self`` (or ``NULL``)


Suggested change
* ``self`` (or ``NULL``)
* ``self`` or ``NULL``

gvanrossum · 2023-09-12T16:13:16Z

 ``argc`` is the total of the positional and named arguments, excluding
 ``self`` when a ``NULL`` is not present.


(Same thing about whether "not" should be removed.)
Make explicit that if there are N positional and M keyword arguments, oparg is N+M, and len(tuple) must be == M.

gvanrossum · 2023-09-12T16:23:34Z

 # Whenever MAGIC_NUMBER is changed, the ranges in the magic_values array
 # in PC/launcher.c must also be updated.

-MAGIC_NUMBER = (3561).to_bytes(2, 'little') + b'\r\n'


This is already a merge conflict. :-) Might need to manually merge #109269.

gvanrossum · 2023-09-12T16:48:14Z

 // or
- // [method, self, arg1, arg2, ...]
- // (Some args may be keywords, see KW_NAMES, which sets 'kwnames'.)
+ // [callable, self, arg1, arg2, ...]


Maybe add (here too) that oparg counts arg1 etc., but not self? (It has to be this way, of course, but still bears repeating IMO.)

gvanrossum · 2023-09-12T16:55:37Z

+ ``argc`` is the total of the positional arguments, excluding
+ ``self`` when a ``NULL`` is not present.


I think it's clearer to just say "excluding self":
Suggested change
``argc`` is the total of the positional arguments, excluding
``self`` when a ``NULL`` is not present.
``argc`` is the total of the positional arguments, excluding ``self``.

gvanrossum · 2023-09-12T16:56:11Z


 * The callable
- * ``self``
+ * ``self`` (or ``NULL``)


Suggested change
* ``self`` (or ``NULL``)
* ``self`` or ``NULL``

gvanrossum · 2023-09-12T16:56:38Z

 ``argc`` is the total of the positional and named arguments, excluding
 ``self`` when a ``NULL`` is not present.


Suggested change
``argc`` is the total of the positional and named arguments, excluding
``self`` when a ``NULL`` is not present.
``argc`` is the total of the positional and named arguments, excluding ``self``.

gvanrossum · 2023-09-12T17:39:12Z

+ GO_TO_INSTRUCTION(CALL_KW);
+ }
+
+ inst(CALL_KW, (callable, self_or_null, args[oparg], kwnames -- res)) {


Maybe this deserves a comment (like CALL) about the stack contents on entry and exit? Or if not, maybe CALL doesn't need it either? (Because it should be clear from the DSL.)

Yeah, I'd rather just remove it from both. It's not that complex anymore.

brandtbucher · 2023-09-13T00:17:23Z

Ignoring the buildbot failures since they're all either failing on main or on other PRs also. Not a very informative run... :(

brandtbucher · 2023-09-13T00:22:14Z

Can you add CALL_KW to the list of specializable instructions that we don't specialize for the stats. https://github.com/python/cpython/blob/main/Python/specialize.c#L135

That's only useful if we're recording failure kinds, right? We're not in this case, so I don't think it would actually add anything other than an empty section.

ericsnowcurrently · 2023-09-13T15:31:53Z

- :opcode:`KW_NAMES`, if any.
- On the stack are (in ascending order), either:
+ Calls a callable object with the number of arguments specified by ``argc``.
+ On the stack are (in ascending order):

- * NULL
 * The callable
- * The positional arguments
- * The named arguments
-
- or:
-
- * The callable
- * ``self``
+ * ``self`` or ``NULL``
 * The remaining positional arguments
- * The named arguments

- ``argc`` is the total of the positional and named arguments, excluding
- ``self`` when a ``NULL`` is not present.
+ ``argc`` is the total of the positional arguments, excluding ``self``.


That is definitely easier to follow. :)

brandtbucher added 14 commits August 11, 2023 16:53

Split things up (sorta)
5e43544

Fix all specialization
b7820ee

Catch up with main
0ad7112

Catch up with main
72d22a4

Quick hack to fix stats
9c8ccf6

Catch up with main
4c247af

Don't specialize CALL_KW
367e7db

More cleanup
b0e33b6

Catch up with main
6e6a3b4

Catch up with main
135a499

CALL_NO_KW -> CALL
6cd0318

blurb add
53917a5

Feedback from code review
21b7fef

Catch up with main
79f93cd

brandtbucher added the interpreter-core (Objects, Python, Grammar, and Parser dirs) label Sep 12, 2023

brandtbucher requested a review from gvanrossum September 12, 2023 01:01

brandtbucher self-assigned this Sep 12, 2023

brandtbucher requested review from brettcannon, ericsnowcurrently, iritkatriel, markshannon, ncoghlan and warsaw as code owners September 12, 2023 01:01

bedevere-appBot added the awaiting review label Sep 12, 2023

bedevere-appBot mentioned this pull request Sep 12, 2023
Clean up the CALL sequence #105848
Closed

brandtbucher added 🔨 test-with-buildbots Test PR w/ buildbots; report in status section 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section labels Sep 12, 2023

bedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Sep 12, 2023

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Sep 12, 2023

brettcannon removed their request for review September 12, 2023 17:25

gvanrossum reviewed Sep 12, 2023
View reviewed changes

ericsnowcurrently reviewed Sep 13, 2023
View reviewed changes

brandtbucher merged commit 22e65ee into python:mainSep 13, 2023

bedevere-appBot removed the awaiting review label Sep 13, 2023

markshannon mentioned this pull request Oct 24, 2023
Remove eval_breaker and kwnames local variables from interpreter definition. #104580
Closed

coderabbitaiBot mentioned this pull request Apr 3, 2026
#49 Enhance bytecode decompiler with slices and tuples sahebbiswas/pycrefine#50
Merged

coderabbitaiBot mentioned this pull request Aug 1, 2026
Proof: вынести общую BUILD-границу Labpics-Team/lab-colors#508
Closed

		arguments. Also, fix a possible crash when jumping over method calls in a
		debugger.

	or :opcode:`CALL_KW` when calling the unbound method. Otherwise, ``NULL`` and the object returned by
	or :opcode:`CALL_KW` when calling the unbound method.
	Otherwise, ``NULL`` and the object returned by

		``argc`` is the total of the positional and named arguments, excluding
		``self`` when a ``NULL`` is not present.

Uh oh!

Uh oh!

Conversation

brandtbucher commented Sep 12, 2023• edited by github-actions BotLoading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bedevere-bot commented Sep 12, 2023

Uh oh!

bedevere-bot commented Sep 12, 2023

Uh oh!

brandtbucher commented Sep 12, 2023

Uh oh!

markshannon commented Sep 12, 2023

Uh oh!

gvanrossum left a comment • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brandtbucher commented Sep 13, 2023

Uh oh!

brandtbucher commented Sep 13, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

brandtbucher commented Sep 12, 2023•
edited by github-actions Bot
Loading

gvanrossum left a comment •
edited
Loading