GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — gh-107557: Remove unnecessary SAVE_IP instructions by gvanrossum · Pull Request #108583 · python/cpython

Conversation

gvanrossum commented Aug 28, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

As a final optimization pass, squeeze out any SAVE_IP instructions that are not needed.

For example, if we have

SAVE_IP
LOAD_FAST
SAVE_IP
LOAD_GLOBAL

we can remove the first SAVE_IP, since LOAD_FAST can neither deoptimize nor error out; but we cannot remove the second SAVE_IP, since LOAD_GLOBAL can error out.

This pass requires new per-opcode flags, HAS_ERROR_FLAG and HAS_DEOPT_FLAG, which must be defined for regular ops as well as for uops. As a bonus we also remove redundant NOP opcodes.

(It may seem suboptimal to call squeeze_stubs() twice -- once after constructing an initial trace if it has stubs, once after removing unneeded uops; but to avoid this we'd need to change a bunch of APIs to return where the gap is. This seems simpler.)

Issue: Tier 2 abstract interpreter for the optimizer #107557

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

gvanrossum added 3 commits

August 28, 2023 10:59


          Add HAS_ERROR_FLAG and HAS_DEOPT_FLAG

f289749


          Write metadata for all ops and all instrs

290f955


          Squeeze out unneeded SAVE_IP from traces

d9258d8

bedevere-bot mentioned this pull request

Aug 28, 2023

Tier 2 abstract interpreter for the optimizer #107557

Closed

3 tasks

gvanrossum requested review from Fidget-Spinner and iritkatriel

August 28, 2023 20:37

gvanrossum added 3 commits

August 28, 2023 14:04


          Factor squeeze_stubs() out of translate_bytecode_to_trace()

51eaa2f


          Use squeeze_stubs() instead of inserting NOPs into gap created by rem…

4c5c38d

…ove_unneeded_uops()


          When there's no gap we can set trace_length = buffer_size

869ae8d

gvanrossum added the skip news label

Aug 28, 2023

gvanrossum added 2 commits

August 28, 2023 14:30


          Always remove NOPs, even if no new ones were added

bf9fefd


          Change meaning of last_instr to be base-0

8d31dc0

gvanrossum commented Aug 28, 2023

Copy link

Copy Markdown

Member Author

(@Fidget-Spinner I'm not sure if reusing your gh-107557 as the issue in the PR subject is right. I've mostly been skipping news for optimizer/executor based changes because they don't matter to end users and only a few core devs even care. Those interested can look at the commit messages.)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          Tweaks to debug output

afb7cc1

gvanrossum marked this pull request as ready for review

August 28, 2023 22:14

bedevere-bot added the awaiting core review label

Aug 28, 2023

Fidget-Spinner approved these changes

Aug 29, 2023

View reviewed changes

Comment thread

Python/optimizer.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

bedevere-bot added awaiting merge and removed awaiting core review labels

Aug 29, 2023

iritkatriel reviewed

Aug 29, 2023

View reviewed changes

Comment thread

Python/optimizer.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Python/optimizer.c Outdated

		};

		static int
		squeeze_stubs(

iritkatriel Aug 29, 2023

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Suggested change

	squeeze_stubs(
	shift_stubs(

(the name implied to me that something happens to the size of the stubs).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

gvanrossum Aug 29, 2023

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

How about move_stubs() (echoing that it uses memmove()).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Python/optimizer.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Python/optimizer.c

		need_ip = true;
		}
		}
		}

iritkatriel Aug 29, 2023

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

assert here that last_instr >= 0?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

gvanrossum Aug 29, 2023

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

No, I can just initialize it to 0 though. If the trace is empty we shouldn't get here but the code here won't do anything.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Tools/cases_generator/flags.py

		or variable_used(instr, "error")
		or variable_used(instr, "pop_1_error")
		or variable_used(instr, "exception_unwind")
		or variable_used(instr, "resume_with_error")

iritkatriel Aug 29, 2023

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

We could make variable_used accept a sequence of strings to check.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

gvanrossum Aug 29, 2023

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

We could, but why bother? Plus, I'd kind of like variable_used(instr, X, Y, Z) to look for the sequence of identifiers X Y Z. (And I'd also like to allow keywords, e.g. goto.)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

