GH-105848: Replace `KW_NAMES` + `CALL` with `LOAD_CONST` + `CALL_KW` #109300

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

Misc/NEWS.d/next/Core and Builtins/2023-09-07-20-52-27.gh-issue-105848.p799D1.rst

+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

Doc/library/dis.rst

   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

Doc/library/dis.rst

   * 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

Doc/library/dis.rst

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

Lib/importlib/_bootstrap_external.py

 # 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

Python/bytecodes.c

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

Doc/library/dis.rst

+   ``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

Doc/library/dis.rst


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


Suggested change

* ``self`` (or ``NULL``)

* ``self`` or ``NULL``

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

Doc/library/dis.rst

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

Python/bytecodes.c

+            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

Doc/library/dis.rst

-   :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-app bot added the awaiting review label Sep 12, 2023

bedevere-app bot 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:main Sep 13, 2023

bedevere-app bot 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

		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!

GH-105848: Replace KW_NAMES + CALL with LOAD_CONST + CALL_KW #109300

GH-105848: Replace KW_NAMES + CALL with LOAD_CONST + CALL_KW #109300

Uh oh!

Conversation

brandtbucher commented Sep 12, 2023 • edited by github-actions bot Loading 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

GH-105848: Replace `KW_NAMES` + `CALL` with `LOAD_CONST` + `CALL_KW` #109300

GH-105848: Replace `KW_NAMES` + `CALL` with `LOAD_CONST` + `CALL_KW` #109300

brandtbucher commented Sep 12, 2023 •

edited by github-actions bot

Loading

gvanrossum left a comment •

edited

Loading