classification
Title: Logically merge cell and locals array. They are already contiguous in memory
Type: performance Stage: patch review
Components: Interpreter Core Versions:
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: eric.snow Nosy List: Guido.van.Rossum, Mark.Shannon, eric.snow, gvanrossum, pablogsal, rhettinger, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2021-04-01 11:33 by Mark.Shannon, last changed 2021-06-23 16:51 by gvanrossum.

Pull Requests
URL Status Linked Edit
PR 25152 merged Mark.Shannon, 2021-04-02 13:56
PR 26216 merged eric.snow, 2021-05-18 19:14
PR 26258 closed eric.snow, 2021-05-19 23:51
PR 26364 merged eric.snow, 2021-05-25 20:19
PR 26375 merged eric.snow, 2021-05-26 00:58
PR 26388 merged eric.snow, 2021-05-26 20:08
PR 26396 merged eric.snow, 2021-05-27 02:34
PR 26530 merged pablogsal, 2021-06-04 16:32
PR 26577 merged eric.snow, 2021-06-07 15:23
PR 26587 merged eric.snow, 2021-06-07 23:03
PR 26588 merged eric.snow, 2021-06-07 23:34
PR 26597 merged pablogsal, 2021-06-08 11:11
PR 26609 merged eric.snow, 2021-06-08 18:39
PR 26626 merged eric.snow, 2021-06-09 17:21
PR 26749 merged gvanrossum, 2021-06-15 23:23
PR 26839 merged gvanrossum, 2021-06-21 22:30
Messages (31)
msg389974 - (view) Author: Mark Shannon (Mark.Shannon) * (Python committer) Date: 2021-04-01 11:33
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.
msg390069 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2021-04-02 15:09
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.
msg390247 - (view) Author: Guido van Rossum (Guido.van.Rossum) Date: 2021-04-05 18:44
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.
msg390253 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2021-04-05 19:48
LGTM.
msg394455 - (view) Author: Mark Shannon (Mark.Shannon) * (Python committer) Date: 2021-05-26 19:15
New changeset 6cc800d3634fdd002b986c3ffe6a3d5540f311a0 by Eric Snow in branch 'main':
bpo-43693: Clean up the PyCodeObject fields. (GH-26364)
https://github.com/python/cpython/commit/6cc800d3634fdd002b986c3ffe6a3d5540f311a0
msg394556 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2021-05-27 15:54
New changeset 9f494d492944c3a6a7a7471b4ad3a025dc7de289 by Eric Snow in branch 'main':
bpo-43693: Add _PyCode_New(). (gh-26375)
https://github.com/python/cpython/commit/9f494d492944c3a6a7a7471b4ad3a025dc7de289
msg395014 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2021-06-03 16:28
New changeset 2c1e2583fdc4db6b43d163239ea42b0e8394171f by Eric Snow in branch 'main':
bpo-43693: Add new internal code objects fields: co_fastlocalnames and co_fastlocalkinds. (gh-26388)
https://github.com/python/cpython/commit/2c1e2583fdc4db6b43d163239ea42b0e8394171f
msg395059 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2021-06-04 00:04
New changeset b2bf2bc1ece673d387341e06c8d3c2bc6e259747 by Mark Shannon in branch 'main':
bpo-43693: Compute deref offsets in compiler (gh-25152)
https://github.com/python/cpython/commit/b2bf2bc1ece673d387341e06c8d3c2bc6e259747
msg395097 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2021-06-04 16:11
Unfortunately, commit 2c1e2583fdc4db6b43d163239ea42b0e8394171f has broken all the refleak buildbots. Example failure:

BISECTION:

