gh-126072: do not add `None` to `co_consts` if no docstring by xuantengh · Pull Request #126101 · python/cpython

xuantengh · 2024-10-29T02:27:07Z

Hi @markshannon this is my attempt to implement #126072. Initially the value of CO_HAS_DOCSTRING is set as 0x0040, but I found it has been used by the CO_NOFREE in inspect.

cpython/Lib/inspect.py

Lines 409 to 411 in 85799f1

    
                   co_flags            bitmap: 1=optimized | 2=newlocals | 4=*arg | 8=**arg 
        
                                       | 16=nested | 32=generator | 64=nofree | 128=coroutine 
        
                                       | 256=iterable_coroutine | 512=async_generator

~~And I've tested it with the following code:~~ See the test cases in changed test files

I've also updated the related documentation, but maybe there is something I missed. Please help me improve this PR, thank you!

Issue: The method for storing docstrings in code objects is awkward and prevents optimizations. #126072

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

bedevere-app · 2024-10-29T02:27:12Z

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

JelleZijlstra

Please add some tests, probably to test_code.py.

You'll also probably need to change the __doc__ descriptor on function objects so it continues to work correctly.

Python/codegen.c

Objects/funcobject.c

Python/codegen.c

xuantengh · 2024-10-29T08:45:09Z

Maybe the optimization pass should be updated as the hypothesis may not hold:

cpython/Python/flowgraph.c

Lines 2117 to 2119 in 9b14083

    
           // The first constant may be docstring; keep it always. 
        
           index_map[0] = 0;

The generated bytecodes for the same function are different before and after this PR.

markshannon · 2024-10-29T10:06:16Z

FYI, we are in the process of removing small ints from the co_consts tuple: #125972

Can you change the ints in your tests to strings. Doing so will also help test that non-docstring strings do not get interpreted as docstrings.

markshannon · 2024-10-29T10:08:03Z

Maybe the optimization pass should be updated as the hypothesis may not hold:

cpython/Python/flowgraph.c

Lines 2117 to 2119 in 9b14083

// The first constant may be docstring; keep it always.

index_map[0] = 0;

The generated bytecodes for the same function are different before and after this PR.

There are a few improvements we can make to the bytecode compiler once this PR is done.
It is probably best to do those in other PRs, once this is working.

xuantengh · 2024-10-29T10:41:24Z

It is probably best to do those in other PRs, once this is working.

Yeah, what I concern is whether the constant removal optimization will lead to a unexpected results when the first element in co_consts is no longer None if there is no docstring.

xuantengh · 2024-10-29T11:44:22Z

Currently, as the const removal optimization unconditionally keeps the first element in consts, None will present in co_consts even if co has no docstring if there is only one const (i.e., None itself).

Lib/test/test_compiler_assemble.py

Lib/test/test_inspect/test_inspect.py

willingc

Looks good. One small edit for the language reference.

Doc/reference/datamodel.rst

markshannon

Thanks for doing this.

This was one of those little things that's been bothering me for a while.
Glad to see it fixed

Python/codegen.c

xuantengh · 2024-10-30T04:21:04Z

Update doc inspect.rst as well.

willingc

One small comment on inspect.rst.

Doc/library/inspect.rst

markshannon · 2024-10-30T09:00:14Z

The UI shows 2 jobs still in progress. But they have completed successfully 🤷‍♂️

xuantengh · 2024-10-30T10:10:41Z

Thanks for your review, it's my first PR merged into CPython! And I'm also interested in the following related optimizations.

iritkatriel · 2024-10-30T10:52:04Z

Thanks for your review, it's my first PR merged into CPython! And I'm also interested in the following related optimizations.

Congratulations! in your next PR, add your name to Misc/ACKS (we should have done it here, but too late now).

…tring (pythonGH-126101)

xuantengh and others added 9 commits October 28, 2024 21:33

Merge branch 'python:main' into main

b6d9943

Merge branch 'python:main' into main

92d4470

Merge branch 'python:main' into main

b6aee70

Merge branch 'python:main' into main

7fbd458

Merge branch 'python:main' into main

f6f52bc

Merge branch 'python:main' into main

70a1313

add CO_HAS_DOCSTRING flag

257d58e

do not add None to co_consts if no docstring

b6edafd

update doc

7ceaaee

xuantengh requested review from JelleZijlstra, carljm, iritkatriel, markshannon and willingc as code owners October 29, 2024 02:27

bedevere-app bot added the awaiting review label Oct 29, 2024

bedevere-app bot mentioned this pull request Oct 29, 2024

The method for storing docstrings in code objects is awkward and prevents optimizations. #126072

Closed

add news

1674f83

JelleZijlstra reviewed Oct 29, 2024

View reviewed changes

Python/codegen.c Outdated Show resolved Hide resolved

xuantengh force-pushed the main branch from 1b0ba7b to 49d3254 Compare October 29, 2024 06:18

iritkatriel reviewed Oct 29, 2024

View reviewed changes

Objects/funcobject.c Outdated Show resolved Hide resolved

Python/codegen.c Show resolved Hide resolved

xuantengh force-pushed the main branch from bbe1eae to 8d9c6b5 Compare October 29, 2024 11:55

markshannon reviewed Oct 29, 2024

View reviewed changes

Lib/test/test_compiler_assemble.py Outdated Show resolved Hide resolved

Lib/test/test_inspect/test_inspect.py Outdated Show resolved Hide resolved

willingc approved these changes Oct 29, 2024

View reviewed changes

Doc/reference/datamodel.rst Outdated Show resolved Hide resolved

bedevere-app bot removed the awaiting review label Oct 29, 2024

bedevere-app bot added the awaiting merge label Oct 29, 2024

markshannon approved these changes Oct 29, 2024

View reviewed changes

iritkatriel reviewed Oct 29, 2024

View reviewed changes

Python/codegen.c Outdated Show resolved Hide resolved

xuantengh and others added 13 commits October 30, 2024 12:08

Merge branch 'main' into main

52334c4

calculate ste_has_docstring in symtable.c

099a765

update __doc__ for function object

436b2cf

fix test inspect

f92f180

fix potential bugs

63a40e0

fix async func

b34076a

fix tests

be4cffe

add doc string co tests

0450f96

rebase main to resolve conflicts

4fb24e6

fix test

96085e1

fix more tests

7b37e1f

remove issue number comments

67f893e

take advises

6ebadd6

xuantengh force-pushed the main branch from 5ff3450 to 6ebadd6 Compare October 30, 2024 04:13

update inspect doc

0edf4fc

willingc approved these changes Oct 30, 2024

View reviewed changes

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

xuantengh added 2 commits October 30, 2024 14:34

update doc

4879a5d

update inspect.py comment

49944b6

markshannon merged commit 35df4eb into python:main Oct 30, 2024

bedevere-app bot removed the awaiting merge label Oct 30, 2024

xuantengh mentioned this pull request Oct 31, 2024

gh-126072: Set docstring attribute for module and class #126231

Merged

picnixz pushed a commit to picnixz/cpython that referenced this pull request Dec 8, 2024

pythongh-126072: do not add None to co_consts if there is no docs…

6671047

…tring (pythonGH-126101)

ebonnal pushed a commit to ebonnal/cpython that referenced this pull request Jan 12, 2025

pythongh-126072: do not add None to co_consts if there is no docs…

dcf0b8a

…tring (pythonGH-126101)

	co_flags bitmap: 1=optimized \| 2=newlocals \| 4=arg \| 8=*arg
	\| 16=nested \| 32=generator \| 64=nofree \| 128=coroutine
	\| 256=iterable_coroutine \| 512=async_generator

Uh oh!

Conversation

xuantengh commented Oct 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bedevere-app bot commented Oct 29, 2024

Uh oh!

JelleZijlstra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

xuantengh commented Oct 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

markshannon commented Oct 29, 2024

Uh oh!

markshannon commented Oct 29, 2024

Uh oh!

xuantengh commented Oct 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xuantengh commented Oct 29, 2024

Uh oh!

Uh oh!

Uh oh!

willingc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

markshannon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

xuantengh commented Oct 30, 2024

Uh oh!

willingc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

markshannon commented Oct 30, 2024

Uh oh!

xuantengh commented Oct 30, 2024

Uh oh!

iritkatriel commented Oct 30, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

xuantengh commented Oct 29, 2024 •

edited

Loading

xuantengh commented Oct 29, 2024 •

edited

Loading

xuantengh commented Oct 29, 2024 •

edited

Loading