gh-106608: make uop trace variable length #107531

rdrf2838 · 2023-08-01T09:32:34Z

The _PyUOpExecutorObject was hard-coded to have a length-32 trace buffer of _PyUOpInstructions. The goal is to allow the buffer to have a variable length which can be determined at runtime.

To do so, I changed the type of the base _PyExecutorObject to a variable object, and used the PyObject_NewVar function to initialize the object instead.

After using gdb to trace the usage of the _PyUOpExecutorObject, I did not find any use which needed to be modified to work with variable objects, and so I did not make any additional changes.

This is my first time contributing, so please let me know if I made a mistake anywhere!

Issue: Make uop executor objects variable length #106608

bedevere-bot · 2023-08-01T09:32:40Z

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

ghost · 2023-08-01T09:33:00Z

All commit authors signed the Contributor License Agreement.

rdrf2838 · 2023-08-01T11:02:15Z

Python/optimizer.c

@@ -698,7 +698,7 @@ uop_optimize(
        return trace_length;
    }
    OBJECT_STAT_INC(optimization_traces_created);
-    _PyUOpExecutorObject *executor = PyObject_New(_PyUOpExecutorObject, &UOpExecutor_Type);
+    _PyUOpExecutorObject *executor = PyObject_NewVar(_PyUOpExecutorObject, &UOpExecutor_Type, trace_length+1);


The trace_length+1 value is used, as line 708 adds a sentinel uop which causes the number of instructions to be trace_length+1.

Could you please add a comment here so that we don't forget, or to help others understand?

Then again, I think the allocator will allocate an extra item anyways because of

.tp_basicsize = sizeof(_PyUOpExecutorObject),

If you compare to PyTuple_Type, it uses this:

sizeof(PyTupleObject) - sizeof(PyObject *),

so that the basic size excludes the size of the dummy array in the PyTupleObject type:

PyObject *ob_item[1];

I do like that we explicitly have +1 here to make it clear that we intend to use the sentinel value.

However, now that the executor is variable-length and always has room for the sentinel, I think that on line 708 below you need to set the sentinel unconditionally, i.e. remove the condition on line 707.

