gh-107457: update dis documentation with changes in 3.12 #108900

MatthieuDartiailh · 2023-09-05T08:09:42Z

This PR addresses the following points of #107457 which should be backported to 3.12

document MIN_INSTRUMENTED_OPCODE which is needed to determine what are the "normal" opcodes
clarify the LOAD_SUPER_ATTR stack manipulation description
fix the description of CALL_INSTRINSIC_2 stack manipulation
document END_SEND
explain how the presence of CACHE instructions impact the computation of jump instruction argument

I did not address the following points:

the description of COMPARE_OP which was corrected in 3.13. However 3.13 does not detail the meaning of the cache: should it be detailed
POP_JUMP_IF_NOT_NONE and POP_JUMP_IF_NONE were not described as actual instructions but this appear to have been fixed at least in 3.13 if it is not in 3.12 it should be backported

I plan to address the What's new issues in a follow up PR

Issue: Bytecode documentation issues for Python 3.12 #107457

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

carljm

The LOAD_SUPER_ATTR changes look OK to me, modulo suggested edits.

Doc/library/dis.rst

Co-authored-by: Carl Meyer <[email protected]>

carljm

The LOAD_SUPER_ATTR changes look good to me now. The other changes look reasonable, but I'd rather get @markshannon or @iritkatriel to confirm the accuracy of the new paragraph on 3.12 jump changes.

requested changes were made

Doc/library/dis.rst

iritkatriel · 2023-09-25T18:14:23Z

Doc/library/dis.rst

@@ -42,6 +42,16 @@ interpreter.
      bytecode to specialize it for different runtime conditions. The
      adaptive bytecode can be shown by passing ``adaptive=True``.

+   .. versionchanged:: 3.12
+      For instruction leading to a jump, a jump by 0 instruction should always


What is an "instruction leading to a jump"?

I agree it is a bit convoluted. I am happy to use a different phrasing. Initially I went for this because of opcode like FOR_ITER that do not jump often.

I don't understand what you mean, so I can't offer how to reword it.

Would the following re-wording read better ?

When the offset of a jump instruction is 0, the jump should always lead to the instruction directly following the jump instruction. Starting with 3.12, some jump instructions can be followed by a :opcode:CACHE instruction. When the argument of such an instruction is 0, it will jump to the first non-CACHE instruction following the jump instruction.

As a consequence, the presence of the :opcode:CACHE instructions is transparent for forward jumps but need to be taken into account when reasoning about backward jumps.

I think I see what you mean now.

Can we just say that "the arg of a jump is the offset from the instruction that appears immediately after the JUMP instruction's CACHE entries" ?

Can we agree on the following ? I would prefer to insist on the forward backward asymmetry

The argument of a jump is the offset of the target relative to the instruction that appears immediately after the jump instruction's CACHE entries.

As a consequence, the presence of the CACHE instructions is transparent for forward jumps but need to be taken into account when reasoning about backward jumps.

Sure. I think "need --> needs" though.

Doc/library/dis.rst

MatthieuDartiailh · 2023-10-17T10:39:23Z

Can I do anything to help merge this work ?

iritkatriel · 2023-10-17T10:55:31Z

There was this question: https://github.com/python/cpython/pull/108900/files#r1336237016

iritkatriel · 2023-10-17T10:56:38Z

Doc/library/dis.rst

-   arguments and sets ``STACK[-1]`` to the result. Used to implement functionality that is
-   necessary but not performance critical.
+   Calls an intrinsic function with two arguments. Used to implement functionality
+   that is necessary but not performance critical::


Suggested change

that is necessary but not performance critical::

that is not performance critical::

I can see you copied this from the 1-arg opcode. But I think it's redundant in both cases.

I will update it

miss-islington-app · 2023-10-17T12:59:39Z

Thanks @MatthieuDartiailh for the PR, and @iritkatriel for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

…nGH-108900) (cherry picked from commit 198aa67) Co-authored-by: Matthieu Dartiailh <[email protected]>

bedevere-app · 2023-10-17T12:59:54Z

GH-110985 is a backport of this pull request to the 3.12 branch.

