Closed
Bug 936740
Opened 11 years ago
Closed 11 years ago
Inline Math.ceil()
Categories
(Core :: JavaScript Engine: JIT, defect)
Tracking
()
RESOLVED
FIXED
mozilla30
People
(Reporter: mbx, Assigned: bbouvier)
References
Details
(Whiteboard: [Shumway] )
Attachments
(3 files)
944 bytes,
application/x-javascript
|
Details | |
7.17 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
1.70 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
Math.ceil() is useful in various tight computational kernels and should be inlined. The self hosted JS version (possibly incomplete) is roughly 3.5x faster.
Measure: Math.ceil() Count: 1024 Elapsed: 2934.0000 (2.8652)
Measure: ceil() Count: 1024 Elapsed: 850.0000 (0.8301)
Reporter | ||
Updated•11 years ago
|
Whiteboard: [Shumway]
Assignee | ||
Comment 1•11 years ago
|
||
On my computer:
- with your ceil(): 538
- a local patch which only inlines the call to math_ceil_impl (without doing any inlining in assembly like MFloor does for Math.floor()): 305
There might still be room for improvement if we inline it directly in assembly.
Comment 2•11 years ago
|
||
(In reply to Benjamin Bouvier [:bbouvier] from comment #1)
> - a local patch which only inlines the call to math_ceil_impl (without doing
> any inlining in assembly like MFloor does for Math.floor()): 305
Do you happen to still have that patch? Inlining is faster but that's still much better than what we're doing now :)
Flags: needinfo?(benj)
Assignee | ||
Comment 3•11 years ago
|
||
Jan, here it is. It only inlines for the case where arg is a double and the return value is a double, which is hit in this test case. Feel free to review it if you think it's worth it :)
Inlining in the other case (input is a double and returned value is an int32) shouldn't be hard to do: some factoring would be needed to avoid too much code duplication with MFloor. I thought we could use the fact that for all non-integers x, ceil(x) == floor(x) + 1 (that still holds for +/- Infinity and NaN), but x being a MIRType_Double doesn't make it necessarily a non-int. I will work on that probably later, unless somebody beats me to it :)
Attachment #8345331 -
Flags: review?(jdemooij)
Flags: needinfo?(benj)
Comment 4•11 years ago
|
||
Comment on attachment 8345331 [details] [diff] [review]
Inline ceil in some cases
Review of attachment 8345331 [details] [diff] [review]:
-----------------------------------------------------------------
Awesome, thanks!
Attachment #8345331 -
Flags: review?(jdemooij) → review+
Assignee | ||
Comment 5•11 years ago
|
||
I set leave open as we'd like some asm inlining too for the other case.
https://hg.mozilla.org/integration/mozilla-inbound/rev/e3e21c3ada8c
Whiteboard: [Shumway] → [Shumway] [leave open]
Assignee | ||
Comment 7•11 years ago
|
||
When investigating the trigonometrics interpolations made for cos / sin in v8's math.js file, I saw that identity stating that Math.ceil == -Math.floor(-x). Fair enough, here's a free optimization for the last missing inlining case.
Comment 8•11 years ago
|
||
Comment on attachment 8375610 [details] [diff] [review]
Missing cases
Review of attachment 8375610 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good, but can you add a jit-test like this:
function ceil(x) { return Math.ceil(x); }
assertEq(ceil(1.1), 2);
assertEq(ceil(-1.1), -1);
assertEq(ceil(-3), -3);
assertEq(ceil(-0), -0);
We should Ion compile this function with --ion-eager and inline the call. Then the last call should trigger a bailout, right? Please verify this is what happens. r=me with that.
Attachment #8375610 -
Flags: review?(jdemooij) → review+
Assignee | ||
Comment 9•11 years ago
|
||
That's right, this is what's happening with your test case. I have also added cases for x == 0, since if Math.ceil is inlined as the Double -> Int32 variant (and thus uses the Floor trick), MFloor will bail out as it sees -0. Also added float32 tests for sanity.
https://tbpl.mozilla.org/?tree=Try&rev=085275ae0185
Assignee | ||
Comment 10•11 years ago
|
||
Try looks good (orange is unrelated).
https://hg.mozilla.org/integration/mozilla-inbound/rev/fefba9d06e03
Assignee | ||
Updated•11 years ago
|
Whiteboard: [Shumway] [leave open] → [Shumway]
Comment 11•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in
before you can comment on or make changes to this bug.
Description
•