gvanrossum added 3 commits

August 29, 2023 08:59


          Rename squeeze_stubs() to move_stubs()

67b9e6b


          Shorten comment

ad5c50f


          Initialize last_instr to 0, not -1

ebce668

gvanrossum commented

Aug 29, 2023

View reviewed changes

gvanrossum left a comment

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Thanks for the reviews! I've changed the function name, shortened the comment, and changed the initialization of last_instr. I'll merge once tests pass.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Python/optimizer.c Outdated

		};

		static int
		squeeze_stubs(

gvanrossum Aug 29, 2023

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

How about move_stubs() (echoing that it uses memmove()).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Python/optimizer.c

		need_ip = true;
		}
		}
		}

gvanrossum Aug 29, 2023

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

No, I can just initialize it to 0 though. If the trace is empty we shouldn't get here but the code here won't do anything.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Tools/cases_generator/flags.py

		or variable_used(instr, "error")
		or variable_used(instr, "pop_1_error")
		or variable_used(instr, "exception_unwind")
		or variable_used(instr, "resume_with_error")

gvanrossum Aug 29, 2023

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

We could, but why bother? Plus, I'd kind of like variable_used(instr, X, Y, Z) to look for the sequence of identifiers X Y Z. (And I'd also like to allow keywords, e.g. goto.)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

gvanrossum enabled auto-merge (squash)

August 29, 2023 16:15

gvanrossum merged commit 4f22152 into python:main

Aug 29, 2023

bedevere-bot removed the awaiting merge label

Aug 29, 2023

bedevere-bot commented Aug 29, 2023

Copy link

Copy Markdown

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

Hi! The buildbot AMD64 Windows10 3.x has failed when building commit 4f22152.

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/146/builds/6164) and take a look at the build logs.
Check if the failure is related to this commit (4f22152) 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/146/builds/6164

Failed tests:

test.test_concurrent_futures.test_shutdown

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

== Tests result: FAILURE then SUCCESS ==

426 tests OK.

10 slowest tests:

test_math: 6 min 21 sec
test_peg_generator: 2 min 37 sec
test_wmi: 2 min 20 sec
test_tokenize: 2 min 5 sec
test.test_multiprocessing_spawn.test_processes: 2 min 2 sec
test_unparse: 2 min 2 sec
test_compileall: 1 min 31 sec
test_mmap: 1 min 29 sec
test_pickle: 1 min 26 sec
test_unicodedata: 1 min 25 sec

37 tests skipped:
test.test_asyncio.test_unix_events
test.test_multiprocessing_fork.test_manager
test.test_multiprocessing_fork.test_misc
test.test_multiprocessing_fork.test_processes
test.test_multiprocessing_fork.test_threads
test.test_multiprocessing_forkserver.test_manager
test.test_multiprocessing_forkserver.test_misc
test.test_multiprocessing_forkserver.test_processes
test.test_multiprocessing_forkserver.test_threads test_curses
test_dbm_gnu test_dbm_ndbm test_devpoll test_epoll test_fcntl
test_fork1 test_gdb test_grp test_ioctl test_kqueue test_openpty
test_perf_profiler test_perfmaps test_poll test_posix test_pty
test_pwd test_readline test_resource test_syslog
test_threadsignals test_wait3 test_wait4 test_xxlimited
test_xxtestfuzz test_zipfile64 test_zoneinfo

1 re-run test:
test.test_concurrent_futures.test_shutdown

Total duration: 22 min 32 sec

Click to see traceback logs

Traceback (most recent call last):
  File "D:\buildarea\3.x.bolen-windows10\build\Lib\test\support\__init__.py", line 203, in _force_run
    return func(*args)
           ^^^^^^^^^^^
PermissionError: [WinError 32] The process cannot access the file because it is being used by another process: 'D:\\buildarea\\3.x.bolen-windows10\\build\\build\\test_python_7492�\\test_python_worker_10044�'


