Closed Bug 936740 Opened 6 years ago Closed 6 years ago

Inline Math.ceil()

Categories

(Core :: JavaScript Engine: JIT, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: mbx, Assigned: bbouvier)

References

Details

(Whiteboard: [Shumway] )

Attachments

(3 files)

Attached file ceil.js
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)
Whiteboard: [Shumway]
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.
(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)
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 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+
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]
Attached patch Missing casesSplinter Review
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.
Assignee: nobody → benj
Status: NEW → ASSIGNED
Attachment #8375610 - Flags: review?(jdemooij)
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+
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
Whiteboard: [Shumway] [leave open] → [Shumway]
https://hg.mozilla.org/mozilla-central/rev/fefba9d06e03
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Depends on: 1000605
Depends on: 1010747
Depends on: 1122401
You need to log in before you can comment on or make changes to this bug.