gh-116168: Remove extra `_CHECK_STACK_SPACE` uops #117242

lazorchakp · 2024-03-26T01:18:41Z

This PR reduces the number of _CHECK_STACK_SPACE operands in tier2 bytecode.

Here's the basic design:

Introduce a new tier2-only uop, _CHECK_STACK_SPACE_OPERAND, which contains the amount space to check in its operand.
during uops optimization, calculate how much stack space we will need for the whole trace. Combine all _CHECK_STACK_SPACEs into a single _CHECK_STACK_SPACE_OPERAND.

potential changes / refactors to consider:

Merge combine_stack_space_checks with peephole_opt or remove_unneeded_uops?
All the #ifdef Py_DEBUGs in the core logic here are kind of distracting. Not sure if there's a more preferable way to implement these safety checks.

I'm wide open to feedback / criticism / suggestions!

Issue: Tier2 peephole optimization: remove extraneous _CHECK_STACK_SPACE ops #116168

…ove-extra-_check_stack_space-ops

Fidget-Spinner

Thanks for the PR! Just one comment for now.

Python/optimizer.c

…ove-extra-_check_stack_space-ops

lazorchakp · 2024-03-28T04:43:22Z

Not sure if this needs news, but I'm happy to add it if necessary. The failing test is a timeout that seems to be occurring on more than just my branch

Fidget-Spinner

This is awesome work! I just have a few comments about the tests.

Fidget-Spinner · 2024-03-28T08:46:53Z

Modules/_testinternalcapi.c

@@ -957,6 +957,16 @@ iframe_getlasti(PyObject *self, PyObject *frame)
    return PyLong_FromLong(PyUnstable_InterpreterFrame_GetLasti(f));
 }

+static PyObject *
+get_co_framesize(PyObject *self, PyObject *arg) {


I really like how comprehensive this test is. But I don't think we shouldn't test for such implementation details. We should just test the bytecode trace is correct and that's the limit.

Also, _CHECK_STACK_SPACE shouldn't affect frame size. It's a check after all.

I'm going to remove the assertions for exact framesizes, but I'd still like to keep this function. It's useful to help determine that _CHECK_STACK_SPACE_OPERAND is assigned the correct operand.

To be clear, I'm not worried about my changes affecting the framesizes themselves, just that the calculated total stack space required is accurate.

Fidget-Spinner · 2024-03-28T08:50:43Z

Python/optimizer_analysis.c

+            }
+        }
+    }
+    Py_FatalError("No terminating instruction");


Py_UNREACHABLE alone is enough usually.

Suggested change

Py_FatalError("No terminating instruction");

markshannon

Thanks for doing this.

The approach looks sound, and the tests thorough.
I have a few suggestions, specifically that the pass by added to optimize_uops to avoid adding another pass.

markshannon · 2024-03-28T09:07:38Z

Lib/test/test_capi/test_opt.py

+    def _assert_framesize_cross_platform(self, fn_obj, expected_framesize):
+        self.assertEqual(
+            _testinternalcapi.get_co_framesize(fn_obj.__code__),
+            expected_framesize + self.FRAMESIZE_ADJUSTMENT


It isn't clear why this adjustment is necessary.
Also, would it be cleaner to do the adjustment in _testinternalcapi.get_co_framesize?

It isn't clear to me either why framesizes vary across platforms 😅 but based on other comments I'm thinking that this assertion is overly specific and a bit brittle, so I'm just going to remove it. I don't think I want to adjust get_co_framesize, since I need this to report the actual framesize in every case

markshannon · 2024-03-28T09:07:47Z

Python/optimizer_analysis.c

+    return co;
+}
+
+// _PUSH_FRAME is 3 ops in front of _CHECK_STACK_SPACE


This seems fragile, and I don't think you need it if you ~~track the current stack depth~~ record the location of the previous _CHECK_STACK_SPACE and patch it in _PUSH_FRAME.

markshannon · 2024-03-28T09:07:52Z

Python/optimizer_analysis.c

