Skip to content

gh-146381: Constant fold local frozendict subscript lookups via recording - #146564

Closed
corona10 wants to merge 1 commit into
python:mainfrom
corona10:gh-146381-local-jit
Closed

gh-146381: Constant fold local frozendict subscript lookups via recording#146564
corona10 wants to merge 1 commit into
python:mainfrom
corona10:gh-146381-local-jit

Conversation

@corona10

@corona10corona10 commented Mar 28, 2026

Copy link
Copy Markdown
Member

Comment on lines +1641 to +1647
else {
PyObject *probable = sym_get_probable_value(nos);
if (probable != NULL && PyFrozenDict_CheckExact(probable)) {
ADD_OP(_GUARD_NOS_IS, 0, (uintptr_t)probable);
sym_set_const(nos, probable);
}
}

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@Fidget-Spinner
With this change, we can just simply use the change from what we added from
#146490, it looks like a more natural way from JIT optimizer.

@Fidget-SpinnerFidget-Spinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks! I think this might not be an optimization we want. Wouldnt the frozendict identity change over function invocations?

@corona10
corona10force-pushed the gh-146381-local-jit branch from fc38b65 to 170a3e0CompareMarch 28, 2026 10:33
@corona10

Copy link
Copy Markdown
MemberAuthor

Thanks! I think this might not be an optimization we want. Wouldnt the frozendict identity change over function invocations?

Ah yes.. that's correct.

@corona10

corona10 commented Mar 28, 2026

Copy link
Copy Markdown
MemberAuthor

I thought this would be helpful for loop-heavy workloads within a single function call. Would you prefer a different approach? (if we assume that deopt could be cheap for the new call)

@Fidget-Spinner

Copy link
Copy Markdown
Member

I thought this would be helpful for loop-heavy workloads within a single function call. Would you prefer a different approach? (if we assume that deopt could be cheap for the new call)

Yeah I think this will be useful for loops, but we want to make sure the loops are valid for the next function invocation, so I think we shouldn't do any identity stuff, unless we know the frozendict is always the same.

@corona10

Copy link
Copy Markdown
MemberAuthor

After discussing it with @Fidget-Spinner, we decided not to do it.

@corona10corona10 closed this Apr 4, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@corona10@Fidget-Spinner@savannahostrowski