◐ Shell
reader mode source ↗
Skip to content

GH-106008: Make implicit boolean conversions explicit#106003

Merged
brandtbucher merged 18 commits into
python:mainfrom
brandtbucher:to-bool
Jun 29, 2023
Merged

GH-106008: Make implicit boolean conversions explicit#106003
brandtbucher merged 18 commits into
python:mainfrom
brandtbucher:to-bool

Conversation

@brandtbucher

@brandtbucher brandtbucher commented Jun 23, 2023

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/

@brandtbucher brandtbucher added performance Performance or resource usage interpreter-core 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 Jun 23, 2023
@brandtbucher brandtbucher marked this pull request as ready for review June 23, 2023 03:53

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

Hide comment

A few minor comments, otherwise LGTM.

@markshannon

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.

@brandtbucher

Copy link
Copy Markdown
Member Author

Merging is currently blocked on #106250.

@brandtbucher brandtbucher merged commit 7b2d94d into python:main Jun 29, 2023
@gvanrossum

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

@markshannon

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.

facebook-github-bot pushed a commit to facebookincubator/cinderx that referenced this pull request Aug 22, 2025
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

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants