Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh-115859: Re-enable T2 optimizer pass by default #116062

Merged
merged 9 commits into from
Feb 28, 2024

Conversation

gvanrossum
Copy link
Member

@gvanrossum gvanrossum commented Feb 28, 2024

This undoes the temporary default disabling of the T2 optimizer pass in gh-115860.

  • Add a new test that reproduces Brandt's example from Type propagation: just because something is const doesn't mean it automatically matches the type #115859; it indeed crashes before gh-115816: Improve internal symbols API in optimizer #116028 with PYTHONUOPSOPTIMIZE=1
  • Re-enable the optimizer pass in T2, stop checking PYTHONUOPSOPTIMIZE
  • Rename the env var to disable T2 entirely to PYTHON_UOPS_OPTIMIZE (must be explicitly set to 0 to disable)
  • Fix skipIf conditions on tests in test_opt.py accordingly
  • Export sym_is_bottom() (for debugging)
  • Fix various things in the _BINARY_OP_ specializations in the abstract interpreter:
    • DECREF(temp)
    • out-of-space check after sym_new_const()
    • add sym_matches_type() checks, so even if we somehow reach a binary op with symbolic constants of the wrong type on the stack we won't trigger the type assert

@Fidget-Spinner Please review when you get up. :-)

Fixes gh-115859.

@gvanrossum
Copy link
Member Author

Once tests pass I will try to remember to run the leaks buildbot.

@gvanrossum gvanrossum changed the title gh-115859: Restore optimizer gh-115859: Re-enable T2 optimizer pass by default Feb 28, 2024
Copy link
Member

@Fidget-Spinner Fidget-Spinner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a comment on refcounts, everything else looks good to me.

@@ -118,14 +121,17 @@ dummy_func(void) {
OUT_OF_SPACE_IF_NULL(res = sym_new_const(ctx, temp));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't you need to decref it here too on this error branch? Or am I remembering the ownership wrongly? Same for everything below.

Might be better to write it as:

res = sym_new_const(ctx, temp);
Py_DECREF(temp);
OUT_OF_SPACE_IF_NULL(res);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!

@gvanrossum
Copy link
Member Author

gvanrossum commented Feb 28, 2024

There seems to be wrong with test_external_inspection.py on all the JIT / aarch64-unknown-linux CI runs; from one of the error logs:

Re-running test_external_inspection in verbose mode (matching: test_remote_stack_trace, test_self_trace)
test_remote_stack_trace (test.test_external_inspection.TestGetStackTrace.test_remote_stack_trace) ... ERROR
test_self_trace (test.test_external_inspection.TestGetStackTrace.test_self_trace) ... ERROR

======================================================================
ERROR: test_remote_stack_trace (test.test_external_inspection.TestGetStackTrace.test_remote_stack_trace)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/runner/work/cpython/cpython/Lib/test/test_external_inspection.py", line 59, in test_remote_stack_trace
    stack_trace = get_stack_trace(p.pid)
                  ~~~~~~~~~~~~~~~^^^^^^^
OSError: [Errno 38] Function not implemented

======================================================================
ERROR: test_self_trace (test.test_external_inspection.TestGetStackTrace.test_self_trace)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/runner/work/cpython/cpython/Lib/test/test_external_inspection.py", line 80, in test_self_trace
    stack_trace = get_stack_trace(os.getpid())
                  ~~~~~~~~~~~~~~~^^^^^^^^^^^^^
OSError: [Errno 38] Function not implemented

----------------------------------------------------------------------
Ran 2 tests in 0.342s

FAILED (errors=2)
test test_external_inspection failed
1 test failed again:
    test_external_inspection

@gvanrossum gvanrossum added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Feb 28, 2024
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @gvanrossum for commit bbe8cd9 🤖

If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again.

@gvanrossum
Copy link
Member Author

gvanrossum commented Feb 28, 2024

Argh, test_type_inconsistency fails when run multiple times. Fix coming up. EDIT: In a separate PR (#116079).

@gvanrossum gvanrossum merged commit 3409bc2 into python:main Feb 28, 2024
48 of 59 checks passed
@gvanrossum gvanrossum deleted the restore-optimizer branch February 28, 2024 22:38
@bedevere-bot
Copy link

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot AMD64 Ubuntu NoGIL Refleaks 3.x has failed when building commit 3409bc2.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/1226/builds/1341) and take a look at the build logs.
  4. Check if the failure is related to this commit (3409bc2) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/1226/builds/1341

Failed tests:

  • test_capi

Failed subtests:

  • test_type_inconsistency - test.test_capi.test_opt.TestUopsOptimization.test_type_inconsistency

Test leaking resources:

  • test_processes: memory blocks

Summary of the results of the build (if available):

==

Click to see traceback logs
Traceback (most recent call last):
  File "/home/ubuntu/buildarea/3.x.itamaro-ubuntu-aws.refleak.nogil/build/Lib/test/test_capi/test_opt.py", line 903, in test_type_inconsistency
    self.assertIsNone(ex)
    ~~~~~~~~~~~~~~~~~^^^^
AssertionError: <uop_executor object at 0x200006b2d90> is not None


Traceback (most recent call last):
  File "/home/ubuntu/buildarea/3.x.itamaro-ubuntu-aws.refleak.nogil/build/Lib/test/test_capi/test_opt.py", line 903, in test_type_inconsistency
    self.assertIsNone(ex)
    ~~~~~~~~~~~~~~~~~^^^^
AssertionError: <uop_executor object at 0x200006b1dd0> is not None

@bedevere-bot
Copy link

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot ARM64 MacOS M1 Refleaks NoGIL 3.x has failed when building commit 3409bc2.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/1368/builds/352) and take a look at the build logs.
  4. Check if the failure is related to this commit (3409bc2) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/1368/builds/352

