gh-106290: Fix edge cases around uops #106319

gvanrossum · 2023-07-01T20:44:12Z

A bit of an omnibus PR, alas (let me know if you want it split up). This does roughly the following (though not in that order):

Tweak uops debugging output
Fix the bug from Line numbers for errors off in traces #106290
Rename SET_IP to SAVE_IP (per Creation of superblocks faster-cpython/ideas#558)
Add a SAVE_IP uop at the start of the trace (ditto)
Allow unbound_local_error; this gives us uops for LOAD_FAST_CHECK, LOAD_CLOSURE, and DELETE_FAST
Longer traces
Support STORE_FAST_LOAD_FAST, STORE_FAST_STORE_FAST
Add deps on pycore_uops.h to Makefile(.pre.in)

- Longer traces - Explain reason for ending a trace - Support STORE_FAST_LOAD_FAST, STORE_FAST_STORE_FAST - Add deps on pycore_uops.h

This gives us LOAD_FAST_CHECK, LOAD_CLOSURE, and DELETE_FAST.

This doesn't work right yet, alas. If an error occurs in the first uop, the lineno is -1.

The trick is that EXIT_TRACE must decrement prev_instr.

To match faster-cpython/ideas#558.

gvanrossum · 2023-07-01T20:49:56Z

While we're cleaning up uops, maybe we should change "trace" to "superblock"? There are already several other things using the name "trace", e.g. sys.settrace(), tracemalloc, lltrace.

gvanrossum · 2023-07-02T03:15:10Z

This (with uops on by default) benchmarks 2% slower than main (with uops off by default).

I will now revert the final (temporary) commit that turns on uops by default and mark it as ready for review.

(I just did that to be able to benchmark it and run the tests.) This reverts commit 5b85a49.

gvanrossum · 2023-07-02T03:19:13Z

(Also, the tests that fail with uops on by default are largely expected. There's one refleak -- maybe I should ask Kumar to review it again. :-)

gvanrossum · 2023-07-02T15:20:50Z

Maybe the leak is the optimizer created and installed initially.

brandtbucher

The code itself looks good, haven't tried running it myself or anything. Just one note:

brandtbucher · 2023-07-03T18:04:38Z

Python/optimizer.c


 #undef ADD_TO_TRACE
+#undef ADD_TO_TRACE_BASIC


I don't see where this is defined...

Turd from the last cleanup. I'll get rid of it, then land it.

Use it to `#undef DPRINTF` instead (which also depends on a variable in this scope).

bedevere-bot · 2023-07-03T20:20:37Z

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

Hi! The buildbot ARM64 macOS 3.x has failed when building commit 2028a4f.

What do you need to do:

Don't panic.
Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/725/builds/4943) and take a look at the build logs.
Check if the failure is related to this commit (2028a4f) or if it is a false positive.
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/725/builds/4943

Failed tests:

test_unittest

Failed subtests:

test_wait_until_any_call_positional - test.test_unittest.testmock.testthreadingmock.TestThreadingMock.test_wait_until_any_call_positional

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

== Tests result: FAILURE then FAILURE ==

420 tests OK.

10 slowest tests:

test_signal: 4 min 13 sec
test_concurrent_futures: 2 min 49 sec
test_multiprocessing_spawn: 2 min 19 sec
test_multiprocessing_forkserver: 1 min 41 sec
test_math: 1 min 38 sec
test_logging: 1 min 3 sec
test_capi: 54.7 sec
test_unparse: 49.4 sec
test_tokenize: 45.2 sec
test_ssl: 43.1 sec

1 test failed:
test_unittest

22 tests skipped:
test.test_asyncio.test_windows_events
test.test_asyncio.test_windows_utils test_devpoll test_epoll
test_gdb test_idle test_ioctl test_launcher
test_multiprocessing_fork test_perf_profiler test_perfmaps
test_startfile test_tcl test_tkinter test_ttk test_ttk_textonly
test_turtle test_winconsoleio test_winreg test_winsound test_wmi
test_zipfile64

1 re-run test:
test_unittest

Total duration: 10 min 6 sec

Click to see traceback logs

Traceback (most recent call last):
  File "/Users/buildbot/buildarea/3.x.pablogsal-macos-m1.macos-with-brew/build/Lib/test/test_unittest/testmock/testthreadingmock.py", line 173, in test_wait_until_any_call_positional
    self.assertNotIn(call(2), something.method_1.mock_calls)
AssertionError: call(2) unexpectedly found in [call(2), call(1)]


Traceback (most recent call last):
  File "/Users/buildbot/buildarea/3.x.pablogsal-macos-m1.macos-with-brew/build/Lib/test/test_unittest/testmock/testthreadingmock.py", line 172, in test_wait_until_any_call_positional
    something.method_1.assert_called_with(1)
  File "/Users/buildbot/buildarea/3.x.pablogsal-macos-m1.macos-with-brew/build/Lib/unittest/mock.py", line 979, in assert_called_with
    raise AssertionError(_error_message()) from cause
AssertionError: expected call not found.
Expected: method_1(1)
Actual: method_1(2)

gvanrossum added 6 commits July 1, 2023 13:19

Add commented-out prints about why an op is not viable

a2216ce

Misc uops changes

028b17d

- Longer traces - Explain reason for ending a trace - Support STORE_FAST_LOAD_FAST, STORE_FAST_STORE_FAST - Add deps on pycore_uops.h

There's no reason to forbid unbound_local_error

4a75f35

This gives us LOAD_FAST_CHECK, LOAD_CLOSURE, and DELETE_FAST.

Add a SET_IP uop at the top of a trace

29a153f

This doesn't work right yet, alas. If an error occurs in the first uop, the lineno is -1.

Fix pythongh-106290 (wrong line numbers in traceback)

7944a80

The trick is that EXIT_TRACE must decrement prev_instr.

Rename SET_IP to SAVE_IP

c6ded69

To match faster-cpython/ideas#558.

bedevere-bot mentioned this pull request Jul 1, 2023

Line numbers for errors off in traces #106290

Closed

Also fix prev_instr when deoptimizing

f6c9d52

gvanrossum added the skip news label Jul 1, 2023

TEMPORARY -- turn on uops always

5b85a49

Revert "TEMPORARY -- turn on uops always"

b2792db

(I just did that to be able to benchmark it and run the tests.) This reverts commit 5b85a49.

gvanrossum marked this pull request as ready for review July 2, 2023 03:16

gvanrossum requested a review from markshannon as a code owner July 2, 2023 03:16

bedevere-bot added the awaiting core review label Jul 2, 2023

gvanrossum added 2 commits July 2, 2023 21:14

Streamline debug output in optimizer.c

8a04a31

Streamline debug output in ceval.c

118b324

brandtbucher approved these changes Jul 3, 2023

View reviewed changes

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

Fix stray #undef

f886d5b

Use it to `#undef DPRINTF` instead (which also depends on a variable in this scope).

gvanrossum enabled auto-merge (squash) July 3, 2023 19:29

Merge branch 'python:main' into tweak-uops

4816076

gvanrossum merged commit 2028a4f into python:main Jul 3, 2023

bedevere-bot removed the awaiting merge label Jul 3, 2023

gvanrossum deleted the tweak-uops branch July 5, 2023 15:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-106290: Fix edge cases around uops #106319

gh-106290: Fix edge cases around uops #106319

gvanrossum commented Jul 1, 2023 •

edited

Loading

gvanrossum commented Jul 1, 2023

gvanrossum commented Jul 2, 2023

gvanrossum commented Jul 2, 2023

gvanrossum commented Jul 2, 2023

brandtbucher left a comment

brandtbucher Jul 3, 2023

gvanrossum Jul 3, 2023

bedevere-bot commented Jul 3, 2023

gh-106290: Fix edge cases around uops #106319

gh-106290: Fix edge cases around uops #106319

Conversation

gvanrossum commented Jul 1, 2023 • edited Loading

gvanrossum commented Jul 1, 2023

gvanrossum commented Jul 2, 2023

gvanrossum commented Jul 2, 2023

gvanrossum commented Jul 2, 2023

brandtbucher left a comment

Choose a reason for hiding this comment

brandtbucher Jul 3, 2023

Choose a reason for hiding this comment

gvanrossum Jul 3, 2023

Choose a reason for hiding this comment

bedevere-bot commented Jul 3, 2023

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

gvanrossum commented Jul 1, 2023 •

edited

Loading