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-106008: Make implicit boolean conversions explicit by brandtbucher · Pull Request #106003 · python/cpython

Conversation

@brandtbucher

@brandtbucher brandtbucher commented Jun 23, 2023 •
edited by bedevere-bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

...and specialize them!

This adds a new TO_BOOL bytecode that prefixes all UNARY_NOT, POP_JUMP_IF_TRUE, and POP_JUMP_IF_FALSE instructions, which now require an exact boolean. We also use a spare bit in COMPARE_OP's oparg to indicate whether the result should be converted to bool (this saves a TO_BOOL for most branches, and is a no-op for all COMPARE_OP specializations).

"0% faster". Stats show a 93.5% hit rate for the new instructions.


📚 Documentation preview 📚: https://cpython-previews--106003.org.readthedocs.build/

  • Issue: Make implicit boolean conversions explicit #106008

Sorry, something went wrong.

Uh oh!

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

All reactions
brandtbucher added 15 commits June 6, 2023 13:58
@brandtbucher
Make boolean conversions in branches explicit
69781b4
@brandtbucher
Specialize UNARY_NOT
5289c7f
@brandtbucher
Remove unused stats
7e5c9df
@brandtbucher
Make UNARY_NOT_BOOL a little bit slicker
27bb264
@brandtbucher
Replace UNARY_NOT with TO_BOOL
0463633
@brandtbucher
Catch up with main
d2932a1
@brandtbucher
Branchless branching
47ec38f
@brandtbucher
Add a "boolean conversion" bit to COMPARE_OP
a0bd53a
@brandtbucher
More stats for heap types
bab539b
@brandtbucher
Specialize TO_BOOL_ALWAYS_TRUE
757b1cc
@brandtbucher
Fix refleak and error handling
f9fa498
@brandtbucher
Catch up with main
0c14344
@brandtbucher
Cleanup
d314078
@brandtbucher
blurb add
9aa83b5
@brandtbucher
Docs
a76f11d
@brandtbucher brandtbucher added performance Performance or resource usage interpreter-core (Objects, Python, Grammar, and Parser dirs) labels Jun 23, 2023
@brandtbucher
brandtbucher requested a review from markshannon June 23, 2023 00:44
@brandtbucher brandtbucher self-assigned this Jun 23, 2023
@brandtbucher brandtbucher changed the title Make implicit boolean conversions explicit GH-106008: Make implicit boolean conversions explicit Jun 23, 2023
@bedevere-bot bedevere-bot mentioned this pull request Jun 23, 2023
Make implicit boolean conversions explicit #106008
Closed
@brandtbucher
brandtbucher marked this pull request as ready for review June 23, 2023 03:53
@brandtbucher
brandtbucher requested review from brettcannon, ericsnowcurrently, iritkatriel, ncoghlan and warsaw as code owners June 23, 2023 03:53
@bedevere-bot bedevere-bot added the awaiting core review label Jun 23, 2023
markshannon
markshannon reviewed Jun 23, 2023
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.

A few minor comments, otherwise LGTM.

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread Python/bytecodes.c Outdated
}

