1036163 - IonMonkey: Implement Hypot recover instruction

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[devillers.nicolas]

User Agent: Mozilla/5.0 (X11; Linux i686 (x86_64)) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/32.0.1700.14 Safari/537.36

Comment 1

[devillers.nicolas]

I would like to work on this bug.

Comment 2

[Nicolas B. Pierron [:nbp | PTO until 11/11]]

Nicolas, did you made any progress on this patch?

Comment 3

[devillers.nicolas]

No progress yet, but still planning to work on it.

Comment 4

[Nicolas B. Pierron [:nbp | PTO until 11/11]]

Implement RHypot in js/src/jit/Recover.cpp.
See Bug 1003801 comment 0 for explanation.

Comment 5

[Jarda]

Based on the discussion in Bug 1092547, I would like fix this.

Comment 6

[Jarda]

I am a bit stuck at the moment. I am not clear about the number of arguments to, while writing a recover function.

I understand that a new class RHypot needs to be defined and functions writeRecoverData and canRecoverOnBailout need to be added to MHypot class. Also, the Math.hypot can take arbitrary number of arguments - i.e. it can be called like Math.hypot(1,2,3,4), Math.hypot(3,4) and so on. If I understand it correctly, each argument is saved after each other to a Snapshot and then they can be retrieved by SnapshotIterator::read function. So I suppose that when writing RHypot::recover function, I would need to store those arguments in an array-like structure, AutoValueVector seems to my current candidate, and then pass it js::math_hypot() and possibly creating js::math_hypot_handle(). I would guess that MHypot::writeRecoverData should write the number of arguments so that it is known, how many arguments are going to be stored in the array-like structure in RHypot::recover. 

So far, so good :) I do not understand, why MHypot is derived from MBinaryInstruction, as the number of operands is variable, therefore I would suggest to derive from MVariadicInstruction instead.

Also, I did a bit of debugging, and studied how arguments are added to Snapshots and saw how a MHypot is added created. According to IonBuilder::inlineMathHypot, a MHypot is created only if the number of arguments is 2, otherwise, if I understand it correctly, it is some other execution path is used and the function is recalculated using the interpreter again. The limitation of number of arguments to 2 is probably the reason, why MHypot is a BinaryOperation.

So if my reasoning is correct, could you clarify, why the "inlining" of hypot is done only in case of two arguments? Is it performance/size of snapshot balance?

Comment 7

[Nicolas B. Pierron [:nbp | PTO until 11/11]]

(In reply to Jarda from comment #6)
> I am a bit stuck at the moment. I am not clear about the number of arguments
> to, while writing a recover function.

I think you should first focus on 2 arguments case for the moment.  At least to finish this issue first ;)

> I understand that a new class RHypot needs to be defined and functions
> writeRecoverData and canRecoverOnBailout need to be added to MHypot class.
> Also, the Math.hypot can take arbitrary number of arguments - i.e. it can be
> called like Math.hypot(1,2,3,4), Math.hypot(3,4) and so on. If I understand
> it correctly, each argument is saved after each other to a Snapshot and then
> they can be retrieved by SnapshotIterator::read function. So I suppose that
> when writing RHypot::recover function, I would need to store those arguments
> in an array-like structure, AutoValueVector seems to my current candidate,
> and then pass it js::math_hypot() and possibly creating
> js::math_hypot_handle(). I would guess that MHypot::writeRecoverData should
> write the number of arguments so that it is known, how many arguments are
> going to be stored in the array-like structure in RHypot::recover. 

The problem of calling js::math_hypot() is that it has to be a valid stack, and in most of the case we do not care about the "this" and "fun", and it does not make sense either to fill them for nothing.

So I think the easiest might be to go for the js::math_hypot_handle() for the moment.

> So far, so good :) I do not understand, why MHypot is derived from
> MBinaryInstruction, as the number of operands is variable, therefore I would
> suggest to derive from MVariadicInstruction instead.
> 
> Also, I did a bit of debugging, and studied how arguments are added to
> Snapshots and saw how a MHypot is added created. According to
> IonBuilder::inlineMathHypot, a MHypot is created only if the number of
> arguments is 2, otherwise, if I understand it correctly, it is some other
> execution path is used and the function is recalculated using the
> interpreter again. The limitation of number of arguments to 2 is probably
> the reason, why MHypot is a BinaryOperation.
> 
> So if my reasoning is correct, could you clarify, why the "inlining" of
> hypot is done only in case of two arguments? Is it performance/size of
> snapshot balance?

