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-93516: Store offset of first traceable instruction in code object #93769

Merged

Conversation

markshannon
Copy link
Member

@markshannon markshannon commented Jun 13, 2022

Avoids having to recompute the offset all the time when tracing.

@markshannon
Copy link
Member Author

@markshannon markshannon commented Jun 13, 2022

Blocked by #93771

@gvanrossum
Copy link
Member

@gvanrossum gvanrossum commented Jun 13, 2022

As an experiment, can you remove the "import opcode" from deepfreeze.py and hardcode the RESUME opcode? We can debate whether that's the right fix, but so far the deepfreeze.py script has been carefully designed not to depend on the details of the target version.

@gvanrossum
Copy link
Member

@gvanrossum gvanrossum commented Jun 13, 2022

Did you consider putting the new field in the marshaled code object? Then deepfreeze.py wouldn't have to import opcode.

@markshannon
Copy link
Member Author

@markshannon markshannon commented Jun 13, 2022

Did you consider putting the new field in the marshaled code object?

Yes, but I decided against it.
This PR needs backporting to 3.11, so I want to keep it as small as possible, and it is already quite large.

Objects/codeobject.c Outdated Show resolved Hide resolved
@markshannon markshannon merged commit 3cd1a5d into python:main Jun 14, 2022
13 checks passed
@miss-islington
Copy link
Contributor

@miss-islington miss-islington commented Jun 14, 2022

Thanks @markshannon for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒🤖

@markshannon markshannon deleted the cache-first-traceable-instruction branch Jun 14, 2022
@@ -20,6 +20,9 @@
verbose = False
identifiers, strings = get_identifiers_and_strings()

# This must be kept in sync with opcode.py
RESUME = 151

Copy link
Member

@iritkatriel iritkatriel Jun 16, 2022

Choose a reason for hiding this comment

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

Can't this be opcode.opmap["RESUME"]?

Copy link
Member

@gvanrossum gvanrossum Jun 16, 2022

Choose a reason for hiding this comment

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

No, that would introduce a dependency on the latest opcode.py which would mean you cannot run deepfreeze.py with e.g. Python 3.10. On Windows and for cross compilations we need to be able to do that. (Windows doesn't have a separate bootstrap Python because it would near-doubel build times.)

We started with this (see #93771).

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.

None yet

6 participants