Skip to content

gh-119786: Edit InternalDocs/frames.md, Python/vm-state.md, Python/tier2_engine.md #124450

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

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

Eclips4
Copy link
Member

@Eclips4 Eclips4 commented Sep 24, 2024

I just changed a few lines in the vm-state file to not describe the frames, but instead refer to the frames themselves. I also added a link to the tier2_engine in the short description for tier 2.

picnixz
picnixz previously approved these changes Sep 27, 2024

There are some other fields in the frame structure of less importance; notably frames are linked together in a singly-linked list via the `previous` pointer, pointing from callee to caller.
The frame also holds a pointer to the current function, globals, builtins, and the locals converted to dict (used to support the `locals()` built-in).
A pointer to the current frame is held in `frame`, for more information about what `frame` contains see [Frames](frames.md):

Copy link
Member

@iritkatriel iritkatriel Sep 27, 2024

Choose a reason for hiding this comment

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

CC @markshannon

I can't comment on lines that haven't changed, so this is for a number of comments on vm-state.md.

L 21: The interpreters share an implementation of what? The frame? Caches the depth - is this stack depth?

L 40: Add a link to exception_handling.md.

L45: Not sure what you mean here: "The implementation of jumps within a single Tier 2 superblock/trace is just that, an implementation."

L51: "within the superblock" is repeated twice.
L52: what cannot be modified?

I think it might be worth moving the contents of the "Thread state and interpreter state" section to the beginning, as a high level overview of the components of the state, and then drill into the parts.

The "Tier 2 IR format" section doesn't seem to belong to the vm-state topic at all.

Copy link
Member

Choose a reason for hiding this comment

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

The interpreters share an implementation of what?

Both tier 1 and tier 2 use the same canonical in-memory representation. Tier 2 might store some values temporarily in registers, but that should be invisible to other code. The reason this is noteworthy is that other VMs, e.g. HotSpot can have different frame layouts for the compiler and interpreter.

@picnixz picnixz dismissed their stale review September 27, 2024 15:39

I hit "approve" instead of "comment"

Copy link
Member

@markshannon markshannon left a comment

Choose a reason for hiding this comment

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

It isn't clear what "vm state" is meant to cover here.
I think the sections on frames and unwinding should be moved to their own files.

The core "vm state", ie. the state needed to execute code is just the thread-state pointer.
From the thread-state, the frame pointer, and thence the stack and instruction pointers can be read.

When executing code however, the stack pointer, frame pointer and instruction pointers are cached in the interpreter (or jitted code).
The frame and instruction pointers are kept up to date in memory, the local versions are just cached for speed. The stack pointer is a bit different in that the in-memory version is marked as invalid, as keeping the local and in-memory versions in sync would be too expensive.


There are some other fields in the frame structure of less importance; notably frames are linked together in a singly-linked list via the `previous` pointer, pointing from callee to caller.
The frame also holds a pointer to the current function, globals, builtins, and the locals converted to dict (used to support the `locals()` built-in).
A pointer to the current frame is held in `frame`, for more information about what `frame` contains see [Frames](frames.md):

Copy link
Member

Choose a reason for hiding this comment

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

The interpreters share an implementation of what?

Both tier 1 and tier 2 use the same canonical in-memory representation. Tier 2 might store some values temporarily in registers, but that should be invisible to other code. The reason this is noteworthy is that other VMs, e.g. HotSpot can have different frame layouts for the compiler and interpreter.

@markshannon
Copy link
Member

Would it make sense to separate the move from any editing we want to do?

Maybe do the move first in one PR, so everything is in the same folder, then do the edits in other PR(s)?
Or the other way around?

@iritkatriel
Copy link
Member

Would it make sense to separate the move from any editing we want to do?

Maybe do the move first in one PR, so everything is in the same folder, then do the edits in other PR(s)? Or the other way around?

If it helps to split, then I would rather edit then move, so that we know that what's in the new folder is good.

@Eclips4 Eclips4 changed the title gh-119786: Move Python/vm-state.md and Python/tier2_engine.md to the InternalDocs gh-119786: Edit InternalDocs/frames.md, Python/vm-state.md, Python/tier2_engine.md` Jan 13, 2025
@Eclips4 Eclips4 changed the title gh-119786: Edit InternalDocs/frames.md, Python/vm-state.md, Python/tier2_engine.md` gh-119786: Edit InternalDocs/frames.md, Python/vm-state.md, Python/tier2_engine.md Jan 13, 2025
@Eclips4 Eclips4 requested a review from markshannon January 13, 2025 12:21
@Eclips4 Eclips4 requested a review from iritkatriel January 13, 2025 12:21
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.

4 participants