I think this might be just an oversight of the hypot function.  Then I do not think we should expect these functions being called explicitly with more than 4 arguments, unless we use it as such:

  var myError = Math.hypot.apply(null, myDiff)

If you are interested, feel free to open a bug and work on it next ;)

Comment 8

[Jarda]

Attachment: rhypot.patch for two arguments hypot

Hi, I created a patch for the two-argument hypot. 
Please, review.

Jarda

Comment 9

[Nicolas B. Pierron [:nbp | PTO until 11/11]]

Comment on attachment 8519745 [details] [diff] [review]
rhypot.patch for two arguments hypot

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

So far this looks good.

::: js/src/jit-test/tests/ion/dce-with-rinstructions.js
@@ +987,5 @@
>  }
>  
> +var uceFault_hypot_number = eval(uneval(uceFault).replace('uceFault', 'uceFault_hypot_number'));
> +function rhypot_number(i) {
> +    var x = Math.hypot(i,i+1,i+2 );

This test case does not even check the MIR Instruction, which only expect 2 arguments.

@@ +1002,5 @@
> +    var t2 = i+2;
> +    var o0 = { valueOf: function () { return t0; } };
> +    var o1 = { valueOf: function () { return t1; } };
> +    var o2 = { valueOf: function () { return t2; } };
> +    var x = Math.hypot(o0, o1, o2);

same here.

::: js/src/jit/Recover.cpp
@@ +861,5 @@
> +    RootedValue x(cx, iter.read());
> +    RootedValue y(cx, iter.read());
> +    RootedValue result(cx);
> +
> +    if(!js::math_hypot_handle(cx, x, y, &result))

Have a Look at AutoValueVector.

http://dxr.mozilla.org/mozilla-central/source/js/src/jsapi.h#441

::: js/src/jsmath.cpp
@@ +1373,5 @@
>      return hypot(x, y);
>  }
>  
>  bool
> +js::math_hypot_handle(JSContext *cx, HandleValue in1, HandleValue in2, MutableHandleValue res)

Any reason to extract this function, and not use the one below in the RHypot::recover function?

@@ +1431,5 @@
> +js::math_hypot(JSContext *cx, unsigned argc, Value *vp)
> +{
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +
> +    HandleValueArray arr(args);

usually we do not create Handle explicitly, and create them implicitly, and this is the reason why we have a MOZ_IMPLICIT on the copy constructor of HandleValueArray.

@@ +1433,5 @@
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +
> +    HandleValueArray arr(args);
> +
> +    return math_hypot_handle(cx, arr, args.rval());

replace "arr" by "args", by keeping the same argument type in the function declaration.  This would call the MOZ_IMPLICIT copy constructor for you.

Comment 10

[Jarda]

(In reply to Nicolas B. Pierron [:nbp] from comment #9)
> Comment on attachment 8519745 [details] [diff] [review]
> rhypot.patch for two arguments hypot
> 
> Review of attachment 8519745 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> So far this looks good.
> 
> ::: js/src/jit-test/tests/ion/dce-with-rinstructions.js
> @@ +987,5 @@
> >  }
> >  
> > +var uceFault_hypot_number = eval(uneval(uceFault).replace('uceFault', 'uceFault_hypot_number'));
> > +function rhypot_number(i) {
> > +    var x = Math.hypot(i,i+1,i+2 );
> 
> This test case does not even check the MIR Instruction, which only expect 2
> arguments.
> 
> @@ +1002,5 @@
> > +    var t2 = i+2;
> > +    var o0 = { valueOf: function () { return t0; } };
> > +    var o1 = { valueOf: function () { return t1; } };
> > +    var o2 = { valueOf: function () { return t2; } };
> > +    var x = Math.hypot(o0, o1, o2);
> 
> same here.
> 

I realized this also, but some time after submission. Will be fixed in the next patch.

> ::: js/src/jit/Recover.cpp
> @@ +861,5 @@
> > +    RootedValue x(cx, iter.read());
> > +    RootedValue y(cx, iter.read());
> > +    RootedValue result(cx);
> > +
> > +    if(!js::math_hypot_handle(cx, x, y, &result))
> 
> Have a Look at AutoValueVector.
> 
> http://dxr.mozilla.org/mozilla-central/source/js/src/jsapi.h#441
> 

I considered using a vector for two arguments case an overkill. I'll check the possibilities of using AutoValueVector.


> ::: js/src/jsmath.cpp
> @@ +1373,5 @@
> >      return hypot(x, y);
> >  }
> >  
> >  bool
> > +js::math_hypot_handle(JSContext *cx, HandleValue in1, HandleValue in2, MutableHandleValue res)
> 
> Any reason to extract this function, and not use the one below in the
> RHypot::recover function?
> 
> @@ +1431,5 @@
> > +js::math_hypot(JSContext *cx, unsigned argc, Value *vp)
> > +{
> > +    CallArgs args = CallArgsFromVp(argc, vp);
> > +
> > +    HandleValueArray arr(args);
> 

Again, I did not mean to use the vector, therefore I created a new function taking only two arguments.

> usually we do not create Handle explicitly, and create them implicitly, and
> this is the reason why we have a MOZ_IMPLICIT on the copy constructor of
> HandleValueArray.
> 
> @@ +1433,5 @@
> > +    CallArgs args = CallArgsFromVp(argc, vp);
> > +
> > +    HandleValueArray arr(args);
> > +
> > +    return math_hypot_handle(cx, arr, args.rval());
> 
> replace "arr" by "args", by keeping the same argument type in the function
> declaration.  This would call the MOZ_IMPLICIT copy constructor for you.

I am not a fan of implicit conversions, that is why I created the HandleValueArray explicitly. What are the reasons for using implicit conversion on Handle?

Comment 11

[Jarda]

Attachment: rhypot2.patch with review comments implemented

Review comments were implemented. Please, have a look at a new version.

Comment 12

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8520505 [details] [diff] [review]
rhypot2.patch with review comments implemented

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

Hi Jarda, thanks for this patch. When asking for a review next time, please choose the "?" in the review dropdown list, then type the name of your reviewer or his nickname (here, :nbp will work) in the text field. Thanks!

Comment 13

[Nicolas B. Pierron [:nbp | PTO until 11/11]]

Comment on attachment 8520505 [details] [diff] [review]
rhypot2.patch with review comments implemented

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

This looks good, still one last modification.

::: js/src/jit/Recover.cpp
@@ +860,5 @@
> +{
> +    JS::AutoValueVector vec(cx);
> +
> +    // currently, only 2 args can be saved in MIR
> +    vec.resize(2);

Use reserve instead of resize, and check for the return value.

::: js/src/jsmath.cpp
@@ +1386,5 @@
>              return false;
>  
>          double result = ecmaHypot(x, y);
> +        res.setNumber(result);
> +        return true;    

nit: Remove trailing white space.

Comment 14

[Jarda]

Attachment: rhypot3.patch with another review comments implemented

Implemented latest review comments. Please, have a look.
Thanks

Comment 15

[Nicolas B. Pierron [:nbp | PTO until 11/11]]

Comment on attachment 8521991 [details] [diff] [review]
rhypot3.patch with another review comments implemented

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

SpiderMonkey does not use exception for security reasons.  This means that any allocation which is failing needs to be checked and protected against.
If we reserve memory, then we have to check that the allocation did not fail before continuing.

::: js/src/jit-test/tests/ion/dce-with-rinstructions.js
@@ +990,5 @@
> +function rhypot_number(i) {
> +  var x = Math.hypot(i,i+1 );
> +  if (uceFault_hypot_number(i) || uceFault_hypot_number(i))
> +    assertEq(x, Math.sqrt(i*i+(i+1)*(i+1)));
> +  return i;

nit: all above functions are indented by 4, follow the same style.

::: js/src/jit/Recover.cpp
@@ +860,5 @@
> +{
> +    JS::AutoValueVector vec(cx);
> +
> +    // currently, only 2 args can be saved in MIR
> +    vec.reserve(2);

Check the return value of this function call and return false, if we are unable to reserve enough space for the vector.

@@ +862,5 @@
> +
> +    // currently, only 2 args can be saved in MIR
> +    vec.reserve(2);
> +    vec[0].set(iter.read());
> +    vec[1].set(iter.read());

Use "infallibleAppend" (as it is reserved before) instead of "set".

@@ +870,5 @@
> +    if(!js::math_hypot_handle(cx, vec, &result))
> +        return false;
> +
> +    iter.storeInstructionResult(result);
> +

nit: Remove this new-line.

::: js/src/jsmath.cpp
@@ +1424,5 @@
> +bool
> +js::math_hypot(JSContext *cx, unsigned argc, Value *vp)
> +{
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +

nit: remove this new-line.

Comment 16

[Jarda]

Attachment: rhypot4.patch with latest review comments implemented

Review comments were now carefully implemented. Hope it is better now. Please, have a look

Comment 17

[Jarda]

Attachment: rhypot4.patch

Latest review comments implemented. I am struggling with white space :)

Comment 18

[Nicolas B. Pierron [:nbp | PTO until 11/11]]

Comment on attachment 8522796 [details] [diff] [review]
rhypot4.patch

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

This looks good, I will push this patch to our try-server which will check if everything is correct by running the test suite with different configurations.

Comment 19

[Nicolas B. Pierron [:nbp | PTO until 11/11]]

The following link reports the result of our try server, if all is green, then we can proceed and add these changes to mozilla-inbound.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=81d6ca3fe5c6

Comment 20

[Jarda]

(In reply to Nicolas B. Pierron [:nbp] from comment #19)
> The following link reports the result of our try server, if all is green,
> then we can proceed and add these changes to mozilla-inbound.
> 
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=81d6ca3fe5c6

Just checked the try server and 13 test failed. After some analysis, it turned out, that there is a typo in RHypot::recover: On line 

if (!vec.reserve(2));

there should be no semicolon. I'll provide an update.

Comment 21

[Jarda]

Attachment: rhypot4.patch with typo fixed

Typo fixed

Comment 22

[Nicolas B. Pierron [:nbp | PTO until 11/11]]

Comment on attachment 8524561 [details] [diff] [review]
rhypot4.patch with typo fixed

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

Nice catch :)

Comment 23

[Nicolas B. Pierron [:nbp | PTO until 11/11]]

Comment on attachment 8524561 [details] [diff] [review]
rhypot4.patch with typo fixed

# User Jarda [[e-mail]]

In the future, do not add [] around your email address, only use <>.
We are converting commit from mercurial to git, and git is much more sensible to these kind of typos.

Comment 24

[Nicolas B. Pierron [:nbp | PTO until 11/11]]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e69b5e4a8ade

Comment 25

[Jarda]

(In reply to Nicolas B. Pierron [:nbp] from comment #23)
> Comment on attachment 8524561 [details] [diff] [review]
> rhypot4.patch with typo fixed
> 
> # User Jarda [[e-mail]]
> 
> In the future, do not add [] around your email address, only use <>.
> We are converting commit from mercurial to git, and git is much more
> sensible to these kind of typos.

OK, understood.

Comment 26

[Nicolas B. Pierron [:nbp | PTO until 11/11]]

Congratulation \o/

The following link is the link to the patch in mozilla-inbound, in a few hours, your patch would be available in Nightly versions of Firefox, and then it will follow the release cycle and be released to every Firefox 36 users :)

Is there any other bug that you want to continue on?
For example, I guess you might be interested for adding support for more than 2 arguments to MHypot.

try-push: see comment 24
https://hg.mozilla.org/integration/mozilla-inbound/rev/c7eefbcb8e13

Comment 27

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

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

Comment 28

[Jarda]

(In reply to Nicolas B. Pierron [:nbp] from comment #26)
> Congratulation \o/
> 
> The following link is the link to the patch in mozilla-inbound, in a few
> hours, your patch would be available in Nightly versions of Firefox, and
> then it will follow the release cycle and be released to every Firefox 36
> users :)
> 
> Is there any other bug that you want to continue on?
> For example, I guess you might be interested for adding support for more
> than 2 arguments to MHypot.
> 
> try-push: see comment 24
> https://hg.mozilla.org/integration/mozilla-inbound/rev/c7eefbcb8e13

I am glad I finally made it :) The extesion of MHypot to handle more arguments sounds good. I'll try to open a bug for it.