+ * calls in this trace.
+ */
+static void
+combine_stack_space_checks(


Rather than make this a separate pass, could you integrate into optimize_uops by adding it to optimizer_bytecodes.c.

Happy to do this, but to confirm - do you definitely prefer integration into optimize_uops over peephole_opt or remove_unneeded_uops? Any of these would prevent an extra pass.

@Fidget-Spinner may also have an opinion here

Both peephole_opt or optimize_uops is fine to me. peephole_opt would likely be clearer as this pass does not use any information from the abstract interpreter.

markshannon · 2024-03-28T09:07:54Z

Python/optimizer_analysis.c

+    bool must_exit = false;
+#endif
+    int depth = 0;
+    uint32_t space_at_depth[MAX_ABSTRACT_FRAME_DEPTH];


You only need to track the current depth, as this is a single pass algorithm.
In fact, this may be incorrect, if more than one function is called with differing stack requirements.

I think, only the following variables are needed:

The first check (address or offset)

The current stack size

The maximum requested stack size

Plus the last seen stack check, to avoid hard-coding the offset between CHECK_STACK_SPACE and _PUSH_FRAME.

Thanks for the suggestion - I was able to remove my makeshift stack. I had to use one additional variable to keep track of the current frame's size, since there's no way I could see to access this from _POP_FRAME (we only have the code object being returned to, not the one we're returning from)

markshannon · 2024-03-28T09:07:58Z

Python/optimizer_analysis.c

+                if (first_valid_check_stack != NULL) {
+                    assert(first_valid_check_stack->opcode == _CHECK_STACK_SPACE);
+                    assert(max_space > 0);
+                    assert(max_space < INT_MAX);


Suggested change

assert(max_space < INT_MAX);

if (max_space < INT_MAX) {

Maybe reduce the limit to UNIT16_MAX?

Ah, setting a valid _CHECK_STACK_SPACE_OPERAND is mandatory if even a single _CHECK_STACK_SPACE is deleted during this pass, so I can't make this conditional on the value of max_space. I'm going to add some additional checks to ensure max_space is always an acceptable value and rework the exit logic

bedevere-app · 2024-03-28T09:10:28Z

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.

…des.c

…ove-extra-_check_stack_space-ops

lazorchakp · 2024-03-30T01:55:19Z

I have made the requested changes; please review again

bedevere-app · 2024-03-30T01:55:24Z

Thanks for making the requested changes!

@markshannon: please review the changes made to this pull request.

Fidget-Spinner · 2024-04-01T16:06:45Z

I think this is fine to merge now. Will wait a few days if Mark has any reviews left.

gvanrossum · 2024-04-01T17:30:45Z

I am working on one last review. Looks like just some style nits.

gvanrossum

This looks great. I do have a few style nits. Once you fix those I will gladly merge it!

gvanrossum · 2024-04-01T15:20:00Z

Python/optimizer_analysis.c

+/* _PUSH_FRAME/_POP_FRAME's operand can be 0, a PyFunctionObject *, or a
+ * PyCodeObject *. Retrieve the code object if possible.
+ */
+static PyCodeObject *get_co(_PyUOpInstruction *op) {


Style nit:

Suggested change

static PyCodeObject *get_co(_PyUOpInstruction *op) {

static PyCodeObject *

get_co(_PyUOpInstruction *op)

{

gvanrossum · 2024-04-01T15:35:36Z

Lib/test/test_capi/test_opt.py

@@ -4,6 +4,7 @@
 import unittest
 import gc
 import os
+import platform


This seems unused?

gvanrossum · 2024-04-01T15:48:03Z

Lib/test/test_capi/test_opt.py

+        self.assertEqual(res, 832)
+        self.assertIsNotNone(ex)
+
+        uops_and_operands = [(opcode, operand) for opcode, _, _, operand in list(ex)]


The list() call is superfluous:

Suggested change

uops_and_operands = [(opcode, operand) for opcode, _, _, operand in list(ex)]

uops_and_operands = [(opcode, operand) for opcode, _, _, operand in ex]

(Ditto below.)

gvanrossum · 2024-04-01T15:51:10Z

Lib/test/test_capi/test_opt.py

+                c, d = dummy13(9)
+                a += b + c + d
+            return a
+


One blank line is enough. :-)

(Ditto below.)

gvanrossum · 2024-04-01T15:56:59Z

Lib/test/test_capi/test_opt.py

+        self.assertEqual(uop_names.count("_PUSH_FRAME"), 2)
+        self.assertEqual(uop_names.count("_CHECK_STACK_SPACE_OPERAND"), 1)
+
+        # this hits a different case during trace projection in refcount test runs only,


Now you've made me curious -- what does the refcount test do different?

@gvanrossum I'm still not sure why this is happening, but here's the behavior:

On the first round of a refcount test, trace projection appears to proceed normally. We end up with a large trace that's cut off part of the way through the uops for dummy_large. On all subsequent rounds, we end up in the new_code == NULL case while emitting _PUSH_FRAME in trace projection (code).

That prevents me from having access to a code object in peephole optimization, so I can't remove the corresponding _CHECK_STACK_SPACE.

Interestingly, we never hit new_code == NULL in testing with --forever.

Probably refcount tests do other weird stuff and maybe somehow the func/code version cache entry is overwritten? Let's not worry about it, refcount tests are always weird.

gvanrossum · 2024-04-01T16:59:06Z

Modules/_testinternalcapi.c

@@ -957,6 +957,16 @@ iframe_getlasti(PyObject *self, PyObject *frame)
    return PyLong_FromLong(PyUnstable_InterpreterFrame_GetLasti(f));
 }

+static PyObject *
+get_co_framesize(PyObject *self, PyObject *arg) {


Style nit

Suggested change

get_co_framesize(PyObject *self, PyObject *arg) {

get_co_framesize(PyObject *self, PyObject *arg)

{

gvanrossum · 2024-04-01T17:39:06Z

Python/optimizer_analysis.c

+                int framesize = co->co_framesize;
+                assert(framesize > 0);
+                curr_space += framesize;
+                if (curr_space < 0 || curr_space > INT32_MAX) {


The second check cannot succeed when int is 32 bits, right? I guess the compiler will then omit the check.

right. I was worried about cases where int is larger than the 32-bit operand. I'm comparing to INT32_MAX instead of UINT32_MAX to avoid compiler warnings; technically that makes this comparison 1 bit more strict than necessary, but I imagine this is an edge case that will almost never be hit anyway.

🤔 actually, the comparison isn't overly strict, since I'm already requiring this number to be a positive int. Anyway, I'm still convinced this is a necessary check.

gvanrossum · 2024-04-01T17:42:43Z

Python/optimizer_analysis.c

+                assert(framesize > 0);
+                curr_space += framesize;
+                if (curr_space < 0 || curr_space > INT32_MAX) {
+                    // overflow or won't fit in operand


It will fit in operand (which is always 64 bits) but not in the cache entry for the new instruction (which is always unsigned 32 bits) or won't be able to be converted to signed int (which it must be for the _PyThreadState_HasStackSpace() call). So the check is right but the comment feels like it is misrepresenting the situation. My suggestion:

Suggested change

// overflow or won't fit in operand

// won't fit in signed 32-bit int

…ove-extra-_check_stack_space-ops

lazorchakp · 2024-04-02T02:32:26Z

I've addressed all comments and the pipeline looks good, so this is once again ready for review.

gvanrossum · 2024-04-02T00:36:05Z

Lib/test/test_capi/test_opt.py

+        #     a99999 = a99998 + 1
+        #     return a99999


Probably more this, since repetitions is 10000:

Suggested change

# a99999 = a99998 + 1

# return a99999

# a9999 = a9998 + 1

# return a9999

gvanrossum · 2024-04-03T16:14:59Z

Can you run make regen-case? Then we'll merge.

…ove-extra-_check_stack_space-ops

lazorchakp · 2024-04-03T16:42:37Z

👍 just ran make regen-cases with the latest updates from main

lazorchakp · 2024-04-03T17:22:13Z

Thank you all for your patience and guidance on this work! I learned a lot throughout the process and quite enjoyed it.

gvanrossum · 2024-04-03T17:42:53Z

Thank you! Looking forward to more contributions from you.

This merges all `_CHECK_STACK_SPACE` uops in a trace into a single `_CHECK_STACK_SPACE_OPERAND` uop that checks whether there is enough stack space for all calls included in the entire trace.

pythongh-116168: Remove extra _CHECK_STACK_SPACE uops

548fb94

bedevere-app bot mentioned this pull request Mar 26, 2024

Tier2 peephole optimization: remove extraneous _CHECK_STACK_SPACE ops #116168

Closed

lazorchakp added 3 commits March 25, 2024 18:19

Merge remote-tracking branch 'upstream/main' into pythongh-116168-rem…

1a7cb94

…ove-extra-_check_stack_space-ops

Assorted cleanup and pipeline fix

5eca7ca

Merge remote-tracking branch 'upstream/main' into pythongh-116168-rem…

76e540a

…ove-extra-_check_stack_space-ops

Fidget-Spinner reviewed Mar 26, 2024

View reviewed changes

Python/optimizer.c Outdated Show resolved Hide resolved

lazorchakp added 6 commits March 26, 2024 15:15

Fix loss of data warning and revert changes to optimizer.c

1b1d8e1

Merge remote-tracking branch 'upstream/main' into pythongh-116168-rem…

781590d

…ove-extra-_check_stack_space-ops

Move all _CHECK_STACK_SPACE consolidation to optimizer_analysis.c

617e4f3

Merge remote-tracking branch 'upstream/main' into pythongh-116168-rem…

49a1fa9

…ove-extra-_check_stack_space-ops

Use get_co in peephole_opt

95ef823

clean up diff

defdc2b

lazorchakp marked this pull request as ready for review March 28, 2024 04:42

lazorchakp requested review from markshannon and gvanrossum as code owners March 28, 2024 04:42

bedevere-app bot added the awaiting review label Mar 28, 2024

lazorchakp requested a review from Fidget-Spinner March 28, 2024 04:43

Fidget-Spinner added the skip news label Mar 28, 2024

Fidget-Spinner reviewed Mar 28, 2024

View reviewed changes

markshannon requested changes Mar 28, 2024

View reviewed changes

bedevere-app bot added awaiting changes and removed awaiting review labels Mar 28, 2024

lazorchakp added 5 commits March 28, 2024 20:41

Update with PR feedback, still need to move logic to optimizer_byteco…

df0d589

…des.c

Merge remote-tracking branch 'upstream/main' into pythongh-116168-rem…

83c57a2

…ove-extra-_check_stack_space-ops

Merge remote-tracking branch 'upstream/main' into pythongh-116168-rem…

3f7ba5b

…ove-extra-_check_stack_space-ops

Make combine_stack_space_checks as similar to peephole_opt as possible

c12119f

Integrate combine_stack_space_checks with peephole_opt

f0b2133

bedevere-app bot added awaiting change review and removed awaiting changes labels Mar 30, 2024

bedevere-app bot requested a review from markshannon March 30, 2024 01:55

gvanrossum reviewed Apr 1, 2024

View reviewed changes

lazorchakp added 3 commits April 1, 2024 16:05

Address style nits

486ece6

Merge remote-tracking branch 'upstream/main' into pythongh-116168-rem…

5b797e8

…ove-extra-_check_stack_space-ops

Undo accidental newline removal

64fac38

gvanrossum approved these changes Apr 3, 2024

View reviewed changes

bedevere-app bot added awaiting merge and removed awaiting change review labels Apr 3, 2024

Fidget-Spinner approved these changes Apr 3, 2024

View reviewed changes

Merge remote-tracking branch 'upstream/main' into pythongh-116168-rem…

77e94cb

…ove-extra-_check_stack_space-ops

Fix variables names in large-framesize test comment

7b62c2b

gvanrossum enabled auto-merge (squash) April 3, 2024 16:44

gvanrossum merged commit 1c43468 into python:main Apr 3, 2024
52 checks passed

bedevere-app bot removed the awaiting merge label Apr 3, 2024

lazorchakp deleted the gh-116168-remove-extra-_check_stack_space-ops branch April 3, 2024 17:14

	uops_and_operands = [(opcode, operand) for opcode, _, _, operand in list(ex)]
	uops_and_operands = [(opcode, operand) for opcode, _, _, operand in ex]

	get_co_framesize(PyObject self, PyObject arg) {
	get_co_framesize(PyObject self, PyObject arg)
	{

	// overflow or won't fit in operand
	// won't fit in signed 32-bit int

gh-116168: Remove extra _CHECK_STACK_SPACE uops #117242

gh-116168: Remove extra _CHECK_STACK_SPACE uops #117242

Conversation

lazorchakp commented Mar 26, 2024 • edited Loading

Fidget-Spinner left a comment

Choose a reason for hiding this comment

lazorchakp commented Mar 28, 2024

Fidget-Spinner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markshannon left a comment

Choose a reason for hiding this comment

markshannon Mar 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markshannon Mar 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-app bot commented Mar 28, 2024

lazorchakp commented Mar 30, 2024

bedevere-app bot commented Mar 30, 2024

Fidget-Spinner commented Apr 1, 2024

gvanrossum commented Apr 1, 2024

gvanrossum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lazorchakp commented Apr 2, 2024

Choose a reason for hiding this comment

gvanrossum commented Apr 3, 2024

lazorchakp commented Apr 3, 2024

lazorchakp commented Apr 3, 2024

gvanrossum commented Apr 3, 2024

gh-116168: Remove extra `_CHECK_STACK_SPACE` uops #117242

gh-116168: Remove extra `_CHECK_STACK_SPACE` uops #117242

lazorchakp commented Mar 26, 2024 •

edited

Loading

markshannon Mar 28, 2024 •

edited

Loading

markshannon Mar 28, 2024 •

edited

Loading