Failed tests:

  • test_capi

Failed subtests:

  • test_type_inconsistency - test.test_capi.test_opt.TestUopsOptimization.test_type_inconsistency

Summary of the results of the build (if available):

==

Click to see traceback logs
Traceback (most recent call last):
  File "/Users/ec2-user/buildbot/buildarea/3.x.itamaro-macos-arm64-aws.macos-with-brew.refleak.nogil/build/Lib/test/test_capi/test_opt.py", line 903, in test_type_inconsistency
    self.assertIsNone(ex)
    ~~~~~~~~~~~~~~~~~^^^^
AssertionError: <uop_executor object at 0x200006b2d90> is not None


Traceback (most recent call last):
  File "/Users/ec2-user/buildbot/buildarea/3.x.itamaro-macos-arm64-aws.macos-with-brew.refleak.nogil/build/Lib/test/test_capi/test_opt.py", line 903, in test_type_inconsistency
    self.assertIsNone(ex)
    ~~~~~~~~~~~~~~~~~^^^^
AssertionError: <uop_executor object at 0x200006b1190> is not None

gvanrossum added a commit that referenced this pull request Feb 28, 2024
woodruffw pushed a commit to woodruffw-forks/cpython that referenced this pull request Mar 4, 2024
This undoes the *temporary* default disabling of the T2 optimizer pass in pythongh-115860.

- Add a new test that reproduces Brandt's example from pythongh-115859; it indeed crashes before pythongh-116028 with PYTHONUOPSOPTIMIZE=1
- Re-enable the optimizer pass in T2, stop checking PYTHONUOPSOPTIMIZE
- Rename the env var to disable T2 entirely to PYTHON_UOPS_OPTIMIZE (must be explicitly set to 0 to disable)
- Fix skipIf conditions on tests in test_opt.py accordingly
- Export sym_is_bottom() (for debugging)
- Fix various things in the `_BINARY_OP_` specializations in the abstract interpreter:
  - DECREF(temp)
  - out-of-space check after sym_new_const()
  - add sym_matches_type() checks, so even if we somehow reach a binary op with symbolic constants of the wrong type on the stack we won't trigger the type assert
woodruffw pushed a commit to woodruffw-forks/cpython that referenced this pull request Mar 4, 2024
adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 25, 2024
This undoes the *temporary* default disabling of the T2 optimizer pass in pythongh-115860.

- Add a new test that reproduces Brandt's example from pythongh-115859; it indeed crashes before pythongh-116028 with PYTHONUOPSOPTIMIZE=1
- Re-enable the optimizer pass in T2, stop checking PYTHONUOPSOPTIMIZE
- Rename the env var to disable T2 entirely to PYTHON_UOPS_OPTIMIZE (must be explicitly set to 0 to disable)
- Fix skipIf conditions on tests in test_opt.py accordingly
- Export sym_is_bottom() (for debugging)
- Fix various things in the `_BINARY_OP_` specializations in the abstract interpreter:
  - DECREF(temp)
  - out-of-space check after sym_new_const()
  - add sym_matches_type() checks, so even if we somehow reach a binary op with symbolic constants of the wrong type on the stack we won't trigger the type assert
adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 25, 2024
diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 17, 2024
This undoes the *temporary* default disabling of the T2 optimizer pass in pythongh-115860.

- Add a new test that reproduces Brandt's example from pythongh-115859; it indeed crashes before pythongh-116028 with PYTHONUOPSOPTIMIZE=1
- Re-enable the optimizer pass in T2, stop checking PYTHONUOPSOPTIMIZE
- Rename the env var to disable T2 entirely to PYTHON_UOPS_OPTIMIZE (must be explicitly set to 0 to disable)
- Fix skipIf conditions on tests in test_opt.py accordingly
- Export sym_is_bottom() (for debugging)
- Fix various things in the `_BINARY_OP_` specializations in the abstract interpreter:
  - DECREF(temp)
  - out-of-space check after sym_new_const()
  - add sym_matches_type() checks, so even if we somehow reach a binary op with symbolic constants of the wrong type on the stack we won't trigger the type assert
diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Type propagation: just because something is const doesn't mean it automatically matches the type
3 participants