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

gh-134584: Eliminate redundant refcounting from _BINARY_OP_ADD_UNICODE by corona10 · Pull Request #135817 · python/cpython · GitHub

  1. python
  2. cpython

Repository navigation

gh-134584: Eliminate redundant refcounting from _BINARY_OP_ADD_UNICODE - #135817

Closed
corona10 wants to merge 5 commits into
python:mainfrom
corona10:gh-134584-unicode
ConversationCommits (5)ChecksFiles changed
Closed

gh-134584: Eliminate redundant refcounting from _BINARY_OP_ADD_UNICODE#135817
corona10 wants to merge 5 commits into
python:mainfrom
corona10:gh-134584-unicode

Conversation

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

Copy link
Copy Markdown
Member

bedevere-app Bot mentioned this pull request Jun 22, 2025
Closed
35 tasks done

corona10 commented Jun 22, 2025

Copy link
Copy Markdown
Member Author

Maybe I have to wait #135761 first

Fidget-Spinner reviewed Jun 22, 2025
View reviewed changes

Fidget-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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Mostly LGTM.

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 Jun 22, 2025

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

tomasr8 reviewed Jun 22, 2025
View reviewed changes
Comment thread Python/optimizer_bytecodes.c Outdated
Comment thread Lib/test/test_capi/test_opt.py
self.assertIsNotNone(ex)
uops = get_opnames(ex)

self.assertIn("_POP_TOP_NOP", uops)

tomasr8 Jun 22, 2025

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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?

corona10 Jun 22, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

tomasr8 Jun 23, 2025

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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)

Fidget-Spinner Jun 23, 2025

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

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 Jun 22, 2025 •
edited
Loading

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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):

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!

corona10 requested a review from Fidget-Spinner June 23, 2025 17:13

corona10 commented Jun 23, 2025

Copy link
Copy Markdown
Member Author

@Fidget-Spinner Done!

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 requested changes Jun 24, 2025
View reviewed changes

markshannon 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

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.

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

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

Assignees

Fidget-Spinner

Labels

awaiting changes skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL