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.

Sign up for GitHub

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

Jump to bottom

Specialization of some instructions does not conform to PEP 659, and prevents PEP 669 #100982

Closed
markshannon opened this issue Jan 12, 2023 · 10 comments
Closed

Specialization of some instructions does not conform to PEP 659, and prevents PEP 669 #100982

markshannon opened this issue Jan 12, 2023 · 10 comments
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) performance Performance or resource usage

Comments

@markshannon
Copy link
Member

markshannon commented Jan 12, 2023 •
edited by bedevere-bot
Loading

As written, PEP 659 says that individual specializations are restricted to a single instruction.
PEP 669 relies on this, as it also wants to replace instructions at runtime, and it would break if specialization occurs across multiple instructions.

Currently there are a two places where we break this design by specializing pairs of instructions together:

  • COMPARE_OP POP_JUMP_IF_ pairs are specialized together
  • FOR_ITER STORE_FAST are specialized together

The second will go away with the register VM, and doesn't seem to be an issue in practice.
It is the COMPARE_OP POP_JUMP_IF_ specialization that is problematic, as PEP 669 wants to instrument branches.
Instrumenting the POP_JUMP_IF_ doesn't work if the COMPARE_OP specialization jumps over it.

The solution is to replace the COMPARE_OP POP_JUMP_IF_ pair with a single COMPARE_AND_BRANCH instruction that can be specialized or instrumented atomically.

Linked PRs
@markshannon markshannon added the performance Performance or resource usage label Jan 12, 2023
@bedevere-bot bedevere-bot mentioned this issue Jan 12, 2023
Merged
@markshannon
Copy link
Member Author

markshannon commented Jan 12, 2023 •
edited
Loading

Superinstructions are also a problem for PEP 669, but are simpler to handle, and will also go away with the register VM.

markshannon added a commit that referenced this issue Jan 16, 2023
@markshannon
GH-100982: Add COMPARE_AND_BRANCH instruction (GH-100983)
7b14c2e
@brandtbucher
Copy link
Member

BINARY_OP_INPLACE_ADD_UNICODE and CALL_NO_KW_LIST_APPEND are two more examples of "adaptive superinstructions".

Like FOR_ITER, though, they don't cross a branch and will probably disappear when we get a register VM.

@markshannon
Copy link
Member Author

https://github.com/python/cpython/pull/100983[)](https://github.com/python/cpython/commit/7b14c2ef194b6eed79670aa9d7e29ab8e2256a56) fixed the COMPARE_OP issue, so I'm retitling this issue.

@markshannon markshannon changed the title Specialization of COMPARE_OP does not conform to PEP 659, and prevents PEP 669 Specialization of some instructions does not conform to PEP 659, and prevents PEP 669 Jan 27, 2023
@markshannon
Copy link
Member Author

Although BINARY_OP_INPLACE_ADD_UNICODE and CALL_NO_KW_LIST_APPEND don't cross boundaries they may cross lines which interferes with instrumentation for LINE events.

@markshannon
Copy link
Member Author

markshannon commented Jan 27, 2023 •
edited
Loading

The instruction that is skipped in CALL_NO_KW_LIST_APPEND is always a POP_TOP.
We can fix this by making sure that the POP_TOP following a call has no location. It probably should anyway.
The compiler already gives POP_TOP NO_LOCATION so nothing to do here

BINARY_OP_INPLACE_ADD_UNICODE is still a problem. It is rare, so the extra cost of de-specializing it during instrumentation and preventing that specialization if instrumentation is present would probably be OK.

Note:
This is only an issue for instrumentation of LINE events. INSTRUCTION events prevent all specialization, so there cannot be a conflict.

@markshannon
Copy link
Member Author

An alternative to fixing BINARY_OP_INPLACE_ADD_UNICODE is to restore the old behavior of BINARY_ADD to special case adding strings, and make sure that BINARY_OP specialization is so good that the performance of plain BINARY_ADD is irrelevant.

@markshannon
Copy link
Member Author

Which leaves FOR_ITER_RANGE. I'm hoping that improvements to freelists and integer representation will make the cost of creating the temporary int tolerable.

