GH-106008: Make implicit boolean conversions explicit #106003

brandtbucher · 2023-06-23T00:44:14Z

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

markshannon

A few minor comments, otherwise LGTM.

markshannon · 2023-06-23T14:12:45Z

Python/bytecodes.c

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


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

markshannon · 2023-06-23T14:14:34Z

Python/bytecodes.c

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


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.

markshannon · 2023-06-23T14:17:30Z

Python/bytecodes.c

+        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))) {


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.

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

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.

markshannon · 2023-06-23T14:17:56Z

Python/bytecodes.c

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


See comment above.

markshannon · 2023-06-23T14:19:38Z

Python/bytecodes.c

-                }
-            }
+            assert(PyBool_Check(cond));
+            JUMPBY(oparg * Py_IsFalse(cond));


This is so much more pleasing 🙂

markshannon · 2023-06-23T14:34:24Z

Python/specialize.c

@@ -107,6 +107,8 @@ _Py_GetSpecializationStats(void) {
    err += add_stat_dict(stats, COMPARE_OP, "compare_op");
    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");


Thanks, I tend to forget about the stats dict .

markshannon · 2023-06-23T14:43:00Z

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.

carljm · 2023-06-23T22:53:05Z

Python/flowgraph.c

+        }
+    }
+    Py_DECREF(newconst);
+    return index;


windows compiler warning here, implicit cast from Py_ssize_t to int

Python/flowgraph.c

brandtbucher · 2023-06-29T18:25:09Z

Merging is currently blocked on #106250.

gvanrossum · 2023-07-04T04:08:30Z

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 · 2023-07-04T08:20:25Z

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.

brandtbucher added 15 commits June 6, 2023 13:58

Make boolean conversions in branches explicit

69781b4

Specialize UNARY_NOT

5289c7f

Remove unused stats

7e5c9df

Make UNARY_NOT_BOOL a little bit slicker

27bb264

Replace UNARY_NOT with TO_BOOL

0463633

Catch up with main

d2932a1

Branchless branching

47ec38f

Add a "boolean conversion" bit to COMPARE_OP

a0bd53a

More stats for heap types

bab539b

Specialize TO_BOOL_ALWAYS_TRUE

757b1cc

Fix refleak and error handling

f9fa498

Catch up with main

0c14344

Cleanup

d314078

blurb add

9aa83b5

Docs

a76f11d

brandtbucher added performance Performance or resource usage interpreter-core (Objects, Python, Grammar, and Parser dirs) labels Jun 23, 2023

brandtbucher requested a review from markshannon June 23, 2023 00:44

brandtbucher self-assigned this Jun 23, 2023

brandtbucher changed the title ~~Make implicit boolean conversions explicit~~ GH-106008: Make implicit boolean conversions explicit Jun 23, 2023

bedevere-bot mentioned this pull request Jun 23, 2023

Make implicit boolean conversions explicit #106008

Closed

brandtbucher marked this pull request as ready for review June 23, 2023 03:53

brandtbucher requested review from brettcannon, ericsnowcurrently, ncoghlan, warsaw and iritkatriel as code owners June 23, 2023 03:53

bedevere-bot added the awaiting core review label Jun 23, 2023

markshannon reviewed Jun 23, 2023

View reviewed changes

Clean up some commnts

c2d4a33

carljm reviewed Jun 23, 2023

View reviewed changes

TeamSpen210 reviewed Jun 24, 2023

View reviewed changes

Python/flowgraph.c Show resolved Hide resolved

tekknolagi mentioned this pull request Jun 27, 2023

Make boolean conversion explicit tekknolagi/skybison#516

Open

brettcannon removed their request for review June 27, 2023 22:54

Add cast

7638b76

Catch up with main

6b4a598

brandtbucher merged commit 7b2d94d into python:main Jun 29, 2023
21 checks passed

bedevere-bot removed the awaiting core review label Jun 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-106008: Make implicit boolean conversions explicit #106003

GH-106008: Make implicit boolean conversions explicit #106003

brandtbucher commented Jun 23, 2023 •

edited by bedevere-bot

markshannon left a comment

markshannon Jun 23, 2023

markshannon Jun 23, 2023

markshannon Jun 23, 2023

brandtbucher Jun 23, 2023

markshannon Jun 26, 2023 •

edited

markshannon Jun 23, 2023

markshannon Jun 23, 2023

markshannon Jun 23, 2023

markshannon commented Jun 23, 2023

carljm Jun 23, 2023

brandtbucher commented Jun 29, 2023

gvanrossum commented Jul 4, 2023

markshannon commented Jul 4, 2023

GH-106008: Make implicit boolean conversions explicit #106003

GH-106008: Make implicit boolean conversions explicit #106003

Conversation

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

markshannon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markshannon Jun 26, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markshannon commented Jun 23, 2023

Choose a reason for hiding this comment

brandtbucher commented Jun 29, 2023

gvanrossum commented Jul 4, 2023

markshannon commented Jul 4, 2023

brandtbucher commented Jun 23, 2023 •

edited by bedevere-bot

markshannon Jun 26, 2023 •

edited