FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-144540: Eliminate `_MAKE_HEAP_FUNCTION` for owned references by Sacul0457 · Pull Request #146320 · python/cpython · GitHub

/ cpython Public

gh-144540: Eliminate _MAKE_HEAP_FUNCTION for owned references - #146320

Draft
Sacul0457 wants to merge 1 commit into
python:mainfrom
Sacul0457:Remove-Redundant-_MAKE_HEAP_SAFE
ConversationCommits (1)ChecksFiles changed
Draft

gh-144540: Eliminate _MAKE_HEAP_FUNCTION for owned references#146320
Sacul0457 wants to merge 1 commit into
python:mainfrom
Sacul0457:Remove-Redundant-_MAKE_HEAP_SAFE

Conversation

Sacul0457 commented Mar 23, 2026 •
edited by bedevere-app Bot
Loading

Copy link
Copy Markdown
Contributor

If the reference of the returned object is not borrowed, we remove the _MAKE_HEAP_FUNCTION. Hence I ended up changing the test_make_heap_safe_optimized_owned test to test that _MAKE_HEAP_FUNCTION is no longer in the trace as the value returned is not a borrowed reference.

Also, for a "return of a return", given the inner function returns a borrowed reference, the double _MAKE_HEAP_FUNCTION is reduced to just 1 :)

  44 OPTIMIZED: _MAKE_HEAP_SAFE_r11 (0, target=3, operand0=0, operand1=0)
  45 OPTIMIZED: _RETURN_VALUE_r11 (0, target=3, operand0=0, operand1=0)
  46 OPTIMIZED: _CHECK_VALIDITY_r11 (0, jump_target=71, operand0=0, operand1=0)
  47 OPTIMIZED: _SET_IP_r11 (0, target=10, operand0=0x1b5f9947d24, operand1=0)
  48 OPTIMIZED: _RETURN_VALUE_r11 (0, target=10, operand0=0, operand1=0)

I hope I didn't misunderstand the issue...

bedevere-app Bot mentioned this pull request Mar 23, 2026
Open

cocolato commented Mar 23, 2026

Copy link
Copy Markdown
Member

I think this might be a problem because in creation functions such as sym_new_not_null and sym_new_unknown, :

cpython/Python/optimizer_symbols.c

Lines 690 to 709 in fb8d8d9

JitOptRef
_Py_uop_sym_new_unknown(JitOptContext *ctx)
{
JitOptSymbol *res = sym_new(ctx);
if (res == NULL) {
return out_of_space_ref(ctx);
}
return PyJitRef_Wrap(res);
}
JitOptRef
_Py_uop_sym_new_not_null(JitOptContext *ctx)
{
JitOptSymbol *res = sym_new(ctx);
if (res == NULL) {
return out_of_space_ref(ctx);
}
res->tag = JIT_SYM_NON_NULL_TAG;
return PyJitRef_Wrap(res);
}

PyJitRef_Wrap produces a ref with tag = 0.
This means that tag = 0 is the default state, indicating “we have no specific information about borrowing/ownership,” rather than “definitely owned.” Only those explicitly marked with tag = 1 via PyJitRef_Borrow() are “known to be borrowed.”

cocolato commented Mar 23, 2026

Copy link
Copy Markdown
Member

Mark also reminded me in the last PR: #144414 (review).

Fidget-Spinner commented Mar 23, 2026

Copy link
Copy Markdown
Member

Hmm Mark is indeed right. However, currently the only place that can generate borrowed references are LOAD_FAST_BORROW. I suspect we need to create a new tag bit for this to work.

We only support good perf on 64 bit systems right now. I propose we drop the 32 bit Windows support so that we have 3 tag bits to use and can support "unknown" references.

cocolato commented Mar 23, 2026

Copy link
Copy Markdown
Member

I propose we drop the 32 bit Windows support so that we have 3 tag bits to use and can support "unknown" references.

I also don’t think it’s necessary to maintain the performance of legacy systems

Sacul0457 commented Mar 23, 2026

Copy link
Copy Markdown
Contributor Author

I guess I'll mark this PR as draft for now until that is in?

Sacul0457 marked this pull request as draft March 23, 2026 12:00

github-actions Bot commented May 8, 2026

Copy link
Copy Markdown

This PR is stale because it has been open for 30 days with no activity.

github-actions Bot added the stale Stale PR or inactive for long period of time. label May 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

markshannon Awaiting requested review from markshannon markshannon is a code owner
tomasr8 Awaiting requested review from tomasr8 tomasr8 is a code owner
Fidget-Spinner Awaiting requested review from Fidget-Spinner Fidget-Spinner is a code owner
savannahostrowski Awaiting requested review from savannahostrowski savannahostrowski is a code owner

Assignees

No one assigned

Labels

stale Stale PR or inactive for long period of time.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL