[CLJS370] 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:  CLJS370v1.patch 
Patch:  Code and Test 
Description 
This includes infinity, exponential expressions, etc. See: http://stackoverflow.com/questions/3885817/howtocheckifanumberisfloatorinteger 
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], (oldinteger? n), 1000000 runs, 0 msecs [n 5.5], (oldinteger? n), 1000000 runs, 0 msecs [n 5.0E300], (oldinteger? n), 1000000 runs, 0 msecs [n ""], (oldinteger? 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], (oldinteger? n), 1000000 runs, 0 msecs [n 5.5], (oldinteger? n), 1000000 runs, 0 msecs [n 5.0E300], (oldinteger? n), 1000000 runs, 0 msecs [n ""], (oldinteger? 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], (oldinteger? n), 1000000 runs, 0 msecs [n 5.5], (oldinteger? n), 1000000 runs, 0 msecs [n 5.0E300], (oldinteger? n), 1000000 runs, 0 msecs [n ""], (oldinteger? 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], (oldinteger? n), 1000000 runs, 924 msecs [n 5.5], (oldinteger? n), 1000000 runs, 1097 msecs [n 5.0E300], (oldinteger? n), 1000000 runs, 1009 msecs [n ""], (oldinteger? 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], (oldinteger? n), 1000000 runs, 236 msecs [n 5.5], (oldinteger? n), 1000000 runs, 362 msecs [n 5.0E300], (oldinteger? n), 1000000 runs, 2885 msecs [n ""], (oldinteger? 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], (oldinteger? n), 1000000 runs, 471 msecs [n 5.5], (oldinteger? n), 1000000 runs, 467 msecs [n 5.0E300], (oldinteger? n), 1000000 runs, 771 msecs [n ""], (oldinteger? 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 microoptimization 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 microoptimize 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 "microoptimization" 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 