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

[markshannon]

BPO	43693
Nosy	@gvanrossum, @rhettinger, @ncoghlan, @markshannon, @ericsnowcurrently, @serhiy-storchaka, @gvanrossum, @pablogsal
PRs	bpo-43693: Compute deref offsets in compiler #25152 bpo-43693: Group the code in codeobject.c logically. #26216 bpo-43693: Add _PyCode_New() and do some related cleanup. #26258 bpo-43693: Clean up the PyCodeObject fields. #26364 bpo-43693: Add _PyCode_New(). #26375 bpo-43693: Add new internal code objects fields: co_fastlocalnames and co_fastlocalkinds. #26388 bpo-43693: Add the MAKE_CELL opcode and interleave fast locals offsets. #26396 bpo-43693: Revert commits 2c1e2583fdc4db6b43d163239ea42b0e8394171f and b2bf2bc1ece673d387341e06c8d3c2bc6e259747  #26530 bpo-43693: Un-revert commits 2c1e258 and b2bf2bc. #26577 bpo-43693: Eliminate unused "fast locals". #26587 bpo-43693: Silence some compiler warnings. #26588 Revert "bpo-43693: Add the MAKE_CELL opcode and interleave fast locals offsets. (gh-26396)" #26597 bpo-43693: Un-revert commit f3fa63e. #26609 bpo-43693: Do not check co_cell2arg if a non-cell offset. #26626 bpo-43693: Turn localspluskinds into an object #26749 bpo-43693 Get rid of CO_NOFREE -- it's unused #26839

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = None
created_at = <Date 2021-04-01.11:33:50.457>
labels = ['interpreter-core', 'performance']
title = 'Logically merge cell and locals array. They are already contiguous in memory'
updated_at = <Date 2021-07-21.18:07:49.033>
user = 'https://github.com/markshannon'

bugs.python.org fields:

activity = <Date 2021-07-21.18:07:49.033>
actor = 'gvanrossum'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Interpreter Core']
creation = <Date 2021-04-01.11:33:50.457>
creator = 'Mark.Shannon'
dependencies = []
files = []
hgrepos = []
issue_num = 43693
keywords = ['patch']
message_count = 32.0
messages = ['389974', '390069', '390247', '390253', '394455', '394556', '395014', '395059', '395097', '395098', '395102', '395105', '395137', '395273', '395294', '395297', '395317', '395318', '395319', '395321', '395326', '395329', '395331', '395332', '395338', '395365', '395448', '395899', '396015', '396288', '396433', '397956']
nosy_count = 8.0
nosy_names = ['gvanrossum', 'rhettinger', 'ncoghlan', 'Mark.Shannon', 'eric.snow', 'serhiy.storchaka', 'Guido.van.Rossum', 'pablogsal']
pr_nums = ['25152', '26216', '26258', '26364', '26375', '26388', '26396', '26530', '26577', '26587', '26588', '26597', '26609', '26626', '26749', '26839']
priority = 'normal'
resolution = None
stage = 'needs patch'
status = 'open'
superseder = None
type = 'performance'
url = 'https://bugs.python.org/issue43693'
versions = []

[markshannon]

In the interpreter and compiler, the "fast" locals array and cells array are treated separately. By merging them in the compiler, the interpreter can be simplified a bit.

[rhettinger]

This doesn't look simpler to me. We will forever lose mental clock cycles disentangling the locals and cells. So, I don't think they should be commingled.

[gvanrossum]

To me it looks simpler though. The locals and cells are already stored in a single array, f_localsplus (which also contains the evaluation stack). There are various complications in ceval.c to translate cell indexes to indexes in this array (ein particular the extra local variable 'freevars', which weighs down the stack frame).

Making the interpreter simpler by moving things to the compiler also makes it easier for the C compiler to optimize the code of the interpreter better.

[serhiy-storchaka]

LGTM.

[markshannon]

New changeset 6cc800d by Eric Snow in branch 'main':
bpo-43693: Clean up the PyCodeObject fields. (GH-26364)
6cc800d

[ericsnowcurrently]

