936740 - Inline Math.ceil()

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Michael Bebenita [:mbx]]

Attachment: 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)

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

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

[Jan de Mooij [:jandem]]

(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 :)

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Inline ceil in some cases

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 :)

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 8345331 [details] [diff] [review]
Inline ceil in some cases

Review of attachment 8345331 [details] [diff] [review]:
-----------------------------------------------------------------

Awesome, thanks!

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

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

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e3e21c3ada8c

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Missing cases

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

[Jan de Mooij [:jandem]]

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.

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

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

Comment 10

[Benjamin Bouvier [:bbouvier] (inactive)]

Try looks good (orange is unrelated).

https://hg.mozilla.org/integration/mozilla-inbound/rev/fefba9d06e03

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/fefba9d06e03