…08900) (#110985) gh-107457: update dis documentation with changes in 3.12 (GH-108900) (cherry picked from commit 198aa67) Co-authored-by: Matthieu Dartiailh <[email protected]>

…n#108900)

MatthieuDartiailh added 5 commits September 4, 2023 16:16

doc: describe how CACHE opcodes are taken into account in jumps

34250f3

doc: document MIN_INSTRUMENTED_OPCODE in dis

e7d9021

doc: document END_SEND and fix SEND documentation

0586fec

Doc: clarify CALL_INSTRINSIC_2 stack effect

7617456

doc: clarify the stack manipulation of LOAD_SUPER_ATTR

6ad38e3

bedevere-bot added awaiting review docs Documentation in the Doc dir skip news labels Sep 5, 2023

bedevere-bot mentioned this pull request Sep 5, 2023

Bytecode documentation issues for Python 3.12 #107457

Closed

20 tasks

AA-Turner added the needs backport to 3.12 only security fixes label Sep 6, 2023

AA-Turner requested review from iritkatriel, carljm and gvanrossum September 25, 2023 06:53

AA-Turner mentioned this pull request Sep 25, 2023

GH-109190: Copyedit 3.12 What's New: Bytecode #109821

Merged

carljm previously requested changes Sep 25, 2023

View reviewed changes

Doc/library/dis.rst Outdated Show resolved Hide resolved

Doc/library/dis.rst Outdated Show resolved Hide resolved

Doc/library/dis.rst Outdated Show resolved Hide resolved

Doc/library/dis.rst Outdated Show resolved Hide resolved

bedevere-app bot added awaiting core review and removed awaiting review labels Sep 25, 2023

Apply suggestions from code review

3644200

Co-authored-by: Carl Meyer <[email protected]>

carljm reviewed Sep 25, 2023

View reviewed changes

bedevere-app bot added awaiting review and removed awaiting core review labels Sep 25, 2023

iritkatriel reviewed Sep 25, 2023

View reviewed changes

brandtbucher reviewed Sep 25, 2023

View reviewed changes

Doc/library/dis.rst Outdated Show resolved Hide resolved

Update dis.rst

0a1dee2

iritkatriel reviewed Oct 17, 2023

View reviewed changes

docs: address review comments

5a93f49

iritkatriel approved these changes Oct 17, 2023

View reviewed changes

bedevere-app bot added awaiting merge and removed awaiting review labels Oct 17, 2023

Merge branch 'main' into dis-docs

4291f95

iritkatriel changed the title ~~gh-107457: Improve dis documentation~~ gh-107457: update dis documentation with some of the changes in 3.12 Oct 17, 2023

iritkatriel changed the title ~~gh-107457: update dis documentation with some of the changes in 3.12~~ gh-107457: update dis documentation with changes in 3.12 Oct 17, 2023

iritkatriel enabled auto-merge (squash) October 17, 2023 12:50

iritkatriel merged commit 198aa67 into python:main Oct 17, 2023

bedevere-app bot removed the awaiting merge label Oct 17, 2023

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 17, 2023

pythongh-107457: update dis documentation with changes in 3.12 (pytho…

02c489d

…nGH-108900) (cherry picked from commit 198aa67) Co-authored-by: Matthieu Dartiailh <[email protected]>

bedevere-app bot removed the needs backport to 3.12 only security fixes label Oct 17, 2023

MatthieuDartiailh deleted the dis-docs branch October 17, 2023 15:04

aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024

pythongh-107457: update dis documentation with changes in 3.12 (pytho…

b5a0c7d

…n#108900)

Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024

pythongh-107457: update dis documentation with changes in 3.12 (pytho…

a132243

…n#108900)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-107457: update dis documentation with changes in 3.12 #108900

gh-107457: update dis documentation with changes in 3.12 #108900

MatthieuDartiailh commented Sep 5, 2023 •

edited by github-actions bot

Loading

carljm left a comment

carljm left a comment

iritkatriel Sep 25, 2023

MatthieuDartiailh Sep 28, 2023

iritkatriel Sep 28, 2023

MatthieuDartiailh Oct 17, 2023

iritkatriel Oct 17, 2023

MatthieuDartiailh Oct 17, 2023

iritkatriel Oct 17, 2023

MatthieuDartiailh Oct 17, 2023

MatthieuDartiailh commented Oct 17, 2023

iritkatriel commented Oct 17, 2023

iritkatriel Oct 17, 2023

iritkatriel Oct 17, 2023

MatthieuDartiailh Oct 17, 2023

miss-islington-app bot commented Oct 17, 2023

bedevere-app bot commented Oct 17, 2023

	that is necessary but not performance critical::
	that is not performance critical::

gh-107457: update dis documentation with changes in 3.12 #108900

gh-107457: update dis documentation with changes in 3.12 #108900

Conversation

MatthieuDartiailh commented Sep 5, 2023 • edited by github-actions bot Loading

carljm left a comment

Choose a reason for hiding this comment

carljm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MatthieuDartiailh commented Oct 17, 2023

iritkatriel commented Oct 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miss-islington-app bot commented Oct 17, 2023

bedevere-app bot commented Oct 17, 2023

MatthieuDartiailh commented Sep 5, 2023 •

edited by github-actions bot

Loading