Please also update uop_len to just return ob_size instead of counting until the sentinel. (This also means we could just get rid of the sentinel -- I think it only exists for uop_size, and it's never reachable by the interpreter.)

Fidget-Spinner

Thanks!

markshannon

Useful improvement

Include/cpython/optimizer.h

gvanrossum

(I'm only requesting changes to counteract @Fidget-Spinner's premature approval.)

Misc/NEWS.d/next/Core and Builtins/2023-08-01-09-41-36.gh-issue-106608.OFZogw.rst

Include/cpython/optimizer.h

gvanrossum · 2023-08-03T18:11:53Z

Python/optimizer.c

@@ -698,7 +698,7 @@ uop_optimize(
        return trace_length;
    }
    OBJECT_STAT_INC(optimization_traces_created);
-    _PyUOpExecutorObject *executor = PyObject_New(_PyUOpExecutorObject, &UOpExecutor_Type);
+    _PyUOpExecutorObject *executor = PyObject_NewVar(_PyUOpExecutorObject, &UOpExecutor_Type, trace_length+1);


Then again, I think the allocator will allocate an extra item anyways because of

.tp_basicsize = sizeof(_PyUOpExecutorObject),

If you compare to PyTuple_Type, it uses this:

sizeof(PyTupleObject) - sizeof(PyObject *),

so that the basic size excludes the size of the dummy array in the PyTupleObject type:

PyObject *ob_item[1];

I do like that we explicitly have +1 here to make it clear that we intend to use the sentinel value.

However, now that the executor is variable-length and always has room for the sentinel, I think that on line 708 below you need to set the sentinel unconditionally, i.e. remove the condition on line 707.

Please also update uop_len to just return ob_size instead of counting until the sentinel. (This also means we could just get rid of the sentinel -- I think it only exists for uop_size, and it's never reachable by the interpreter.)

bedevere-bot · 2023-08-03T18:12:41Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

…e-106608.OFZogw.rst Co-authored-by: Guido van Rossum <[email protected]>

rdrf2838 · 2023-08-04T04:02:14Z

In response to Guido's comments, I have made the following changes:

I have changed .tp_basicsize so that no excess memory is allocated.
I verified that the sentinel value is used only in uop_len.
Following 2., I removed the sentinel value and used Py_SIZE to access the object's length instead.
I verified that the changes work by checking that the tests do call the uop_len function, such as in

cpython/Lib/test/test_capi/test_misc.py

Line 2461 in ee78d01

uops = {opname for opname, _, _ in ex}

I have made the requested changes; please review again

bedevere-bot · 2023-08-04T04:02:18Z

Thanks for making the requested changes!

@gvanrossum, @Fidget-Spinner: please review the changes made to this pull request.

Fidget-Spinner · 2023-08-04T06:21:45Z

Woops yeah I overlooked what Guido mentioned. The new PR LGTM though (FWIW :).

gvanrossum

Peachy perfect. Thanks! I'll merge now.

gvanrossum · 2023-08-05T04:11:24Z

There might be buildbot failures. Don't worry I will handle those if necessary (but we've had a lot of false positives there lately).

make uop trace variable length

5d5a558

bedevere-bot mentioned this pull request Jul 28, 2023

Make uop executor objects variable length #106608

Closed

bedevere-bot added the awaiting review label Aug 1, 2023

📜🤖 Added by blurb_it.

c9b9985

rdrf2838 commented Aug 1, 2023

View reviewed changes

Fidget-Spinner approved these changes Aug 3, 2023

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels Aug 3, 2023

Fidget-Spinner requested review from gvanrossum, markshannon and brandtbucher August 3, 2023 12:19

markshannon reviewed Aug 3, 2023

View reviewed changes

Include/cpython/optimizer.h Show resolved Hide resolved

gvanrossum requested changes Aug 3, 2023

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting merge labels Aug 3, 2023

rdrf2838 and others added 3 commits August 4, 2023 10:17

remove unnneeded memory allocation due to var obj

489bded

use var obj's macros for uop_len

ad68578

Update Misc/NEWS.d/next/Core and Builtins/2023-08-01-09-41-36.gh-issu…

86676e4

…e-106608.OFZogw.rst Co-authored-by: Guido van Rossum <[email protected]>

bedevere-bot added awaiting change review and removed awaiting changes labels Aug 4, 2023

bedevere-bot requested review from Fidget-Spinner and gvanrossum August 4, 2023 04:02

gvanrossum approved these changes Aug 5, 2023

View reviewed changes

bedevere-bot removed the awaiting change review label Aug 5, 2023

bedevere-bot added the awaiting merge label Aug 5, 2023

gvanrossum merged commit 4e6fac7 into python:main Aug 5, 2023

bedevere-bot removed the awaiting merge label Aug 5, 2023

rdrf2838 deleted the gh-106608 branch August 5, 2023 23:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-106608: make uop trace variable length #107531

gh-106608: make uop trace variable length #107531

rdrf2838 commented Aug 1, 2023 •

edited by bedevere-bot

Loading

bedevere-bot commented Aug 1, 2023

ghost commented Aug 1, 2023 •

edited by ghost

Loading

rdrf2838 Aug 1, 2023 •

edited

Loading

Fidget-Spinner Aug 3, 2023

gvanrossum Aug 3, 2023

Fidget-Spinner left a comment

markshannon left a comment

gvanrossum left a comment

gvanrossum Aug 3, 2023

bedevere-bot commented Aug 3, 2023

rdrf2838 commented Aug 4, 2023

bedevere-bot commented Aug 4, 2023

Fidget-Spinner commented Aug 4, 2023

gvanrossum left a comment

gvanrossum commented Aug 5, 2023

gh-106608: make uop trace variable length #107531

gh-106608: make uop trace variable length #107531

Conversation

rdrf2838 commented Aug 1, 2023 • edited by bedevere-bot Loading

bedevere-bot commented Aug 1, 2023

ghost commented Aug 1, 2023 • edited by ghost Loading

rdrf2838 Aug 1, 2023 • edited Loading

Choose a reason for hiding this comment

Fidget-Spinner Aug 3, 2023

Choose a reason for hiding this comment

gvanrossum Aug 3, 2023

Choose a reason for hiding this comment

Fidget-Spinner left a comment

Choose a reason for hiding this comment

markshannon left a comment

Choose a reason for hiding this comment

gvanrossum left a comment

Choose a reason for hiding this comment

gvanrossum Aug 3, 2023

Choose a reason for hiding this comment

bedevere-bot commented Aug 3, 2023

rdrf2838 commented Aug 4, 2023

bedevere-bot commented Aug 4, 2023

Fidget-Spinner commented Aug 4, 2023

gvanrossum left a comment

Choose a reason for hiding this comment

gvanrossum commented Aug 5, 2023

rdrf2838 commented Aug 1, 2023 •

edited by bedevere-bot

Loading

ghost commented Aug 1, 2023 •

edited by ghost

Loading

rdrf2838 Aug 1, 2023 •

edited

Loading