GeezerVison

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.

CI looks good so: I have made the requested changes; please review again :)

Thanks for making the requested changes!

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

I think the tests could be more robust if there was a way to run the executors without optimizing the uops. Each test could then first use assertIn to ensure the relevant uops are in fact present and only then run with optimizations enabled. Though dynamically turning off optimizations would require some changes in the interpreter so I'm not sure if it's worth it.

You could do this if you want. Mark wanted to do this, but no one has time to implement it proper.

The steps to do this would be the following:

1. Export some C API for optimize_uops in _textinternalcapi.c or something.
2. The API should take a bytecode offset/slice to start trace projection from.
3. Write our optimizer tests to just use that.

Ok I'll give it a try! Seems like a nice project for the weekend (or for PyCon if I can't get it working by then 😄)

Thanks for writing down the individual steps for me, it's super helpful :)

I think the tests could be more robust if there was a way to run the executors without optimizing the uops. Each test could then first use assertIn to ensure the relevant uops are in fact present and only then run with optimizations enabled. Though dynamically turning off optimizations would require some changes in the interpreter so I'm not sure if it's worth it.

You could do this if you want. Mark wanted to do this, but no one has time to implement it proper.

The steps to do this would be the following:

1. Export some C API for `optimize_uops` in `_textinternalcapi.c` or something.

2. The API should take a bytecode offset/slice to start trace projection from.

3. Write our optimizer tests to just use that.

@Fidget-Spinner, I noticed that the optimizer can already be turned off by setting PYTHON_UOPS_OPTIMIZE to '0':

Rather than exposing a new internal api, I think we could simply toggle this variable in the tests. I'm not sure if this is what Mark intended as this would still test more than just the optimizer proper. Though I like that these tests are more end-to-end.

Anyway, here's what it'd look like. Let me know if this is something we want to pursue, otherwise I'll go back to thinking how to expose just the optimizer :)

diff --git a/Lib/test/test_capi/test_opt.py b/Lib/test/test_capi/test_opt.py
index 651148336f7..b82b36fa2f5 100644
--- a/Lib/test/test_capi/test_opt.py
+++ b/Lib/test/test_capi/test_opt.py
@@ -11,6 +11,7 @@
 from test.support import (script_helper, requires_specialization,
                           import_helper, Py_GIL_DISABLED, requires_jit_enabled,
                           reset_code)
+from test.support.os_helper import EnvironmentVarGuard
 
 _testinternalcapi = import_helper.import_module("_testinternalcapi")
 
@@ -458,6 +459,12 @@ def _run_with_optimizer(self, testfunc, arg):
         ex = get_first_executor(testfunc)
         return res, ex
 
+    def _run_without_optimizer(self, testfunc, arg):
+        with EnvironmentVarGuard() as env:
+            env["PYTHON_UOPS_OPTIMIZE"] = "0"
+            res = testfunc(arg)
+        ex = get_first_executor(testfunc)
+        return res, ex
 
     def test_int_type_propagation(self):
         def testfunc(loops):
@@ -1951,6 +1958,17 @@ def testfunc(n):
                     x += 1
             return x
 
+        res, ex = self._run_without_optimizer(testfunc, TIER2_THRESHOLD)
+        self.assertEqual(res, TIER2_THRESHOLD)
+        self.assertIsNotNone(ex)
+        uops = get_opnames(ex)
+        self.assertIn("_CALL_ISINSTANCE", uops)
+        self.assertIn("_GUARD_THIRD_NULL", uops)
+        self.assertIn("_GUARD_CALLABLE_ISINSTANCE", uops)
+
+        # Invalidate the executor to force a reoptimization
+        _testinternalcapi.invalidate_executors(testfunc.__code__)
+
         res, ex = self._run_with_optimizer(testfunc, TIER2_THRESHOLD)
         self.assertEqual(res, TIER2_THRESHOLD)
         self.assertIsNotNone(ex)

@tomasr8 yeah I'm not advocating for removing the end-to-end tests altogether. Rather, the optimizer tests should have a mix of both end-to-end and unit.

For example, it would be nice if we could generate an optimized trace without having to even wrap it in a for loop and run till JIT threshold. This actually makes the tests really slow because JIT threshold is quite high right now.

Got it! Yeah, being able to simplify the tests and make them run faster is definitely worth it :)