GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-43693: Turn localspluskinds into an object by gvanrossum · Pull Request #26749 · python/cpython

Conversation

@gvanrossum

@gvanrossum gvanrossum commented Jun 15, 2021 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

This makes memory management for code objects cleaner (no more special casing for ownership of the kinds array).

https://bugs.python.org/issue43693

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
gvanrossum added 6 commits June 15, 2021 15:51
@gvanrossum
Make co_localspluskinds an object instead of a raw pointer
da37894
@gvanrossum
Regenerate frozen files
7564dbe
@gvanrossum
Use PyBytes_AsString instead of PyBytes_AS_STRING
32cc42e 
(But still I get a crash in _freeze_importlib)
@gvanrossum
Fix refcnt bug; tighten checks; regen importlib
9e54613
@gvanrossum
Regenerate more frozen things
e5b413e
@gvanrossum
Merge main (esp. Eric's PR)
00954ec
@gvanrossum
gvanrossum requested a review from ericsnowcurrently June 15, 2021 23:23
@gvanrossum
gvanrossum requested review from markshannon and tiran as code owners June 15, 2021 23:23
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Jun 15, 2021
@bedevere-bot bedevere-bot added the awaiting core review label Jun 15, 2021
@gvanrossum gvanrossum added the skip news label Jun 15, 2021
gvanrossum
gvanrossum commented Jun 15, 2021
View reviewed changes
Comment thread Include/cpython/code.h Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/importlib/_bootstrap_external.py
# Python 3.11a1 3454 (compute cell offsets relative to locals bpo-43693)
# Python 3.11a1 3455 (add MAKE_CELL bpo-43693)
# Python 3.11a1 3456 (interleave cell args bpo-43693)
# Python 3.11a1 3457 (Change localsplus to a bytes object bpo-43693)

@gvanrossum gvanrossum Jun 15, 2021

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I wonder if we're not using up too many magic numbers during pre-alpha...

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ericsnowcurrently ericsnowcurrently Jun 16, 2021

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

what are our options?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@gvanrossum gvanrossum Jun 16, 2021

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

what are our options?

Going back to the first magic number for 3.11a1. Everything is for the same bpo issue it seems.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Objects/codeobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/frameobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/frameobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@gvanrossum
Tweaks I noticed while reviewing this myself
4fba934
@gvanrossum
gvanrossum marked this pull request as draft June 15, 2021 23:36
@gvanrossum
Fix PyCode_NewEmpty()
ebaaeca
@gvanrossum

gvanrossum commented Jun 16, 2021

Copy link
Copy Markdown
Member Author

@ericsnowcurrently I think I have one failing test left, test_ctypes. In particular:

======================================================================
FAIL: test_frozentable (ctypes.test.test_values.PythonValuesTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/guido/cpython/Lib/ctypes/test/test_values.py", line 87, in test_frozentable
    self.assertEqual(items, expected, "PyImport_FrozenModules example "
AssertionError: Lists differ: [('__hello__', 133), ('__phello__', -133), ('__phello__.spam', 133)] != [('__hello__', 128), ('__phello__', -128), ('__phello__.spam', 128)]

First differing element 0:
('__hello__', 133)
('__hello__', 128)

- [('__hello__', 133), ('__phello__', -133), ('__phello__.spam', 133)]
?                 ^^                    ^^                        ^^

+ [('__hello__', 128), ('__phello__', -128), ('__phello__.spam', 128)]
?                 ^^                    ^^                        ^^
 : PyImport_FrozenModules example in Doc/library/ctypes.rst may be out of date

----------------------------------------------------------------------

Have you run into this?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ericsnowcurrently

ericsnowcurrently commented Jun 16, 2021

Copy link
Copy Markdown
Member

Yeah, you have to update the test to match the new memory size
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gvanrossum
Fix test_ctypes (marshalled code size changed)
b09738c
@gvanrossum gvanrossum added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jun 16, 2021
@bedevere-bot

bedevere-bot commented Jun 16, 2021

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @gvanrossum for commit b09738c 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jun 16, 2021
@gvanrossum

gvanrossum commented Jun 16, 2021

Copy link
Copy Markdown
Member Author

I don't see how these changes could make test_ssl fail (only on Mac), and when I run that test myself it passes. I assume it's flakey.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gvanrossum

gvanrossum commented Jun 16, 2021

Copy link
Copy Markdown
Member Author

Happy buildbots, except refleaks on AMD64 Windows8.1 is still churning.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gvanrossum
gvanrossum marked this pull request as ready for review June 16, 2021 14:21
ericsnowcurrently
ericsnowcurrently approved these changes Jun 17, 2021
View reviewed changes

@ericsnowcurrently ericsnowcurrently left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

mostly LGTM

I've left a few comments for some slight adjustments but otherwise everything looks correct. Thanks for taking the time to do this. FWIW, I now see the value of using PyBytesObject instead of a raw array like I had, so thanks for educating me! 😄

I'm approving the PR now, assuming that you'll make the relevant changes (or ask) before merging.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Objects/codeobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/codeobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Python/compile.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Python/compile.c Outdated
Comment on lines +7192 to +7193
static PyObject *
compute_localsplus_info(struct compiler *c, PyObject *names)

@ericsnowcurrently ericsnowcurrently Jun 17, 2021

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

It seems strange to pass one in and return the other. Perhaps create both in this function?

Suggested change
static PyObject *
compute_localsplus_info(struct compiler *c, PyObject *names)
static int
compute_localsplus_info(struct compiler *c, int nlocalsplus, PyObject **pnames, PyObject **pkinds)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ericsnowcurrently ericsnowcurrently Jun 17, 2021

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

...and something like "init_localsplus_info" is probably a better name with the shift in responsibility for this function.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@gvanrossum gvanrossum Jun 18, 2021

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

It seems strange to pass one in and return the other. Perhaps create both in this function?

I find the C idiom for returning multiple values pretty awkward, so I'll just allocate both before passing them into the function.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
ericsnowcurrently reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread Python/compile.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels Jun 17, 2021
gvanrossum added 2 commits June 17, 2021 18:37
@gvanrossum
Tweak things noted in code review
703727c
@gvanrossum
Make symbol static
a64057a
@gvanrossum

gvanrossum commented Jun 18, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

While all the tests pass here, when I try them at home, I get a crash in test_descr, both on Mac and Windows. I need to understand better what's going on there. Maybe I just need to merge a later version of main. But I'm out of time for now.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gvanrossum

gvanrossum commented Jun 18, 2021

Copy link
Copy Markdown
Member Author

Oh wait, I get the same crash on main. Something's wrong!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gvanrossum

gvanrossum commented Jun 19, 2021 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

So, using git bisect I've narrowed it down to GH-26595:

commit e117c0283705943189e6b1aef668a1f68f3f00a4 (HEAD)
Author: Mark Shannon <[email protected]>
Date:   Thu Jun 10 00:46:01 2021

    [bpo-44337](https://bugs.python.org/issue44337): Port LOAD_ATTR to PEP 659 adaptive interpreter (GH-26595)
    
    * Specialize LOAD_ATTR with  LOAD_ATTR_SLOT and LOAD_ATTR_SPLIT_KEYS
    
    * Move dict-common.h to internal/pycore_dict.h
    
    * Add LOAD_ATTR_WITH_HINT specialized opcode.
    
    * Quicken in function if loopy
    
    * Specialize LOAD_ATTR for module attributes.
    
    * Add specialization stats

I can repro this on my Mac using the following sequence:

$ ./python.exe -m test test_unittest test_doctest test_descr
0:00:00 load avg: 2.28 Run tests sequentially
0:00:00 load avg: 2.28 [1/3] test_unittest
0:00:02 load avg: 2.28 [2/3] test_doctest
0:00:02 load avg: 2.17 [3/3] test_descr
Fatal Python error: Segmentation fault

Current thread 0x0000000111041e00 (most recent call first):
  File "<frozen importlib._bootstrap>", line 306 in _module_repr
  File "/Users/guido/cpython/Lib/test/test_descr.py", line 3696 in test_uninitialized_modules
  File "/Users/guido/cpython/Lib/unittest/case.py", line 549 in _callTestMethod
  File "/Users/guido/cpython/Lib/unittest/case.py", line 592 in run
  File "/Users/guido/cpython/Lib/unittest/case.py", line 652 in __call__
  File "/Users/guido/cpython/Lib/unittest/suite.py", line 122 in run
  File "/Users/guido/cpython/Lib/unittest/suite.py", line 84 in __call__
  File "/Users/guido/cpython/Lib/unittest/suite.py", line 122 in run
  File "/Users/guido/cpython/Lib/unittest/suite.py", line 84 in __call__
  File "/Users/guido/cpython/Lib/test/support/testresult.py", line 169 in run
  File "/Users/guido/cpython/Lib/test/support/__init__.py", line 960 in _run_suite
  File "/Users/guido/cpython/Lib/test/support/__init__.py", line 1083 in run_unittest
  File "/Users/guido/cpython/Lib/test/test_descr.py", line 5734 in test_main
  File "/Users/guido/cpython/Lib/test/libregrtest/runtest.py", line 246 in _runtest_inner2
  File "/Users/guido/cpython/Lib/test/libregrtest/runtest.py", line 282 in _runtest_inner
  File "/Users/guido/cpython/Lib/test/libregrtest/runtest.py", line 154 in _runtest
  File "/Users/guido/cpython/Lib/test/libregrtest/runtest.py", line 194 in runtest
  File "/Users/guido/cpython/Lib/test/libregrtest/main.py", line 423 in run_tests_sequential
  File "/Users/guido/cpython/Lib/test/libregrtest/main.py", line 521 in run_tests
  File "/Users/guido/cpython/Lib/test/libregrtest/main.py", line 694 in _main
  File "/Users/guido/cpython/Lib/test/libregrtest/main.py", line 641 in main
  File "/Users/guido/cpython/Lib/test/libregrtest/main.py", line 719 in main
  File "/Users/guido/cpython/Lib/test/__main__.py", line 2 in <module>
  File "/Users/guido/cpython/Lib/runpy.py", line 86 in _run_code
  File "/Users/guido/cpython/Lib/runpy.py", line 196 in _run_module_as_main

Extension modules: _testcapi, xxsubtype (total: 2)
Segmentation fault: 11

The same three tests also cause a crash on Windows, with roughly the same traceback.

@markshannon Can you repro this on your Linux machine?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@markshannon

markshannon commented Jun 21, 2021

Copy link
Copy Markdown
Member

Repro what?
Your PR is green. The buildbots seem OK.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gvanrossum

gvanrossum commented Jun 21, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

@markshannon

Repro what?
Your PR is green. The buildbots seem OK.

I know, but when I try this at home, on both Windows and Mac, I get a crash (details in my previous comment):

$ ./python.exe -m test test_unittest test_doctest test_descr

I've painstakingly bisected this behavior down to your PR GH-26595. If I back up in git to the PR before that, that command passes. Any commit on master after that PR, it also fails (including HEAD).

And this is both on Mac and on Windows, so I can't blame it on environmental issues -- the two environments are just too different.

So I beg you to take this serious. I suspect something odd happens in the LOAD_ATTR cache.

Could it be that the CI tests set PYTHONHASHSEED to a fixed value that happens to avoid the crash?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gvanrossum

gvanrossum commented Jun 21, 2021

Copy link
Copy Markdown
Member Author

@markshannon
To help pin this down further, I bisected at the test level. Here is the minimal file to pass as --matchfile FILE flag to the test runner:

unittest.test.test_discovery.TestDiscovery.test_discovery_failed_discovery
unittest.test.testmock.testpatch.PatchTest.test_patch_wont_create_by_default
test.test_doctest.TestDocTestFinder.test_empty_namespace_package
test.test_doctest.TestDocTestFinder.test_issue35753
test.test_descr.ClassPropertiesAndMethods.test_uninitialized_modules
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gvanrossum
Merge main at 06cda80
0ff4466
@gvanrossum

gvanrossum commented Jun 21, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

There LOAD_ATTR issue is unrelated (discussed offline, we understand it and Mark will fix), so assuming the tests pass after the merge with origin/main I'll land this.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gvanrossum
gvanrossum merged commit 355f5dd into python:main Jun 21, 2021
@bedevere-bot bedevere-bot removed the awaiting merge label Jun 21, 2021
@gvanrossum
gvanrossum deleted the cleaner-kinds branch June 21, 2021 20:53
@markshannon markshannon mentioned this pull request May 2, 2022
Logically merge cell and locals array. They are already contiguous in memory #87859
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ericsnowcurrently ericsnowcurrently ericsnowcurrently approved these changes
@markshannon markshannon Awaiting requested review from markshannon markshannon is a code owner
@tiran tiran Awaiting requested review from tiran

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@gvanrossum @ericsnowcurrently @bedevere-bot @markshannon @the-knights-who-say-ni
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.