Aggressively try to use Integers for arithmetic #1563
Conversation
p-bakker
added
Potentially Breaking Change
|
Afraid I'm not able to try this out in the environment where I use Rhino, but I'm OK if the Rhino integration would break if it wrongfully expects Doubles, instead of Numbers... Do wonder about whether this PR is in any way related to #350 or (parts of) #717 & #507, in the sense of whether those cases are (partly) resolved/changes by this PR or whether those cases give hints for possible further optimizations in the int/double area Specifically for #350 I think this PR will just result in Java receiving an int more often, correct? I think #350 can just be closed as Won't Fix, as Java integration code should not expect Doubles, but Numbers. Agree? Also a question: in #350 (comment) you mentioned there being an integer optimization on optLevel 9. Is that (still?) really the case? I never found any code in Rhino that does anything different between optLevel 1 and 9 |
|
sorry.... |
|
@gbrail spend some time to give this a try by applying to core-js. I had one issue to fix - my Reflect/Proxy impl (see #1431) uses instanceof Double at two places in NativeReflect. After fixing that all my tests are running fine. Maybe other products extending Rhino in some ways and also depending on the assumption that every numbers a double internally. But for me that is fine, i guess i know what happens and a have a large test suite to check the compatibility. Summary: i'm fine with this |
When doing math, try to do integer arithmetic when the arguments passed are Integer objects, rather than always falling back to floating-point math. This speeds up some benchmarks that rely heavily on integer arithmetic.
gbrail
force-pushed
the
integer-operations
branch
from
8634af5
to
4e1ae12
Compare
This PR changes some of the logic that does arithmetic using Number objects to try and do integer arithmetic when both operands are integers, rather than assuming that all arithmetic should be done using floating-point.
This also changes a few fundamental constants -- for example, Rhino has always defined "zero" as "0.0" but this PR makes it truly "0".
If Rhino embedders are assuming that all numbers coming back from Rhino are Double objects (rather than checking for Number and then getting either intValue() or doubleValue() as they need), then their code may break. I had to change a few test cases for this reason. If, on the other hand, embedders are working with Number, there should not be an issue.
Some math-heavy benchmarks, like some of the V8 and SunSpider benchmarks, run quite a bit faster with this change.
I would be interested to see if some people have code that breaks because of this, so I'm hoping that a few of you could try it out -- there are likely other optimizations along these lines that are possible if it works.