inst(TO_BOOL_BOOL, (unused/1, unused/2, value -- value)) {
// Coolest (and dumbest-named) specialization ever:

@markshannon markshannon Jun 23, 2023

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.

True, but the not the most useful comment for someone trying to understand the code 🙂

Sorry, something went wrong.

Uh oh!

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

brandtbucher and artugro reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
Comment thread Python/bytecodes.c Outdated

inst(TO_BOOL_NONE, (unused/1, unused/2, value -- res)) {
// This one is a bit weird, because we expect *some* failures...
// it might be worth combining with TO_BOOL_ALWAYS_TRUE somehow:

@markshannon markshannon Jun 23, 2023

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 think we decided it wasn't, as it reflects the underlying type instability when doing if x: as a stand in for if x is None.

Sorry, something went wrong.

Uh oh!

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

brandtbucher reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread Python/bytecodes.c Outdated
inst(TO_BOOL_STR, (unused/1, unused/2, value -- res)) {
DEOPT_IF(!PyUnicode_CheckExact(value), TO_BOOL);
STAT_INC(TO_BOOL, hit);
if (Py_Is(value, &_Py_STR(empty))) {

@markshannon markshannon Jun 23, 2023

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.

Use value == &_Py_STR(empty). The semantics is value == "", not value is "".
In general I wouldn't use Py_Is except when you want the exact semantics of Python's x is y.

Sorry, something went wrong.

Uh oh!

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

All reactions

@brandtbucher brandtbucher Jun 23, 2023

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.

Hm, I mean, we are checking for the identity of the singleton string here. I'll just change it, though.

Sorry, something went wrong.

Uh oh!

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

All reactions

@markshannon markshannon Jun 26, 2023 •
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.

Hypothetically we could have more than one "" object. I don't think that " ".strip() is "" is part of the language spec, so Py_Is doesn't add any safety, just obfuscation.

We can also potentially have tagged ints, in which case Py_Is would need to become a lot more complex, but value == &_Py_STR(empty) would remain efficient.

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread Python/bytecodes.c Outdated

inst(TO_BOOL_ALWAYS_TRUE, (unused/1, version/2, value -- res)) {
// This one is a bit weird, because we expect *some* failures...
// it might be worth combining with TO_BOOL_NONE somehow:

@markshannon markshannon Jun 23, 2023

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.

See comment above.

Sorry, something went wrong.

Uh oh!

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

brandtbucher reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread Python/bytecodes.c
}
}
assert(PyBool_Check(cond));
JUMPBY(oparg * Py_IsFalse(cond));

@markshannon markshannon Jun 23, 2023

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 is so much more pleasing 🙂

Sorry, something went wrong.

Uh oh!

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

brandtbucher reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread Python/specialize.c
err += add_stat_dict(stats, UNPACK_SEQUENCE, "unpack_sequence");
err += add_stat_dict(stats, FOR_ITER, "for_iter");
err += add_stat_dict(stats, TO_BOOL, "to_bool");
err += add_stat_dict(stats, SEND, "send");

@markshannon markshannon Jun 23, 2023

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.

Thanks, I tend to forget about the stats dict .

Sorry, something went wrong.

Uh oh!

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

brandtbucher reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@markshannon

markshannon commented Jun 23, 2023

Copy link
Copy Markdown
Member

Note for possible future PR:

We could, at the cost of two bits in tp_flags avoid the version number and combine the ALWAYS_TRUE and NONE specializations.
We need a ALWAYS_TRUE_OR_FALSE and a IS_TRUE bit.
Classes that don't override __bool__ or __len__ and None would set the ALWAYS_TRUE_OR_FALSE bit. The IS_TRUE bit would be set to 0 for None, and to 1 for always true objects.

Rather than check the version number, check the ALWAYS_TRUE_OR_FALSE, then res = (tp_flags & IS_TRUE) ? Py_True : Py_False;

For abi4, we could add a per-object bit to handle immutable objects like ints and strings.

All reactions

Sorry, something went wrong.

Uh oh!

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

@brandtbucher
Clean up some commnts
c2d4a33
carljm
carljm reviewed Jun 23, 2023
View reviewed changes
Comment thread Python/flowgraph.c Outdated
}
}
Py_DECREF(newconst);
return index;

@carljm carljm Jun 23, 2023

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.

windows compiler warning here, implicit cast from Py_ssize_t to int

Sorry, something went wrong.

Uh oh!

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

brandtbucher reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
TeamSpen210
TeamSpen210 reviewed Jun 24, 2023
View reviewed changes
Comment thread Python/flowgraph.c Resolved

Uh oh!

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

@tekknolagi tekknolagi mentioned this pull request Jun 27, 2023
Make boolean conversion explicit tekknolagi/skybison#516
Open
@brettcannon
brettcannon removed their request for review June 27, 2023 22:54
@brandtbucher
Add cast
7638b76
@brandtbucher

brandtbucher commented Jun 29, 2023

Copy link
Copy Markdown
Member Author

Merging is currently blocked on #106250.

All reactions

Sorry, something went wrong.

Uh oh!

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

@brandtbucher
Catch up with main
6b4a598
@brandtbucher
brandtbucher merged commit 7b2d94d into python:main Jun 29, 2023
@bedevere-bot bedevere-bot removed the awaiting core review label Jun 29, 2023
@gvanrossum

gvanrossum commented Jul 4, 2023

Copy link
Copy Markdown
Member

Hey @brandtbucher, I have a question about this PR. In #106393 I had to change the code in POP_JUMP_IF_TRUE/FALSE from

JUMPBY(oparg * Py_IsFalse(cond));

to

if (Py_IsFalse(cond)) {
    JUMP_POP_DISPATCH(oparg, 1);  // Macro that wraps JUMPBY()
}

The reason is that the uop executor currently exits whenever it jumps, and your original code from this PR always jumps.

Did you (or @markshannon) have a reason to prefer the oparg * Py_IsFalse(cond) version over the conditional?

If there's no deep reason I'll keep it the way I coded it up; but if there is (maybe it came out faster in a micro-benchmark?) then I suppose I could fix it another way in the uop interpreter (e.g. only exiting if the jump offset is nonzero).

All reactions

Sorry, something went wrong.

Uh oh!

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

@markshannon

markshannon commented Jul 4, 2023

Copy link
Copy Markdown
Member

That the implementation of branches is itself branchless has a certain aesthetic appeal 🙂. TBH, that's the main reason.

The multiplication form will be quicker for unpredictable branches, and slower for predictable ones in the tier 1 interpreter.
I doubt it makes any difference in terms of overall performance, and will likely be irrelevant once the tier 2 interpreter does most of the work.

gvanrossum reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

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

@alexander-beedie alexander-beedie mentioned this pull request May 18, 2024
feat(python): Start updating BytecodeParser for Python 3.13 pola-rs/polars#16304
Merged
facebook-github-bot pushed a commit to facebookincubator/cinderx that referenced this pull request Aug 22, 2025
@jbower-fb @facebook-github-bot
Support TO_BOOL and related optimizations
5ceeb6a
Summary:
Implements what we need from [GH-106008](python/cpython#106003).

Technically the included updates to `UNARY_NOT`, and `POP_JUMP_IF_FALSE|TRUE` are optimizations and not strictly needed. However, it really makes sense to to them.

Reviewed By: DinoV

Differential Revision: D80682012

fbshipit-source-id: 6150d52dddd28ef5c2e401ed17df7c7cad09074b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@carljm carljm carljm left review comments
@markshannon markshannon markshannon left review comments
@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently ericsnowcurrently is a code owner
@ncoghlan ncoghlan Awaiting requested review from ncoghlan ncoghlan is a code owner
@warsaw warsaw Awaiting requested review from warsaw warsaw is a code owner
@iritkatriel iritkatriel Awaiting requested review from iritkatriel iritkatriel is a code owner
+1 more reviewer
@TeamSpen210 TeamSpen210 TeamSpen210 left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

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

Assignees

@brandtbucher brandtbucher

Labels

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

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.

6 participants

@brandtbucher @markshannon @gvanrossum @carljm @TeamSpen210 @bedevere-bot
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.