Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Boolean preallocation optimization #830

Merged
merged 6 commits into from Jun 9, 2018

Conversation

Projects
None yet
2 participants
@patiences
Copy link
Contributor

commented Jun 4, 2018

This PR improves execution time by using preallocated instances of True and False, never creating new Boolean objects.

Performance benchmark stats (test here):

Pre-change
Running test_booleans
Elapsed time: 85.63200672800303 sec
CPU process time: 0.0009250000000000091 sec

Running test_booleans
Elapsed time: 83.78228437301004 sec
CPU process time: 0.00013499999999999623 sec

Running test_booleans
Elapsed time: 80.98856862200773 sec
CPU process time: 0.00012000000000000899 sec

With this change
Running test_booleans
Elapsed time: 42.00830070799566 sec
CPU process time: 0.0008920000000000039 sec

Running test_booleans
Elapsed time: 41.94127046901849 sec
CPU process time: 0.0007800000000000029 sec

Running test_booleans
Elapsed time: 43.51021371400566 sec
CPU process time: 0.0009509999999999796 sec

@freakboy3742
Copy link
Member

left a comment

Ok, so it's a completely synthetic benchmark - but almost 50% speedup across the board?! Hot damn!! 🎉 🍾

I've dropped in one suggestion that might squeeze out a little bit more performance.

@@ -384,12 +384,12 @@ public static void initializeModule(java.lang.Class cls, java.util.Map<java.lang
while (true) {
org.python.Object next = iter.__next__();
if (!next.toBoolean()) {
return new org.python.types.Bool(false);
return org.python.types.Bool.getBool(false);

This comment has been minimized.

Copy link
@freakboy3742

freakboy3742 Jun 4, 2018

Member

In the case where you're referencing a constant, is there any reason to not just use the static member reference directly? Variable access should always be faster than a function call...

This comment has been minimized.

Copy link
@patiences

patiences Jun 6, 2018

Author Contributor

Ah, yes! Done (Here and many other places)

@patiences

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2018

With this change, things seem to be marginally better:

Running test_booleans
Elapsed time: 41.18798470598995 sec
CPU process time: 0.00010700000000002374 sec

Running test_booleans
Elapsed time: 41.623120495001785 sec
CPU process time: 0.00012199999999995548 sec

Running test_booleans
Elapsed time: 41.56958808301715 sec
CPU process time: 0.000154000000000043 sec

Running test_booleans
Elapsed time: 43.66924121600459 sec
CPU process time: 0.00013999999999997348 sec

@freakboy3742
Copy link
Member

left a comment

Looking good! I've flagged a couple more simplifications that might shave a little more performance out of things.

} else if (other instanceof org.python.types.Bool) {
return new org.python.types.Bool((((org.python.types.Bool) this).value ? 1 : 0) < (((org.python.types.Bool) other).value ? 1 : 0));
return org.python.types.Bool.getBool((((org.python.types.Bool) this).value ? 1 : 0) < (((org.python.types.Bool) other).value ? 1 : 0));

This comment has been minimized.

Copy link
@freakboy3742

freakboy3742 Jun 7, 2018

Member

Another tweak, but this expression can be simplified as well; if we're comparing two bools, there's no need to do a transition through integer logic to get back to a boolean.

This comment has been minimized.

Copy link
@patiences

patiences Jun 7, 2018

Author Contributor

I'm not sure we can use < directly on booleans here as that's not defined in Java, but other places in the file like == or | have been tweaked!

@@ -302,7 +311,7 @@ public boolean __setattr_null(java.lang.String name, org.python.Object value) {
return new org.python.types.Int(0);
} else {
if (org.Python.VERSION < 0x03060000) {
return new org.python.types.Bool(false);
return (org.python.types.Bool) FALSE;

This comment has been minimized.

Copy link
@freakboy3742

freakboy3742 Jun 7, 2018

Member

The cast shouldn't be needed here (and the constant should be fully qualified to avoid ambiguity)

This comment has been minimized.

Copy link
@patiences

patiences Jun 7, 2018

Author Contributor

Done!

@@ -413,33 +413,33 @@ public ByteArray(org.python.Object[] args, java.util.Map<java.lang.String, org.p
byte[] other_bytes = (byte[]) ((org.python.types.Bytes) other).value;
for (int i = 0; i < Math.min(this.value.length, other_bytes.length); i++) {
if (this.value[i] > other_bytes[i]) {
return new org.python.types.Bool(1);
return org.python.types.Bool.getBool(1);

This comment has been minimized.

Copy link
@freakboy3742

freakboy3742 Jun 7, 2018

Member

getBool(1) should have a very predictable output... :-)

This comment has been minimized.

Copy link
@patiences

patiences Jun 7, 2018

Author Contributor

Done, here and in other places :-)

@freakboy3742
Copy link
Member

left a comment

🎉 Fantastic!

@freakboy3742 freakboy3742 merged commit 8c37903 into beeware:master Jun 9, 2018

5 checks passed

beekeeper:0/beefore:javacheckstyle Java lint checks passed.
Details
beekeeper:0/beefore:pycodestyle Python lint checks passed.
Details
beekeeper:1/smoke-test Smoke build (Python 3.4) passed.
Details
beekeeper:2/full-test:py3.5 Python 3.5 tests passed.
Details
beekeeper:2/full-test:py3.6 Python 3.6 tests passed.
Details

@patiences patiences deleted the patiences:boolean-preallocation branch Jul 2, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.