Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

gh-144145: Track nullness of properties in the Tier 2 JIT optimizer#144122

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
Fidget-Spinner merged 20 commits intopython:mainfromcocolato:dev_143414
Jan 30, 2026

Conversation

@cocolato
Copy link
Contributor

@cocolatococolato commentedJan 21, 2026•
edited
Loading

This PR implements symbolic tracking for__slots__ object attributes in the Tier 2 JIT optimizer.

Currently, when the JIT optimizer encounters_LOAD_ATTR_SLOT, it creates a new unknown symbol (<!NULL>) even if the same slot was previously written in the same trace. This prevents the optimizer from:

  1. Knowing the type of values loaded from slots
  2. Eliminating redundant type guards
  3. Performing further type-based optimizations

Before(set PYTHON_OPT_DEBUG=5):

51 abs: _LOAD_ATTR_SLOT (4, target=48, operand0=0x10, ...)    stack=[..., <Point at 0x...440>]52 abs: _POP_TOP    stack=[..., <!NULL at 0x7fb981d874c0>, <Point at 0x...440>]                ↑↑↑ NOT NULL TYPE

Now:

51 abs: _LOAD_ATTR_SLOT (4, target=48, operand0=0x10, ...)    stack=[..., <Point slots[2] v131145 at 0x...440>]52 abs: _POP_TOP    stack=[..., <compact_int at 0x7f02ba1d6490>, <Point slots[2]>]                ↑↑↑ we know the exact type

Comment on lines 19 to 21
// Maximum slots per object tracked symbolically
#define MAX_SYMBOLIC_SLOTS_SIZE 16
#define SLOTS_ARENA_SIZE (MAX_SYMBOLIC_SLOTS_SIZE * 100)
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

We might need to adjust this arena_size.

@markshannon
Copy link
Member

This PR seems unrelated to the issue. It tracks the internal state of objects, not the uniqueness of references.
Please create a new issue.

Tracking the internal state of objects is tricky, as we need to keep track of where code might escape and invalidate any knowledge prior to the escape. So far, we have only tracked properties of stateless objects, so it isn't an issue.
There is value in tracking the state of objects, but it needs to be done carefully.

In future, I would also advise waiting to see if there is general support on the issue before spending time on the implementation.

cocolato reacted with heart emoji

@cocolato
Copy link
ContributorAuthor

cocolato commentedJan 22, 2026•
edited
Loading

Thanks for reply!

This PR seems unrelated to the issue. It tracks the internal state of objects, not the uniqueness of references.
Please create a new issue.

Sorry, I'm linking to this issue because I discussed the implementation logic with@Fidget-Spinner in this issue, and I think this should be a pre-task for unique reference tracking (if I'm wrong, please point it out, thanks!). I'll create a new issue to link to this PR.

Tracking the internal state of objects is tricky, as we need to keep track of where code might escape and invalidate any knowledge prior to the escape. So far, we have only tracked properties of stateless objects, so it isn't an issue.
There is value in tracking the state of objects, but it needs to be done carefully.

Sorry again for overlooking object escape. I think may be Ken Jin's original requirement was just to add tracking for the Slots object(if I'm wrong, please point it out again, thanks!). Perhaps I should remove the optimization in the_LOAD_ATTR_SLOT section?

In future, I would also advise waiting to see if there is general support on the issue before spending time on the implementation.

Thanks for the reminder. I'll be more careful in the future.

@cocolatococolato changed the titlegh-143414: Add__slots__ object property tracking for the Tier 2 JIT optimizergh-144145: Add__slots__ object property tracking for the Tier 2 JIT optimizerJan 22, 2026
@cocolatococolato marked this pull request as draftJanuary 22, 2026 10:56
@Fidget-Spinner
Copy link
Member

Sorry again for overlooking object escape. I think may be Ken Jin's original requirement was just to add tracking for the Slots object(if I'm wrong, please point it out again, thanks!). Perhaps I should remove the optimization in the_LOAD_ATTR_SLOT section?

Yeah we need to track for escapes. I mentioned it here how to do it#143414 (comment). It got lost in the noise though, so no worries.

@markshannon

In future, I would also advise waiting to see if there is general support on the issue before spending time on the implementation.

I advised@cocolato to embark on this issue. I think there is value in doing it. They waited for me to give my go-ahead before doing it. So I think they didn't do anything wrong.

cocolato and nitishxp reacted with heart emoji

@cocolato
Copy link
ContributorAuthor

Thanks! I will refactor this to also support#144141.

@cocolato
Copy link
ContributorAuthor

@Fidget-Spinner Hi, one thing i want to know. Should this PR flag which property escape during escape handling UOPs (e.g._CALL_*)? This would allow us to reset only those objects instead of resetting all__slot__ objects tracked in s_arena. However, this will made this PR more complex.

@Fidget-Spinner
Copy link
Member

Sorry I'm busy for these 2 days. I'll try to review on Saturday. If Mark reviews it earlier, we can go with his ideas.

cocolato reacted with heart emoji

@Fidget-Spinner
Copy link
Member

@Fidget-Spinner Hi, one thing i want to know. Should this PR flag which property escape during escape handling UOPs (e.g._CALL_*)? This would allow us to reset only those objects instead of resetting all__slot__ objects tracked in s_arena. However, this will made this PR more complex.

No. The problem is any uop that escapes must be handled. For example, anything withPOP_TOP if not eliminated is escaping, because it might call__del__ which can trigger arbitrary code.

Comment on lines 512 to 515
// Track escapes
if (_PyUop_Flags[out_ptr->opcode] & HAS_ESCAPES_FLAG) {
ctx->last_escape_index = uop_buffer_length(&ctx->out_buffer);
}
Copy link
ContributorAuthor

@cocolatococolatoJan 26, 2026•
edited
Loading

Choose a reason for hiding this comment

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

@Fidget-Spinner If we use this method to detect escapes, then eachPOP_TOP following_STORE_ATTR_SLOT will cause tracked slot values invalid due toHAS_ESCAPES_FLAG. Could you please give me some guidance? Thanks！

@cocolatococolato changed the titlegh-144145: Add__slots__ object property tracking for the Tier 2 JIT optimizergh-144145: Add object property tracking for the Tier 2 JIT optimizerJan 27, 2026
@cocolato
Copy link
ContributorAuthor

Updated.

@cocolatococolato marked this pull request as ready for reviewJanuary 28, 2026 14:57
@cocolato
Copy link
ContributorAuthor

Added new uopSTORE_ATTR_SLOT_NULL and now use thewas_init_shim andis_init_shim flag to avoid track escape when Initializing the shim frame (e.g._CREATE_INIT_FRAME and_RETUEN_VALUE ).

@Fidget-Spinner
Copy link
Member

Added new uopSTORE_ATTR_SLOT_NULL and now use thewas_init_shim andis_init_shim flag to avoid track escape when Initializing the shim frame (e.g._CREATE_INIT_FRAME and_RETUEN_VALUE ).

That doesn't seem right. RETURN_VALUE is escaping, as it's possibly freeing locals in a frame, which can trigger arbitrary code.

Copy link
Member

@Fidget-SpinnerFidget-Spinner left a comment

Choose a reason for hiding this comment

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

Looks pretty good, just two comments

@cocolato
Copy link
ContributorAuthor

Updated, added_STORE_ATTR_INSTANCE_VALUE_NULL and now only useis_init_shim flag to avoid_CREATE_INIT_FRAME escaping.

@Fidget-Spinner
Copy link
Member

Fidget-Spinner commentedJan 30, 2026•
edited
Loading

Cool, this LGTM now I think. This deserves a news entry. Potentially call it:

Track nullness of attributes of objects in the JIT optimizer. Patch by Hai Zhu.

I know it's not just nullness, but in all practicality due to the escapes, it's just nullness for now.

cocolato reacted with heart emoji

@Fidget-SpinnerFidget-Spinner changed the titlegh-144145: Add object property tracking for the Tier 2 JIT optimizergh-144145: Track nullness of properties in the Tier 2 JIT optimizerJan 30, 2026
Comment on lines +99 to +102
typedef struct {
uint16_t slot_index;
uint16_t symbol;
} JitOptDescrMapping;

Choose a reason for hiding this comment

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

Is it safe to use this for both normal objects and objects with__slots__, or do we need a separate symbol?

Basically I'm asking if it's possible for an object to both have STORE_ATTR_INSTANCE_VALUE and STORE_ATTR_SLOT, as this will cause a index collision between the slots and offset. It it safe also withSTORE_ATTR_WITH_HINT, as can that can mix withSTORE_ATTR_INSTANCE_VALUE?

If you think of an answer, please let me know, and we can add it as a comment in the code. otherwise, this is kind of scary.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

These two never appear on the same object type because:
STORE_ATTR_INSTANCE_VALUE needsPy_TPFLAGS_MANAGED_DICT flag.
Therefore, there is no index collision between slot offsets and inline value offsets.

// No descriptor, or non overriding.
if ((type->tp_flags&Py_TPFLAGS_MANAGED_DICT)==0) {
SPECIALIZATION_FAIL(base_op,SPEC_FAIL_ATTR_NOT_MANAGED_DICT);
return0;
}

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

However, conflicts betweenSTORE_ATTR_INSTANCE_VALUE andSTORE_ATTR_WITH_HINT can indeed occur. Perhaps a flag could be added to the index to distinguish between the two types?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Should we trackSTORE_ATTR_WITH_HINT in this PR?

Choose a reason for hiding this comment

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

No lets ignore with hint for now.

Copy link
Member

@Fidget-SpinnerFidget-Spinner left a comment

Choose a reason for hiding this comment

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

Thanks

@Fidget-SpinnerFidget-Spinner merged commit1dc12b2 intopython:mainJan 30, 2026
75 checks passed
@Fidget-Spinner
Copy link
Member

Thanks! Do you want to handle STORE_ATTR_WITH_HINT Too?

@cocolato
Copy link
ContributorAuthor

Thanks! Do you want to handle STORE_ATTR_WITH_HINT Too?

Sure, I will work on it :)

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@Fidget-SpinnerFidget-SpinnerFidget-Spinner approved these changes

@markshannonmarkshannonAwaiting requested review from markshannonmarkshannon is a code owner

@tomasr8tomasr8Awaiting requested review from tomasr8tomasr8 is a code owner

@savannahostrowskisavannahostrowskiAwaiting requested review from savannahostrowskisavannahostrowski is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@cocolato@markshannon@Fidget-Spinner
[8]ページ先頭
©2009-2026 Movatter.jp