GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-134584: Eliminate redundant refcounting from _BINARY_OP_ADD_UNICODE by corona10 · Pull Request #135817 · python/cpython

Conversation

@corona10

@corona10 corona10 commented Jun 22, 2025 •
edited by bedevere-app Bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member
  • Issue: Eliminate redundant refcounting in the JIT #134584

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
corona10 added 2 commits June 22, 2025 20:18
@corona10
pythongh-134584: Elimiate redundant refcounting from _BINARY_OP_X_UNI…
d5afe15 
…CODE
@corona10
WIP
46b423f
@corona10
corona10 requested review from Fidget-Spinner, ericsnowcurrently, iritkatriel, markshannon and tomasr8 as code owners June 22, 2025 14:40
@bedevere-app bedevere-app Bot added the awaiting core review label Jun 22, 2025
@bedevere-app bedevere-app Bot mentioned this pull request Jun 22, 2025
Eliminate redundant refcounting in the JIT #134584
Closed
35 tasks done
@corona10 corona10 assigned Fidget-Spinner Jun 22, 2025
@corona10 corona10 added the skip news label Jun 22, 2025
@corona10

corona10 commented Jun 22, 2025

Copy link
Copy Markdown
Member Author

Maybe I have to wait #135761 first
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Fidget-Spinner
Fidget-Spinner reviewed Jun 22, 2025
View reviewed changes

@Fidget-Spinner Fidget-Spinner left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Mostly LGTM.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Python/optimizer_bytecodes.c Outdated
Comment on lines 330 to 338
l = left;
r = right;
Py_DECREF(temp);
}
else {
res = sym_new_type(ctx, &PyUnicode_Type);
l = left;
r = right;
}

@Fidget-Spinner Fidget-Spinner Jun 22, 2025

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This doesn't need to be in the branch. You can write this at the end of the instruction here.

That way, we only need to have one l = left; r = right

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
tomasr8
tomasr8 reviewed Jun 22, 2025
View reviewed changes
Comment thread Python/optimizer_bytecodes.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/test/test_capi/test_opt.py
self.assertIsNotNone(ex)
uops = get_opnames(ex)

self.assertIn("_POP_TOP_NOP", uops)

@tomasr8 tomasr8 Jun 22, 2025

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Maybe also add

self.assertIn("_BINARY_OP_ADD_UNICODE", uops)

to make sure this is the op that's actually generated?

Should we also test for _POP_TOP_UNICODE?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@corona10 corona10 Jun 22, 2025

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

In this code, It will be replaced to _POP_TOP_NOP because _POP_TOP_UNICODE will be optimized to _POP_TOP_NOP .

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@tomasr8 tomasr8 Jun 23, 2025

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Yup, do you think it makes sense to test _POP_TOP_UNICODE separately? That is, the case when it is not optimized to _POP_TOP_NOP? (not sure how difficult it'll be to come up with a test case though)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@Fidget-Spinner Fidget-Spinner Jun 23, 2025

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I have a test for that in my original PR already.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
tomasr8 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Fidget-Spinner
Fidget-Spinner reviewed Jun 22, 2025
View reviewed changes
Comment thread Lib/test/test_capi/test_opt.py
Comment on lines +2367 to +2383
def test_store_fast_pop_top_specialize_unicode(self):
def random_str(n):
return ''.join(random.choice(string.ascii_uppercase + string.digits) for _ in range(n))
def testfunc(n):
y = random_str(32)
for _ in range(n):
x = y + y # _POP_TOP
x = None # _POP_TOP_NOP

testfunc(TIER2_THRESHOLD)

ex = get_first_executor(testfunc)
self.assertIsNotNone(ex)
uops = get_opnames(ex)

self.assertIn("_POP_TOP_NOP", uops)

@Fidget-Spinner Fidget-Spinner Jun 22, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Sorry I thought of a better test: you can use this one but change for str, and assert that the _POP_TOP_UNICODE uop is inside

cpython/Lib/test/test_capi/test_opt.py

Line 2310 in 46b423f

def test_float_op_refcount_elimination(self):

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
corona10 added 2 commits June 23, 2025 00:17
@corona10
Address code review
c901079
@corona10
Address code review
ea112a5
@Fidget-Spinner

Fidget-Spinner commented Jun 23, 2025

Copy link
Copy Markdown
Member

Sorry I just merged my PR in #135761. You'll have to rebase/merge in changes!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@corona10
Merge remote-tracking branch 'upstream/main' into pythongh-134584-uni…
d95aee9 
…code
@corona10
corona10 requested a review from Fidget-Spinner June 23, 2025 17:13
@corona10

corona10 commented Jun 23, 2025

Copy link
Copy Markdown
Member Author

@Fidget-Spinner Done!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@corona10 corona10 changed the title gh-134584: Elimiate redundant refcounting from _BINARY_OP_ADD_UNICODE gh-134584: Eliminate redundant refcounting from _BINARY_OP_ADD_UNICODE Jun 23, 2025
markshannon
markshannon requested changes Jun 24, 2025
View reviewed changes

@markshannon markshannon left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This adds an extra field to every frame, which is likely to cost more than the improvement this PR would otherwise bring.
If we apply this change to every BINARY_OP, then it might be worth it.

Having said that, TOS caching might solve that problem.
If we can guarantee that each uop that would overflow the stack always leaves the necessary number of outputs in registers and that we never spill those values to memory,
than we can do this without the extra stack space.

Let's wait until TOS is in, and then we can investigate if that approach makes sense.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
corona10 and Fidget-Spinner reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
@bedevere-app bedevere-app Bot added awaiting changes and removed awaiting core review labels Jun 24, 2025
@bedevere-app

bedevere-app Bot commented Jun 24, 2025

Copy link
Copy Markdown

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Fidget-Spinner Fidget-Spinner closed this Dec 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tomasr8 tomasr8 tomasr8 left review comments
@markshannon markshannon markshannon requested changes
@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently ericsnowcurrently is a code owner
@iritkatriel iritkatriel Awaiting requested review from iritkatriel iritkatriel is a code owner
@Fidget-Spinner Fidget-Spinner Awaiting requested review from Fidget-Spinner Fidget-Spinner is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@Fidget-Spinner Fidget-Spinner

Labels

awaiting changes skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@corona10 @Fidget-Spinner @tomasr8 @markshannon
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.