<< Back to previous view

[CLJS-370] Incorrect behavior of integer? for integral floating point expressions Created: 03/Sep/12  Updated: 27/Jul/13  Resolved: 05/Sep/12

Status: Closed
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: patch, patch,

Attachments: Text File CLJS-370-v1.patch    
Patch: Code and Test

 Description   

This includes infinity, exponential expressions, etc.

See: http://stackoverflow.com/questions/3885817/how-to-check-if-a-number-is-float-or-integer



 Comments   
Comment by David Nolen [ 03/Sep/12 4:26 PM ]

changing the behavior of integer? outside of a more comprehensive numeric discussion is low priority. The current implementation is already suboptimal in its coercion of numbers to strings.

Comment by Brandon Bloom [ 03/Sep/12 5:31 PM ]

I'm not changing the behavior of integer?, I'm fixing it. Also, regarding the suboptimal nature of string coercion, the following benchmark seems to show this isn't an issue at all:

Benchmarking with V8
[n 5], (old-integer? n), 1000000 runs, 0 msecs
[n 5.5], (old-integer? n), 1000000 runs, 0 msecs
[n 5.0E300], (old-integer? n), 1000000 runs, 0 msecs
[n ""], (old-integer? n), 1000000 runs, 0 msecs
[n 5], (integer? n), 1000000 runs, 0 msecs
[n 5.5], (integer? n), 1000000 runs, 0 msecs
[n 5.0E300], (integer? n), 1000000 runs, 0 msecs
[n ""], (integer? n), 1000000 runs, 0 msecs


Benchmarking with SpiderMonkey
[n 5], (old-integer? n), 1000000 runs, 0 msecs
[n 5.5], (old-integer? n), 1000000 runs, 0 msecs
[n 5.0E300], (old-integer? n), 1000000 runs, 0 msecs
[n ""], (old-integer? n), 1000000 runs, 0 msecs
[n 5], (integer? n), 1000000 runs, 0 msecs
[n 5.5], (integer? n), 1000000 runs, 0 msecs
[n 5.0E300], (integer? n), 1000000 runs, 0 msecs
[n ""], (integer? n), 1000000 runs, 0 msecs


Benchmarking with JavaScriptCore
[n 5], (old-integer? n), 1000000 runs, 0 msecs
[n 5.5], (old-integer? n), 1000000 runs, 0 msecs
[n 5.0E300], (old-integer? n), 1000000 runs, 0 msecs
[n ""], (old-integer? n), 1000000 runs, 0 msecs
[n 5], (integer? n), 1000000 runs, 0 msecs
[n 5.5], (integer? n), 1000000 runs, 0 msecs
[n 5.0E300], (integer? n), 1000000 runs, 0 msecs
[n ""], (integer? n), 1000000 runs, 0 msecs
Comment by David Nolen [ 04/Sep/12 10:02 AM ]

Those benchmarks are not revealing anything. Advanced compilation is eliminating that code. Try with :simple or :whitespace.

Comment by Brandon Bloom [ 04/Sep/12 11:13 PM ]

sigh I've once again shown my profiling ineptitude. Here's results running with whitespace optimizations:

Benchmarking with V8
[n 5], (old-integer? n), 1000000 runs, 924 msecs
[n 5.5], (old-integer? n), 1000000 runs, 1097 msecs
[n 5.0E300], (old-integer? n), 1000000 runs, 1009 msecs
[n ""], (old-integer? n), 1000000 runs, 77 msecs
[n 5], (integer? n), 1000000 runs, 210 msecs
[n 5.5], (integer? n), 1000000 runs, 556 msecs
[n 5.0E300], (integer? n), 1000000 runs, 731 msecs
[n ""], (integer? n), 1000000 runs, 78 msecs


Benchmarking with SpiderMonkey
[n 5], (old-integer? n), 1000000 runs, 236 msecs
[n 5.5], (old-integer? n), 1000000 runs, 362 msecs
[n 5.0E300], (old-integer? n), 1000000 runs, 2885 msecs
[n ""], (old-integer? n), 1000000 runs, 94 msecs
[n 5], (integer? n), 1000000 runs, 216 msecs
[n 5.5], (integer? n), 1000000 runs, 230 msecs
[n 5.0E300], (integer? n), 1000000 runs, 1360 msecs
[n ""], (integer? n), 1000000 runs, 98 msecs


Benchmarking with JavaScriptCore
[n 5], (old-integer? n), 1000000 runs, 471 msecs
[n 5.5], (old-integer? n), 1000000 runs, 467 msecs
[n 5.0E300], (old-integer? n), 1000000 runs, 771 msecs
[n ""], (old-integer? n), 1000000 runs, 212 msecs
[n 5], (integer? n), 1000000 runs, 882 msecs
[n 5.5], (integer? n), 1000000 runs, 756 msecs
[n 5.0E300], (integer? n), 1000000 runs, 851 msecs
[n ""], (integer? n), 1000000 runs, 213 msecs

Seems like a win on V8 & SpiderMonkey, but a small loss on JavaScriptCore.

Comment by David Nolen [ 04/Sep/12 11:40 PM ]

The slow down on JSC gives me pause. WebKit is ubiquitous in the mobile space.

I'm also not convinced we need to handle Infinity or NaN (though others may differ on that point). Perhaps we can in some debug mode intelligently detect and error out precisely where these kinds of mistakes may occur.

Comment by Brandon Bloom [ 05/Sep/12 1:19 AM ]

It seems like a bad precedent to favor micro-optimization over correctness when speed is easily obtainable through platform interop.

Infinity, NaN, and floating point exponentiation expressions are not integers. I changed the function's behavior to match JVM Clojure's. We're going to need a reliable integer predicate if we want to implement ratios and other compound numeric types.

If you need need a fast predicate for numbers, the number? predicate only checks type. It's behavior also matches JVM Clojure's: it accepts infinity, NaN, etc. If you need a fast integer test, but insist that edge cases like 5e300 are exceptional and ignorable, then you can micro-optimize by calling the necessary javascript primitives yourself.

Comment by David Nolen [ 05/Sep/12 9:55 AM ]

On my machine your patch is slower across all JS engines w/ JSC faring the worst at nearly 2X. I do agree that the behavior is better. However moving forward this means that even simple functions like even? are 100X slower than their Clojure JVM counterparts. There's nothing "micro-optimization" about 2 orders of magnitude.

Comment by David Nolen [ 05/Sep/12 10:06 AM ]

Oops after a lot more testing and playing around with your patch I turned out to be very wrong! It looks your patch + a boolean type hint is a win across the board on all engines, both in terms of performance and correctness.

Comment by David Nolen [ 05/Sep/12 10:07 AM ]

fixed, http://github.com/clojure/clojurescript/commit/3c210d9430b30ffeac9600ae4851e1c347c2cced

Comment by Brandon Bloom [ 05/Sep/12 11:40 AM ]

Heh. Performance is hard

Generated at Wed Oct 22 09:16:41 CDT 2014 using JIRA 4.4#649-r158309.