New changeset 9f494d4 by Eric Snow in branch 'main':
bpo-43693: Add _PyCode_New(). (gh-26375)
9f494d4

[ericsnowcurrently]

New changeset 2c1e258 by Eric Snow in branch 'main':
bpo-43693: Add new internal code objects fields: co_fastlocalnames and co_fastlocalkinds. (gh-26388)
2c1e258

[ericsnowcurrently]

New changeset b2bf2bc by Mark Shannon in branch 'main':
bpo-43693: Compute deref offsets in compiler (gh-25152)
b2bf2bc

[pablogsal]

Unfortunately, commit 2c1e258 has broken all the refleak buildbots. Example failure:

BISECTION:

c1e2583fdc4db6b43d163239ea42b0e8394171f is the first bad commit
commit 2c1e258
Author: Eric Snow <ericsnowcurrently@gmail.com>
Date: Thu Jun 3 10:28:27 2021 -0600

bpo-43693: Add new internal code objects fields: co_fastlocalnames and co_fastlocalkinds. (gh-26388)

https://buildbot.python.org/all/#/builders/384/builds/50/steps/5/logs/stdio

As this is affecting all test, I am proceeding with a revert of commit 2c1e258 directly to not mask other issues.

[pablogsal]

Please, in the future, check with the buildbots before merging these kinds of big PRs.

[pablogsal]

PR reverts commits 2c1e258 and b2bf2bc. Please, commit them back once the refleaks are resolved. I took a quick look and seems that there are several issues but there are more I could find. Some of the issues seem to be related to commit 2c1e258 calling _PyCode_GetVarnames and _PyCode_GetFreevars without decrementing the reference after use but there are more.

[pablogsal]

New changeset 17c4edc by Pablo Galindo in branch 'main':
bpo-43693: Revert commits 2c1e258 and b2bf2bc (GH-26530)
17c4edc

[ericsnowcurrently]

Thanks Pablo. I'll get this sorted out. Sorry for the pain.

[ericsnowcurrently]

New changeset 2ab27c4 by Eric Snow in branch 'main':
bpo-43693: Un-revert commits 2c1e258 and b2bf2bc. (gh-26577)
2ab27c4

[ericsnowcurrently]

New changeset 631f993 by Eric Snow in branch 'main':
bpo-43693: Add the MAKE_CELL opcode and interleave fast locals offsets. (gh-26396)
631f993

[ericsnowcurrently]

New changeset 165c884 by Eric Snow in branch 'main':
bpo-43693: Silence some compiler warnings. (gh-26588)
165c884

[pablogsal]

Hi,

Unfortunately, the address sanitizer buildbot has been broken by commit631f9938b1604d4f893417ec339b9e0fa9196fb1 :

https://buildbot.python.org/all/#/builders/585

[pablogsal]

Here is the top trace:

test_jump_in_nested_finally_3 (test.test_sys_settrace.JumpTestCase) ... ok
test_jump_into_finally_block (test.test_sys_settrace.JumpTestCase) ... ok
test_jump_into_finally_block_from_try_block (test.test_sys_settrace.JumpTestCase) ... ok
=================================================================
==28726==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6040000db2c8 at pc 0x55fb8bd662d1 bp 0x7ffef9e25cf0 sp 0x7ffef9e25ce0
READ of size 4 at 0x6040000db2c8 thread T0
#0 0x55fb8bd662d0 in PyFrame_FastToLocalsWithError Objects/frameobject.c:985
#1 0x55fb8bbad5a3 in call_trampoline Python/sysmodule.c:953
#2 0x55fb8bbb0eb5 in trace_trampoline Python/sysmodule.c:1008
#3 0x55fb8bac9a03 in call_trace Python/ceval.c:5618
#4 0x55fb8bacd3c6 in call_trace_protected Python/ceval.c:5576
#5 0x55fb8bad8a57 in _PyEval_EvalFrameDefault Python/ceval.c:1631
#6 0x55fb8bafc2ff in

[pablogsal]

Eric, I saw thay you ran the buildbots on your PR (thanks a lot), but something has gone wrong in some of them and they didn't run correctly. For example:

