Repository: python/cpython
Pull request: gh-100758: Refactor initialisation of frame headers into a single function (_PyFrame_Initialize)
Fixes #100758.

This is version 1 (with an arg telling where to start NULLing).

<!-- gh-issue-number: gh-100758 -->
* Issue: gh-100758
<!-- /gh-issue-number -->


Problem statement:
Refactor initialisation of frame headers into a single function
There are 3 places in the code where we prepare a frame for execution:

1. _PyFrame_PushUnchecked in pycore_frame.h
2. init_frame in frameobject.c
3. _PyEvalFramePushAndInit in ceval.c

All of these call the function _PyFrame_InitializeSpecials to set values from the code object to various fields on the frame. Two of them (2 and 3 above) also NULL the localsplus array.

When we will change the frame layout for the register machine, currently we need to update those three places. But if we add NULLing the locals to _PyFrame_InitializeSpecials (and rename it to something clearer) then we can just make our changes there. 

There isn't really a problem with NULLing the localsplus in _PyFrame_PushUnchecked - all the call sites follow up by setting some fields and NULLing the rest. So now we will NULL all fields and overwrite a few after returning from _PyFrame_InitializeSpecials. Will benchmark to make sure.

(Lesson learned from the register machine experiment).



<!-- gh-linked-prs -->
### Linked PRs
* gh-100759
* gh-100775
<!-- /gh-linked-prs -->

Hints from issue discussion:

Commit message: gh-100758: Refactor initialisation of frame headers into a single function (_PyFrame_InitializeHeader)

Patch to review:
diff --git a/Include/internal/pycore_frame.h b/Include/internal/pycore_frame.h
index f18723b303224f..f13cb32afbc277 100644
--- a/Include/internal/pycore_frame.h
+++ b/Include/internal/pycore_frame.h
@@ -101,7 +101,7 @@ void _PyFrame_Copy(_PyInterpreterFrame *src, _PyInterpreterFrame *dest);
    when frame is linked into the frame stack.
  */
 static inline void