Traceback (most recent call last):
  File "D:\buildarea\3.x.bolen-windows10\build\Lib\runpy.py", line 198, in _run_module_as_main
    return _run_code(code, main_globals, None,
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "D:\buildarea\3.x.bolen-windows10\build\Lib\runpy.py", line 88, in _run_code
    exec(code, run_globals)
  File "D:\buildarea\3.x.bolen-windows10\build\Lib\test\__main__.py", line 2, in <module>
    main()
  File "D:\buildarea\3.x.bolen-windows10\build\Lib\test\libregrtest\main.py", line 826, in main
    Regrtest().main(tests=tests, **kwargs)
  File "D:\buildarea\3.x.bolen-windows10\build\Lib\test\libregrtest\main.py", line 756, in main
    with os_helper.temp_cwd(test_cwd, quiet=True):
  File "D:\buildarea\3.x.bolen-windows10\build\Lib\contextlib.py", line 159, in __exit__
    self.gen.throw(value)
  File "D:\buildarea\3.x.bolen-windows10\build\Lib\test\support\os_helper.py", line 531, in temp_cwd
    with temp_dir(path=name, quiet=quiet) as temp_path:
  File "D:\buildarea\3.x.bolen-windows10\build\Lib\contextlib.py", line 159, in __exit__
    self.gen.throw(value)
  File "D:\buildarea\3.x.bolen-windows10\build\Lib\test\support\os_helper.py", line 485, in temp_dir
    rmtree(path)
  File "D:\buildarea\3.x.bolen-windows10\build\Lib\test\support\os_helper.py", line 442, in rmtree
    _rmtree(path)
  File "D:\buildarea\3.x.bolen-windows10\build\Lib\test\support\os_helper.py", line 385, in _rmtree
    _waitfor(_rmtree_inner, path, waitall=True)
  File "D:\buildarea\3.x.bolen-windows10\build\Lib\test\support\os_helper.py", line 330, in _waitfor
    func(pathname)
  File "D:\buildarea\3.x.bolen-windows10\build\Lib\test\support\os_helper.py", line 382, in _rmtree_inner
    _force_run(fullname, os.rmdir, fullname)
  File "D:\buildarea\3.x.bolen-windows10\build\Lib\test\support\__init__.py", line 214, in _force_run
    return func(*args)
           ^^^^^^^^^^^
PermissionError: [WinError 32] The process cannot access the file because it is being used by another process: 'D:\\buildarea\\3.x.bolen-windows10\\build\\build\\test_python_7492�\\test_python_worker_10044�'


Traceback (most recent call last):
  File "D:\buildarea\3.x.bolen-windows10\build\Lib\test\support\os_helper.py", line 480, in temp_dir
    yield path
  File "D:\buildarea\3.x.bolen-windows10\build\Lib\test\support\os_helper.py", line 533, in temp_cwd
    yield cwd_dir
  File "D:\buildarea\3.x.bolen-windows10\build\Lib\test\libregrtest\main.py", line 762, in main
    self._main(tests, kwargs)
  File "D:\buildarea\3.x.bolen-windows10\build\Lib\test\libregrtest\main.py", line 821, in _main
    sys.exit(0)
SystemExit: 0

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

gvanrossum deleted the omit-save-ip branch

August 29, 2023 17:30

gvanrossum commented Aug 29, 2023

Copy link

Copy Markdown

Member Author

Benchmarking results show this would be 4% slower than main if the optimizer was always on (it isn't).

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

gvanrossum mentioned this pull request

Sep 4, 2023

We need to change the contract and interface of _PyExecutorObject and _PyOptimizerObject #108866

Closed

Reviewers

iritkatriel

iritkatriel left review comments

Fidget-Spinner

Fidget-Spinner approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

gvanrossum commented Aug 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gvanrossum commented Aug 28, 2023

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gvanrossum left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Aug 29, 2023

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

Uh oh!

gvanrossum commented Aug 29, 2023

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gvanrossum commented Aug 28, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.