◐ Shell clean mode source ↗

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

brandtbucher

Conversation

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/

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just love seeing this! Mostly some docs nits.

Comment on lines +2 to +3

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

		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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Break long line

	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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines 1427 to 1428

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

		# 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'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines +1400 to +1401

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's clearer to just say "excluding self":

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


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines 1427 to 1428

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

		GO_TO_INSTRUCTION(CALL_KW);
		}

		inst(CALL_KW, (callable, self_or_null, args[oparg], kwnames -- res)) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Comment on lines -1395 to +1401

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is definitely easier to follow. :)

Reviewers

ericsnowcurrently ericsnowcurrently left review comments

gvanrossum gvanrossum left review comments

ncoghlan Awaiting requested review from ncoghlan ncoghlan is a code owner

warsaw Awaiting requested review from warsaw warsaw is a code owner

markshannon Awaiting requested review from markshannon markshannon is a code owner

iritkatriel Awaiting requested review from iritkatriel iritkatriel is a code owner

Labels