@bedevere-bot bedevere-bot mentioned this issue Feb 17, 2023
Merged
markshannon added a commit that referenced this issue Feb 22, 2023
@markshannon
GH-100982: Restrict FOR_ITER_RANGE to a single instruction to allow…
7c106a4 
… instrumentation. (GH-101985)
carljm added a commit to carljm/cpython that referenced this issue Feb 23, 2023
@carljm
Merge branch 'main' into lockfreecp
b257a16 
* main: (76 commits)
  Fix syntax error in struct doc example (python#102160)
  pythongh-99108: Import MD5 and SHA1 from HACL* (python#102089)
  pythonGH-101777: `queue.rst`: use 2 spaces after a period to be consistent. (python#102143)
  Few coverage nitpicks for the cmath module (python#102067)
  pythonGH-100982: Restrict `FOR_ITER_RANGE` to a single instruction to allow instrumentation. (pythonGH-101985)
  pythongh-102135: Update turtle docs to rename wikipedia demo to rosette (python#102137)
  pythongh-99942: python.pc on android/cygwin should link to libpython per configure.ac (pythonGH-100356)
  pythongh-95672 fix typo SkitTest to SkipTest (pythongh-102119)
  pythongh-101936: Update the default value of fp from io.StringIO to io.BytesIO (pythongh-102100)
  pythongh-102008: simplify test_except_star by using sys.exception() instead of sys.exc_info() (python#102009)
  pythongh-101903: Remove obsolete undefs for previously removed macros Py_EnterRecursiveCall and Py_LeaveRecursiveCall (python#101923)
  pythongh-100556: Improve clarity of `or` docs (python#100589)
  pythongh-101777: Make `PriorityQueue` docs slightly clearer (python#102026)
  pythongh-101965: Fix usage of Py_EnterRecursiveCall return value in _bisectmodule.c (pythonGH-101966)
  pythongh-101578: Amend exception docs (python#102057)
  pythongh-101961 fileinput.hookcompressed should not set the encoding value for the binary mode (pythongh-102068)
  pythongh-102056: Fix a few bugs in error handling of exception printing code (python#102078)
  pythongh-102011: use sys.exception() instead of sys.exc_info() in docs where possible (python#102012)
  pythongh-101566: Sync with zipp 3.14. (pythonGH-102018)
  pythonGH-99818: improve the documentation for zipfile.Path and Traversable (pythonGH-101589)
  ...
@bedevere-bot bedevere-bot mentioned this issue Mar 17, 2023
Merged
brandtbucher added a commit that referenced this issue Mar 23, 2023
@brandtbucher
GH-100982: Break up COMPARE_AND_BRANCH (GH-102801)
0444ae2
Fidget-Spinner pushed a commit to Fidget-Spinner/cpython that referenced this issue Mar 27, 2023
@brandtbucher @Fidget-Spinner
pythonGH-100982: Break up COMPARE_AND_BRANCH (pythonGH-102801)
6183cbd
@Fidget-Spinner Fidget-Spinner mentioned this issue Mar 27, 2023
Closed
warsaw pushed a commit to warsaw/cpython that referenced this issue Apr 11, 2023
@brandtbucher @warsaw
pythonGH-100982: Break up COMPARE_AND_BRANCH (pythonGH-102801)
73558f2
@gvanrossum
Copy link
Member

  • FOR_ITER STORE_FAST are specialized together

The second will go away with the register VM, and doesn't seem to be an issue in practice.

Since there is no register VM (and probably won't be one, or not for a long time), what should we do about this? Ignore it because it "doesn't seem to be an issue in practice"?

OTOH maybe this issue can be closed, since it appears we do have PEP 669 after all? @markshannon

@markshannon
Copy link
Member Author

FOR_ITER and STORE_FAST are no longer specialized together (specifically for PEP 669): #101985

So, yes this can be closed.

@markshannon markshannon mentioned this issue Jun 19, 2023
Merged
@markshannon markshannon reopened this Jun 19, 2023
@markshannon
Copy link
Member Author

It looks like we never fixed up BINARY_OP_INPLACE_ADD_UNICODE

@iritkatriel iritkatriel added the interpreter-core (Objects, Python, Grammar, and Parser dirs) label Nov 27, 2023
JelleZijlstra pushed a commit to JelleZijlstra/cpython that referenced this issue Sep 10, 2024
@markshannon @JelleZijlstra
pythonGH-100982: Restrict FOR_ITER_RANGE to a single instruction to…
ee28e14 
… allow instrumentation. (pythonGH-101985)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) performance Performance or resource usage
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@iritkatriel @gvanrossum @markshannon @brandtbucher