https://buildbot.python.org/all/#/builders/581/builds/58

I'm investigating why that this. Meanwhile, could you take a look? We have already some problems on windows and we don't want more stuff to pile up.

[pablogsal]

The problem seems to be that The problem is that PyFrame_LocalsToFast(frame, 1); calls into PyFrame_LocalsToFast where co->co_nlocals has a value of 3, and this causes co->co_cell2arg[i - co->co_nlocals]; to access outside the bounds of the array.

As this change is affecting all tests (there are many tests in the test suite that fail with a buffer overflow in the ASAN builds) I opened PR 26597 to revert commit commit631f9938b1604d4f893417ec339b9e0fa9196fb1. As always, please, recommit commit631f9938b1604d4f893417ec339b9e0fa9196fb1 once this issues are fixed.

To reproduce these failures locally you can:

$ export ASAN_OPTIONS=detect_leaks=0:allocator_may_return_null=1:handle_segv=0
$ ./configure --with-address-sanitizer --without-pymalloc
$ make -j -s
$ ./python -m test test_sys_settrac

[pablogsal]

New changeset 3fe921c by Pablo Galindo in branch 'main':
Revert "bpo-43693: Add the MAKE_CELL opcode and interleave fast locals offsets. (gh-26396)" (GH-26597)
3fe921c

[markshannon]

Pablo,

Is there a bpo issue for the buildbot failures on Windows?
The failures I've been seeing are C stack overflows.

Long term, I expect to fix it by decoupling the C and Python stacks.
In the short term I have a couple of changes that might get it working again

[pablogsal]

Long term, I expect to fix it by decoupling the C and Python stacks.

That won't fix half of the Windows failures because the stack overflow you are seeing are for C to C calls if I am not mistaken. This happens on very deep calls when converting Python objects to C objects. There are no Python frames involved in that case IIRC.

[pablogsal]

Are there a bpo issue for the buildbot failures on Windows?

Yes, this one: https://bugs.python.org/issue44348

But just to clarify, the ones affecting this issue are the ones in ASAN, for example:

https://buildbot.python.org/all/#/builders/582/builds/227/steps/5/logs/stdio

[ericsnowcurrently]

Thanks, Pablo!

[ericsnowcurrently]

New changeset 3e1c716 by Eric Snow in branch 'main':
bpo-43693: Un-revert commit f3fa63e. (bpo-26609)
3e1c716

[ericsnowcurrently]

New changeset e6e34e4 by Eric Snow in branch 'main':
bpo-43693: Do not check co_cell2arg if a non-cell offset. (gh-26626)
e6e34e4

[ericsnowcurrently]

New changeset ac38a9f by Eric Snow in branch 'main':
bpo-43693: Eliminate unused "fast locals". (gh-26587)
ac38a9f

[ericsnowcurrently]

FWIW, I've wrapped up the key parts that I wanted to get done here
(co_localplusnames/kinds, MAKE_CELL, eliminate unused fast local
for arg cells). I'm leaving this open for now as there are a few
things I didn't do that seem part of the original intention of this
issue:

• fully interleave the cells with the locals in their "natural" order
  (rather than only interleaving arg cells)
• update the compiler to track names/kinds rather than computing
  them after the fact during the assembler step
  (this will allow us to remove a decent amount of code)
• track the specific arg kinds in localspluskinds
  (this should allow us to make _PyEval_MakeFrameVector() simpler
  and a bit more efficient)

[gvanrossum]

New changeset 355f5dd by Guido van Rossum in branch 'main':
bpo-43693: Turn localspluskinds into an object (GH-26749)
355f5dd

[gvanrossum]

New changeset 769d7d0 by Guido van Rossum in branch 'main':
bpo-43693 Get rid of CO_NOFREE -- it's unused (GH-26839)
769d7d0

[gvanrossum]

Another task seems to have appeared, Nick Coghlan would like the typedef to be renamed to PyLocal_VarKind, and the getter/setter similarly.

[markshannon]

I don't think there is anything left to do here, so closing.