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-92031: Deoptimize Static Code at Finalization #92039

Merged
merged 8 commits into from
May 3, 2022

Conversation

sweeneyde
Copy link
Member

@sweeneyde sweeneyde commented Apr 29, 2022

#92031

TODO:

  • Find a more deterministic test case
  • Decide on EXTENDED_ARG_QUICK/EXTENDED_ARG behavior

@sweeneyde sweeneyde added type-bug An unexpected behavior, bug, or error interpreter-core (Objects, Python, Grammar, and Parser dirs) 3.11 only security fixes 🔨 test-with-buildbots Test PR w/ buildbots; report in status section labels Apr 29, 2022
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @sweeneyde for commit df424cf 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Apr 29, 2022
@sweeneyde
Copy link
Member Author

sweeneyde commented Apr 29, 2022

One remark: this will convert EXTENDED_ARG_QUICK to EXTENDED_ARG for subsequent interpreters using this code, even though EXTENDED_ARG_QUICK is the one emitted by the compiler.

We should probably add a case for EXTENDED_ARG --> EXTENDED_ARG_QUICK in _PyCode_Quicken. Though we have a choice: do we

  1. Emit (unquickened) EXTENDED_ARG in the compiler, to let deopt_code() convert it back to that state. This makes unquickened EXTENDED_ARG opcodes a hair slower, but adds consistency between interpreter invocations.
  2. Continue emitting EXTENDED_ARG_QUICK. Then most programs will start as EXTENDED_ARG_QUICK, but there will be an inconsistency between before and after the first Py_FINALIZE()
  3. Add a special case to deopt_code that keeps EXTENDED_ARG_QUICK as is.

The central issue is that _PyOpcode_Deopt[] has two different contradictory meanings in this case: one is "restore to the original" and one is "what to do during tracing".

@markshannon
Copy link
Member

Feel free to add another table, they are only 256 bytes.
Maybe _PyOpcode_Original for restoring back to the original?

@sweeneyde sweeneyde added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label May 2, 2022
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @sweeneyde for commit ad9a38a 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label May 2, 2022
@sweeneyde
Copy link
Member Author

@markshannon is this okay to merge now?

@markshannon
Copy link
Member

Yes it is

@markshannon markshannon merged commit b156578 into python:main May 3, 2022
@vstinner
Copy link
Member

vstinner commented May 4, 2022

        for i in range(50):
            out, err = run("test_repeated_init_exec", code, timeout=60)

test_repeated_init_exec() already repeats the code 4 times. What is the point of repeating the test 50 x 4 = 200 times?

@sweeneyde
Copy link
Member Author


        for i in range(50):

            out, err = run("test_repeated_init_exec", code, timeout=60)

test_repeated_init_exec() already repeats the code 4 times. What is the point of repeating the test 50 x 4 = 200 times?

The failure was still intermittent when I tried it, and just 4 runs only made the test fail about 10-15% of the time when I measured.

@sweeneyde sweeneyde deleted the unquicken branch May 5, 2022 02:27
@vstinner
Copy link
Member

vstinner commented May 5, 2022

The failure was still intermittent when I tried it, and just 4 runs only made the test fail about 10-15% of the time when I measured.

The Python test suite is run many times per day on each CI job and we have tons of CI jobs. IMO there is no need to loop so many tests inside the test.

On buildbots, it's very common that bugs which is barely possible to trigger manually when I really want to reproduce them... fail all the time on a specific worker because each worker has different timings and a different operating system ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.11 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants