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

GH-130415: Narrow `str` to `""` based on boolean tests by fluhus · Pull Request #130476 · python/cpython · GitHub

Repository navigation

GH-130415: Narrow str to "" based on boolean tests - #130476

Merged
brandtbucher merged 9 commits into
python:mainfrom
fluhus:hack-night2
Mar 4, 2025
Merged

brandtbucher merged 9 commits into
python:mainfrom
fluhus:hack-night2

Conversation

fluhus commented Feb 22, 2025 •
edited by bedevere-app Bot
Loading

Copy link
Copy Markdown
Contributor

Assign value to string when an if evaluates to false.

@brandtbucher

ghost commented Feb 22, 2025 •
edited by ghost
Loading

Copy link
Copy Markdown

All commit authors signed the Contributor License Agreement.

bedevere-app Bot commented Feb 22, 2025

Copy link
Copy Markdown

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

brandtbucher self-assigned this Feb 22, 2025
brandtbucher added performance Performance or resource usage interpreter-core (Objects, Python, Grammar, and Parser dirs) topic-JIT labels Feb 22, 2025
Comment thread Python/optimizer_bytecodes.c Outdated
Comment thread Lib/test/test_capi/test_opt.py Outdated

bedevere-app Bot commented Feb 22, 2025

Copy link
Copy Markdown

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

fluhus commented Feb 22, 2025

Copy link
Copy Markdown
Contributor Author

Added requested corrections. Thanks, @brandtbucher !

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

Thanks for contribution.
Unfortunately, I think there is a critical flaw in this approach as it could result in mis-optimizations in the future.

This would be a useful optimization, so if you're willing to pursue this further, it would be appreciated.

Comment thread Lib/test/test_capi/test_opt.py Outdated
dummy = "aaa"
# Hopefully the optimizer can't guess what the value is.
# empty is always "", but we can only prove that it's a string:
empty = dummy[:0]

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 can easily see the optimizer turning "aaa"[:0] into "".
empty doesn't need to be a constant, we just need it to be mostly "", for profiling.
Use something like empty = "a"[:(n % 1000) == 0]

brandtbucher Mar 3, 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

Since we check the actual path taken as part of the test, we need the value to always be "", not just mostly "". So maybe:

false = i == TIER2_THRESHOLD
empty = "X"[:false]

The optimizer can't prove false is False, so it's good enough for our purposes.

Comment thread Python/optimizer_bytecodes.c Outdated
// *can't* narrow res, since that would cause the guard to be
// removed and the narrowed value to be invalid:
if (next_opcode == _GUARD_IS_FALSE_POP) {
sym_set_const(value, Py_GetConstant(Py_CONSTANT_EMPTY_STR));

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 is strictly incorrect. We don't know that value is "" until after the _GUARD_IS_FALSE_POP.
The reason that matters is that when we start attaching type information to side exits, as we probably will in 3.15, then this could lead us to infer that value is "" on both branches. Which would be wrong.

There are two possible fixes for this.

  • Combine TO_BOOL_STR and _GUARD_IS_FALSE_POP/_GUARD_IS_TRUE_POP into a single (super)instruction, then optimize that.
  • Annotate the bool value resulting from the TO_BOOL with its input, then in _GUARD_IS_FALSE_POP convert the input value to TO_BOOL.

I prefer the second option, although it may be more work, as it is more flexible and can be extended more easily.

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

Yeah, @Fidget-Spinner and I suggested something like the latter on the issue (new symbols like JitBoolOf(JitOptSymbol *source, bool inverted) and JitEqualTo(JitOptSymbol *lhs, JitOptSymbol *rhs, bool inverted)). That's probably the direction we're headed in longer term.

However, I don't think we should let perfect be the enemy of good here. We have nice, working optimizations in these PRs; just because we might sink info onto side exits in the future probably shouldn't prevent us from making changes like this now for 3.14, which are perfectly correct for the current optimizer (which doesn't sink anything).

I'm inclined to land these changes and other similar ones for ==/!= now, and make the symbolic representation of derived boolean values more complex later as an improvement (it will also be able to handle more uncommon cases like x = y == 42; if x: ...). I'm really worried that if we try to "future-proof" optimizations based on what we could do six months from now, it will prevent actual improvements in the near term.

But I'll defer to you here. If having value be narrowed one uop too early in the instruction stream is enough to block this PR, I can work with these new contributors on the more complex solution. But as-is, this has no bugs and works as intended. We don't sink value info onto side exits, so it's correct.

bedevere-app Bot commented Feb 24, 2025

Copy link
Copy Markdown

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

brandtbucher changed the title GH-130415: Add JIT optimization path for _TO_BOOL_STR GH-130415: Narrow str to "" based on boolean tests Mar 3, 2025
brandtbucher merged commit 691354c into python:main Mar 4, 2025
fluhus deleted the hack-night2 branch March 21, 2025 20:23
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

interpreter-core (Objects, Python, Grammar, and Parser dirs) performance Performance or resource usage topic-JIT

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants


Back | FazBrowse Home | New Git URL