Improve test.check ClojureScript's random number generator speed

Description

test.check's random number ClojureScript Implementation has a slow path in goog.math.Long's multiply method. The conditional checks (https://github.com/google/closure-library/blob/master/closure/goog/math/long.js#L683-L711) don't affect the correctness of the implementation. These conditionals instead add extra branching and allocation overhead.

This patch ports the goog.math.Long's multiply over to ClojureScript with the conditionals removed. My benchmarking (benchmark code here https://gist.github.com/spinningtopsofdoom/7fb7fbe4ae7de159922a79e6f3f3e730) saw around a 2X speedup of Long generation

I've ported of Google Closure Libraries test of the goog.math.Long multiply method to ClojureScript in this gist (https://gist.github.com/spinningtopsofdoom/e479bf6f3485cd85ba979b4e5562057a).

Environment

None

Activity

Show:
gfredericks
October 27, 2017, 12:38 PM

This is a good catch, thanks!

This kind of change is a good candidate for a property-based test (comparing your implementation and .multiply to check that they return the same values) – I can wait for you to add that to the patch if you'd like, or apply the patch and write it myself.

Peter Schuck
October 27, 2017, 3:23 PM

I can add that to the patch. Since .multiply is the current standard would this the property based test would need to use the .multiply based PRNG? Also where would you like this test to be located?

Peter Schuck
October 27, 2017, 4:30 PM

The new patch added the test to clojure.test.check.test and used the new test.check multiply generator.

gfredericks
November 2, 2017, 12:19 PM

Thanks, this looks good. I'm just trying to reproduce the speedup (which is taking me days just due to being busy) and then will apply the patch.

gfredericks
November 4, 2017, 5:02 PM

Applied on master, thanks!

Completed

Assignee

gfredericks

Reporter

Peter Schuck

Labels

Approval

None

Patch

Code

Priority

Minor