-_PyFrame_InitializeSpecials(
+_PyFrame_InitializeHeader(
     _PyInterpreterFrame *frame, PyFunctionObject *func,
     PyObject *locals, PyCodeObject *code)
 {
@@ -115,6 +115,11 @@ _PyFrame_InitializeSpecials(
     frame->prev_instr = _PyCode_CODE(code) - 1;
     frame->yield_offset = 0;
     frame->owner = FRAME_OWNED_BY_THREAD;
+
+    PyObject **localsarray = &frame->localsplus[0];
+    for (int i = 0; i < code->co_nlocalsplus; i++) {
+        localsarray[i] = NULL;
+    }
 }
 
 /* Gets the pointer to the locals array
@@ -222,7 +227,7 @@ _PyFrame_PushUnchecked(PyThreadState *tstate, PyFunctionObject *func)
     _PyInterpreterFrame *new_frame = (_PyInterpreterFrame *)tstate->datastack_top;
     tstate->datastack_top += code->co_framesize;
     assert(tstate->datastack_top < tstate->datastack_limit);
-    _PyFrame_InitializeSpecials(new_frame, func, NULL, code);
+    _PyFrame_InitializeHeader(new_frame, func, NULL, code);
     return new_frame;
 }
 
diff --git a/Objects/frameobject.c b/Objects/frameobject.c
index eab85c08fc0165..d4790a351034cc 100644
--- a/Objects/frameobject.c
+++ b/Objects/frameobject.c
@@ -1011,12 +1011,9 @@ static void
 init_frame(_PyInterpreterFrame *frame, PyFunctionObject *func, PyObject *locals)
 {
     PyCodeObject *code = (PyCodeObject *)func->func_code;
-    _PyFrame_InitializeSpecials(frame, (PyFunctionObject*)Py_NewRef(func),
-                                Py_XNewRef(locals), code);
+    _PyFrame_InitializeHeader(frame, (PyFunctionObject*)Py_NewRef(func),
+                              Py_XNewRef(locals), code);
     frame->previous = NULL;
-    for (Py_ssize_t i = 0; i < code->co_nlocalsplus; i++) {
-        frame->localsplus[i] = NULL;
-    }
 }
 
 PyFrameObject*
diff --git a/Python/bytecodes.c b/Python/bytecodes.c
index 839fac3fcd1176..a9f3168258310b 100644
--- a/Python/bytecodes.c
+++ b/Python/bytecodes.c
@@ -457,9 +457,6 @@ dummy_func(
             STACK_SHRINK(2);
             new_frame->localsplus[0] = container;
             new_frame->localsplus[1] = sub;
-            for (int i = 2; i < code->co_nlocalsplus; i++) {
-                new_frame->localsplus[i] = NULL;
-            }
             JUMPBY(INLINE_CACHE_ENTRIES_BINARY_SUBSCR);
             DISPATCH_INLINED(new_frame);
         }
@@ -1787,9 +1784,6 @@ dummy_func(
             int shrink_stack = !(oparg & 1);
             STACK_SHRINK(shrink_stack);
             new_frame->localsplus[0] = owner;
-            for (int i = 1; i < code->co_nlocalsplus; i++) {
-                new_frame->localsplus[i] = NULL;
-            }
             JUMPBY(INLINE_CACHE_ENTRIES_LOAD_ATTR);
             DISPATCH_INLINED(new_frame);
         }
@@ -1823,9 +1817,6 @@ dummy_func(
             STACK_SHRINK(shrink_stack);
             new_frame->localsplus[0] = owner;
             new_frame->localsplus[1] = Py_NewRef(name);
-            for (int i = 2; i < code->co_nlocalsplus; i++) {
-                new_frame->localsplus[i] = NULL;
-            }
             JUMPBY(INLINE_CACHE_ENTRIES_LOAD_ATTR);
             DISPATCH_INLINED(new_frame);
         }
@@ -2793,9 +2784,6 @@ dummy_func(
             for (int i = 0; i < argcount; i++) {
                 new_frame->localsplus[i] = stack_pointer[i];
             }
-            for (int i = argcount; i < code->co_nlocalsplus; i++) {
-                new_frame->localsplus[i] = NULL;
-            }
             STACK_SHRINK(2-is_meth);
             JUMPBY(INLINE_CACHE_ENTRIES_CALL);
             DISPATCH_INLINED(new_frame);
@@ -2828,9 +2816,6 @@ dummy_func(
                                                  i - minargs);
                 new_frame->localsplus[i] = Py_NewRef(def);
             }
-            for (int i = code->co_argcount; i < code->co_nlocalsplus; i++) {
-                new_frame->localsplus[i] = NULL;
-            }
             STACK_SHRINK(2-is_meth);
             JUMPBY(INLINE_CACHE_ENTRIES_CALL);
             DISPATCH_INLINED(new_frame);
diff --git a/Python/ceval.c b/Python/ceval.c
index 45f42800d7ce58..503891f481ccfc 100644
--- a/Python/ceval.c
+++ b/Python/ceval.c
@@ -1974,11 +1974,8 @@ _PyEvalFramePushAndInit(PyThreadState *tstate, PyFunctionObject *func,
     if (frame == NULL) {
         goto fail;
     }
-    _PyFrame_InitializeSpecials(frame, func, locals, code);
+    _PyFrame_InitializeHeader(frame, func, locals, code);
     PyObject **localsarray = &frame->localsplus[0];
-    for (int i = 0; i < code->co_nlocalsplus; i++) {
-        localsarray[i] = NULL;
-    }
     if (initialize_locals(tstate, func, localsarray, args, argcount, kwnames)) {
         assert(frame->owner != FRAME_OWNED_BY_GENERATOR);
         _PyEvalFrameClearAndPop(tstate, frame);
diff --git a/Python/generated_cases.c.h b/Python/generated_cases.c.h
index ed89e90b7c564d..93b3d0393b3c80 100644
--- a/Python/generated_cases.c.h
+++ b/Python/generated_cases.c.h
@@ -578,9 +578,6 @@
             STACK_SHRINK(2);
             new_frame->localsplus[0] = container;
             new_frame->localsplus[1] = sub;
-            for (int i = 2; i < code->co_nlocalsplus; i++) {
-                new_frame->localsplus[i] = NULL;
-            }
             JUMPBY(INLINE_CACHE_ENTRIES_BINARY_SUBSCR);
             DISPATCH_INLINED(new_frame);
         }
@@ -2027,9 +2024,6 @@
             int shrink_stack = !(oparg & 1);
             STACK_SHRINK(shrink_stack);
             new_frame->localsplus[0] = owner;
-            for (int i = 1; i < code->co_nlocalsplus; i++) {
-                new_frame->localsplus[i] = NULL;
-            }
             JUMPBY(INLINE_CACHE_ENTRIES_LOAD_ATTR);
             DISPATCH_INLINED(new_frame);
         }
@@ -2062,9 +2056,6 @@
             STACK_SHRINK(shrink_stack);
             new_frame->localsplus[0] = owner;
             new_frame->localsplus[1] = Py_NewRef(name);
-            for (int i = 2; i < code->co_nlocalsplus; i++) {
-                new_frame->localsplus[i] = NULL;
-            }
             JUMPBY(INLINE_CACHE_ENTRIES_LOAD_ATTR);
             DISPATCH_INLINED(new_frame);
         }
@@ -3148,9 +3139,6 @@
             for (int i = 0; i < argcount; i++) {
                 new_frame->localsplus[i] = stack_pointer[i];
             }
-            for (int i = argcount; i < code->co_nlocalsplus; i++) {
-                new_frame->localsplus[i] = NULL;
-            }
             STACK_SHRINK(2-is_meth);
             JUMPBY(INLINE_CACHE_ENTRIES_CALL);
             DISPATCH_INLINED(new_frame);
@@ -3182,9 +3170,6 @@
                                                  i - minargs);
                 new_frame->localsplus[i] = Py_NewRef(def);
             }
-            for (int i = code->co_argcount; i < code->co_nlocalsplus; i++) {
-                new_frame->localsplus[i] = NULL;
-            }
             STACK_SHRINK(2-is_meth);
             JUMPBY(INLINE_CACHE_ENTRIES_CALL);
             DISPATCH_INLINED(new_frame);


Code review comments:
diff_hunk: @@ -101,7 +101,7 @@ void _PyFrame_Copy(_PyInterpreterFrame *src, _PyInterpreterFrame *dest);
    when frame is linked into the frame stack.
  */
 static inline void
-_PyFrame_InitializeSpecials(
+_PyFrame_InitializeHeader(
path: Include/internal/pycore_frame.h
text: @user1:
If you initialize the locals, then this initializes the whole frame. Maybe call it `_PyFrame_Initialize`?