bpo-43693: Turn localspluskinds into an object #26749

gvanrossum · 2021-06-15T23:23:42Z

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

https://bugs.python.org/issue43693

(But still I get a crash in _freeze_importlib)

Include/cpython/code.h

gvanrossum · 2021-06-15T23:28:41Z

Lib/importlib/_bootstrap_external.py

@@ -359,6 +359,7 @@ def _write_atomic(path, data, mode=0o666):
 #     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)


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

what are our options?

what are our options?

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

Objects/codeobject.c

Objects/frameobject.c

gvanrossum · 2021-06-16T00:02:32Z

@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?

ericsnowcurrently · 2021-06-16T00:09:39Z

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

bedevere-bot · 2021-06-16T01:59:56Z

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

gvanrossum · 2021-06-16T03:28:57Z

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.

gvanrossum · 2021-06-16T14:21:03Z

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

ericsnowcurrently

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.

Objects/codeobject.c

Python/compile.c

ericsnowcurrently · 2021-06-17T00:34:41Z

Python/compile.c

+static PyObject *
+compute_localsplus_info(struct compiler *c, PyObject *names)


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)

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

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.

Python/compile.c

gvanrossum · 2021-06-18T16:38:50Z

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.

gvanrossum · 2021-06-18T16:39:18Z

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

gvanrossum · 2021-06-19T19:42:56Z

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?

markshannon · 2021-06-21T09:29:57Z

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

gvanrossum · 2021-06-21T14:11:51Z

@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?

gvanrossum · 2021-06-21T15:42:44Z

@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

gvanrossum · 2021-06-21T17:38:45Z

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.

gvanrossum added 6 commits June 15, 2021 15:51

Make co_localspluskinds an object instead of a raw pointer

da37894

Regenerate frozen files

7564dbe

Use PyBytes_AsString instead of PyBytes_AS_STRING

32cc42e

(But still I get a crash in _freeze_importlib)

Fix refcnt bug; tighten checks; regen importlib

9e54613

Regenerate more frozen things

e5b413e

Merge main (esp. Eric's PR)

00954ec

gvanrossum requested a review from ericsnowcurrently June 15, 2021 23:23

gvanrossum requested review from markshannon and tiran as code owners June 15, 2021 23:23

the-knights-who-say-ni added the CLA signed label Jun 15, 2021

bedevere-bot added the awaiting core review label Jun 15, 2021

gvanrossum added the skip news label Jun 15, 2021

gvanrossum commented Jun 15, 2021

View reviewed changes

Tweaks I noticed while reviewing this myself

4fba934

gvanrossum marked this pull request as draft June 15, 2021 23:36

Fix PyCode_NewEmpty()

ebaaeca

Fix test_ctypes (marshalled code size changed)

b09738c

gvanrossum added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jun 16, 2021

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jun 16, 2021

gvanrossum marked this pull request as ready for review June 16, 2021 14:21

ericsnowcurrently approved these changes Jun 17, 2021

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels Jun 17, 2021

gvanrossum added 2 commits June 17, 2021 18:37

Tweak things noted in code review

703727c

Make symbol static

a64057a

Merge main at 06cda80

0ff4466

gvanrossum merged commit 355f5dd into python:main Jun 21, 2021

bedevere-bot removed the awaiting merge label Jun 21, 2021

gvanrossum deleted the cleaner-kinds branch June 21, 2021 20:53

markshannon mentioned this pull request May 2, 2022

Logically merge cell and locals array. They are already contiguous in memory #87859

Closed

		static PyObject *
		compute_localsplus_info(struct compiler c, PyObject names)

Uh oh!

bpo-43693: Turn localspluskinds into an object #26749

bpo-43693: Turn localspluskinds into an object #26749

Uh oh!

Conversation

gvanrossum commented Jun 15, 2021 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

gvanrossum Jun 15, 2021

Choose a reason for hiding this comment

Uh oh!

ericsnowcurrently Jun 16, 2021

Choose a reason for hiding this comment

Uh oh!

gvanrossum Jun 16, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gvanrossum commented Jun 16, 2021

Uh oh!

ericsnowcurrently commented Jun 16, 2021

Uh oh!

bedevere-bot commented Jun 16, 2021

Uh oh!

gvanrossum commented Jun 16, 2021

Uh oh!

gvanrossum commented Jun 16, 2021

Uh oh!

ericsnowcurrently left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ericsnowcurrently Jun 17, 2021

Choose a reason for hiding this comment

Uh oh!

ericsnowcurrently Jun 17, 2021

Choose a reason for hiding this comment

Uh oh!

gvanrossum Jun 18, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gvanrossum commented Jun 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gvanrossum commented Jun 18, 2021

Uh oh!

gvanrossum commented Jun 19, 2021 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

markshannon commented Jun 21, 2021

Uh oh!

gvanrossum commented Jun 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gvanrossum commented Jun 21, 2021

Uh oh!

gvanrossum commented Jun 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

gvanrossum commented Jun 15, 2021 •

edited by bedevere-bot

Loading

gvanrossum commented Jun 18, 2021 •

edited

Loading

gvanrossum commented Jun 19, 2021 •

edited by bedevere-bot

Loading

gvanrossum commented Jun 21, 2021 •

edited

Loading

gvanrossum commented Jun 21, 2021 •

edited

Loading