c1e2583fdc4db6b43d163239ea42b0e8394171f is the first bad commit
commit 2c1e2583fdc4db6b43d163239ea42b0e8394171f
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 2c1e2583fdc4db6b43d163239ea42b0e8394171f directly to not mask other issues.
msg395098 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2021-06-04 16:12
Please, in the future, check with the buildbots before merging these kinds of big PRs.
msg395102 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2021-06-04 16:39
PR reverts commits 2c1e2583fdc4db6b43d163239ea42b0e8394171f and b2bf2bc1ece673d387341e06c8d3c2bc6e259747. 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 2c1e2583fdc4db6b43d163239ea42b0e8394171f calling _PyCode_GetVarnames and _PyCode_GetFreevars without decrementing the reference after use but there are more.
msg395105 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2021-06-04 16:51
New changeset 17c4edc4e0692fe55e185755ea8a2f5238f3ef08 by Pablo Galindo in branch 'main':
bpo-43693: Revert commits 2c1e2583fdc4db6b43d163239ea42b0e8394171f and b2bf2bc1ece673d387341e06c8d3c2bc6e259747  (GH-26530)
https://github.com/python/cpython/commit/17c4edc4e0692fe55e185755ea8a2f5238f3ef08
msg395137 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2021-06-04 23:07
Thanks Pablo.  I'll get this sorted out.  Sorry for the pain.
msg395273 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2021-06-07 18:22
New changeset 2ab27c4af4ddf7528e1375e77c787c7fbb09b5e6 by Eric Snow in branch 'main':
bpo-43693: Un-revert commits 2c1e258 and b2bf2bc. (gh-26577)
https://github.com/python/cpython/commit/2ab27c4af4ddf7528e1375e77c787c7fbb09b5e6
msg395294 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2021-06-07 22:52
New changeset 631f9938b1604d4f893417ec339b9e0fa9196fb1 by Eric Snow in branch 'main':
bpo-43693: Add the MAKE_CELL opcode and interleave fast locals offsets. (gh-26396)
https://github.com/python/cpython/commit/631f9938b1604d4f893417ec339b9e0fa9196fb1
msg395297 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2021-06-07 23:58
New changeset 165c884154901deae46b5e328a6414d130e6bfff by Eric Snow in branch 'main':
bpo-43693: Silence some compiler warnings. (gh-26588)
https://github.com/python/cpython/commit/165c884154901deae46b5e328a6414d130e6bfff
msg395317 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2021-06-08 10:43
Hi,

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

https://buildbot.python.org/all/#/builders/585
msg395318 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2021-06-08 10:45
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
msg395319 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2021-06-08 10:49
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.
msg395321 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2021-06-08 11:24
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
msg395326 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2021-06-08 12:18
New changeset 3fe921cd49959181163671364c8b84faa88f7895 by Pablo Galindo in branch 'main':
Revert "bpo-43693: Add the MAKE_CELL opcode and interleave fast locals offsets. (gh-26396)" (GH-26597)
https://github.com/python/cpython/commit/3fe921cd49959181163671364c8b84faa88f7895
msg395329 - (view) Author: Mark Shannon (Mark.Shannon) * (Python committer) Date: 2021-06-08 13:11
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
msg395331 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2021-06-08 13:21
> 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.
msg395332 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2021-06-08 13:24
> 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
msg395338 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2021-06-08 16:18
Thanks, Pablo!
msg395365 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2021-06-08 22:01
New changeset 3e1c7167d86a2a928cdcb659094aa10bb5550c4c by Eric Snow in branch 'main':
 bpo-43693: Un-revert commit f3fa63e. (#26609)
https://github.com/python/cpython/commit/3e1c7167d86a2a928cdcb659094aa10bb5550c4c
msg395448 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2021-06-09 17:41
New changeset e6e34e45222b9c7a63ba92386612acf768082ba0 by Eric Snow in branch 'main':
bpo-43693: Do not check co_cell2arg if a non-cell offset. (gh-26626)
https://github.com/python/cpython/commit/e6e34e45222b9c7a63ba92386612acf768082ba0
msg395899 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2021-06-15 22:35
New changeset ac38a9f2dfbba95f5d4338eb11a0221d38ef9328 by Eric Snow in branch 'main':
bpo-43693: Eliminate unused "fast locals". (gh-26587)
https://github.com/python/cpython/commit/ac38a9f2dfbba95f5d4338eb11a0221d38ef9328
msg396015 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2021-06-17 18:51
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)
msg396288 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2021-06-21 20:53
New changeset 355f5dd36a0f53175517f35798aa874564d1113a by Guido van Rossum in branch 'main':
bpo-43693: Turn localspluskinds into an object (GH-26749)
https://github.com/python/cpython/commit/355f5dd36a0f53175517f35798aa874564d1113a
msg396433 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2021-06-23 16:51
New changeset 769d7d0c66c5b86e2dd29b9ce67ac2daaab1bb38 by Guido van Rossum in branch 'main':
bpo-43693 Get rid of CO_NOFREE -- it's unused (GH-26839)
https://github.com/python/cpython/commit/769d7d0c66c5b86e2dd29b9ce67ac2daaab1bb38
History
Date User Action Args
2021-06-23 16:51:51gvanrossumsetmessages: + msg396433
2021-06-21 22:30:32gvanrossumsetpull_requests: + pull_request25420
2021-06-21 20:53:14gvanrossumsetmessages: + msg396288
2021-06-17 18:51:41eric.snowsetmessages: + msg396015
2021-06-17 10:39:48Mark.Shannonsetassignee: Mark.Shannon -> eric.snow
2021-06-15 23:23:44gvanrossumsetpull_requests: + pull_request25334
2021-06-15 22:35:33eric.snowsetmessages: + msg395899
2021-06-09 17:41:03eric.snowsetmessages: + msg395448
2021-06-09 17:21:42eric.snowsetpull_requests: + pull_request25212
2021-06-08 22:01:42eric.snowsetmessages: + msg395365
2021-06-08 18:39:27eric.snowsetpull_requests: + pull_request25192
2021-06-08 16:18:22eric.snowsetmessages: + msg395338
2021-06-08 13:24:19pablogsalsetmessages: + msg395332
2021-06-08 13:22:56pablogsalsetmessages: - msg395330
2021-06-08 13:21:23pablogsalsetmessages: + msg395331
2021-06-08 13:19:40pablogsalsetmessages: + msg395330
2021-06-08 13:11:31Mark.Shannonsetmessages: + msg395329
2021-06-08 12:18:01pablogsalsetmessages: + msg395326
2021-06-08 11:24:22pablogsalsetmessages: + msg395321
2021-06-08 11:11:38pablogsalsetpull_requests: + pull_request25181
2021-06-08 10:49:38pablogsalsetmessages: + msg395319
2021-06-08 10:45:56pablogsalsetmessages: + msg395318
2021-06-08 10:43:37pablogsalsetmessages: + msg395317
2021-06-07 23:58:55eric.snowsetmessages: + msg395297
2021-06-07 23:34:34eric.snowsetpull_requests: + pull_request25173
2021-06-07 23:03:19eric.snowsetpull_requests: + pull_request25172
2021-06-07 22:52:09eric.snowsetmessages: + msg395294
2021-06-07 18:22:46eric.snowsetmessages: + msg395273
2021-06-07 15:23:56eric.snowsetpull_requests: + pull_request25165
2021-06-04 23:07:04eric.snowsetmessages: + msg395137
2021-06-04 16:51:09pablogsalsetmessages: + msg395105
2021-06-04 16:39:22pablogsalsetmessages: + msg395102
2021-06-04 16:32:28pablogsalsetpull_requests: + pull_request25123
2021-06-04 16:12:54pablogsalsetmessages: + msg395098
2021-06-04 16:11:16pablogsalsetnosy: + pablogsal
messages: + msg395097
2021-06-04 00:04:03eric.snowsetmessages: + msg395059
2021-06-03 16:28:35eric.snowsetmessages: + msg395014
2021-05-27 15:54:43eric.snowsetmessages: + msg394556
2021-05-27 02:34:44eric.snowsetpull_requests: + pull_request24989
2021-05-26 20:46:23gvanrossumsetnosy: + gvanrossum
2021-05-26 20:08:10eric.snowsetpull_requests: + pull_request24981
2021-05-26 19:15:52Mark.Shannonsetmessages: + msg394455
2021-05-26 00:58:30eric.snowsetpull_requests: + pull_request24967
2021-05-25 20:19:15eric.snowsetpull_requests: + pull_request24955
2021-05-19 23:51:48eric.snowsetpull_requests: + pull_request24865
2021-05-18 19:14:57eric.snowsetnosy: + eric.snow
pull_requests: + pull_request24833
2021-04-05 19:48:29serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg390253
2021-04-05 18:44:58Guido.van.Rossumsetnosy: + Guido.van.Rossum
messages: + msg390247
2021-04-02 15:09:54rhettingersetnosy: + rhettinger
messages: + msg390069
2021-04-02 13:56:29Mark.Shannonsetkeywords: + patch
stage: needs patch -> patch review
pull_requests: + pull_request23899
2021-04-01 11:33:50Mark.Shannoncreate