1068605 - IonMonkey: Implement Random recover instruction

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Nicolas B. Pierron [:nbp]]

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

Comment 1

[justin willeman]

This is my first bug to work on, I want to know how i can help, and what you will need me to do?

Comment 2

[tejdeepg]

Would like to work on this bug if Justin is working on something else. Please assign it to me in that case. Thanks

Comment 3

[Nicolas B. Pierron [:nbp]]

Bugs are assigned to the first person which is making a patch for it, because we do not want anybody to keep a bug on hold with no modifications.

Comment 4

[Gaurav Rai [:gaurav0x]]

I'll work on this bug as it seems no one is working on it. Help me to start

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Gaurav Rai [:gaurav0x] from comment #4)
> I'll work on this bug as it seems no one is working on it. Help me to start

See Bug 1003801 comment 0 and other previous similar bugs (there are links in bug 1003801). If you get blocked at some point, don't hesitate to show up in #jsapi or #ionmonkey on our irc, or to post a work in progress patch here and request feedback from :nbp!

Comment 6

[Sodan [:sodan]]

Attachment: Implement recover instructions for Random.

Can you make a first feedback please? I don't know to test my patch.

Comment 7

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8515648 [details] [diff] [review]
Implement recover instructions for Random.

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

::: js/src/jit/Recover.cpp
@@ +1206,5 @@
>      return true;
>  }
> +
> +bool
> +RResumePoint::recover(JSContext *cx, SnapshotIterator &iter) const

This function already exists,m there is no need to duplicate it unless you want to break your compiler.

@@ +1227,5 @@
> +RRandom::recover(JSContext *cx, SnapshotIterator &iter) const
> +{
> +    RootedValue result(cx);
> +
> +    iter.storeInstructionResult(result);

?!

You are supposed to initialize result before storing it.

::: js/src/jit/Recover.h
@@ +57,5 @@
>      _(NewDerivedTypedObject)                    \
>      _(CreateThisWithTemplate)                   \
>      _(ObjectState)                              \
> +    _(ArrayState)                               \
> +    _(Random)

Please, follow the order defined in Bug 1003801.

@@ +625,5 @@
>  
>      bool recover(JSContext *cx, SnapshotIterator &iter) const;
>  };
>  
> +class RRandom MOZ_FINAL : public RInstruction

Same thing here.

@@ +628,5 @@
>  
> +class RRandom MOZ_FINAL : public RInstruction
> +{
> +  private:
> +    bool isFloatOperation_;

This is unlikely to be needed for Random.

::: js/src/jsmath.cpp
@@ +809,4 @@
>  js::math_random(JSContext *cx, unsigned argc, Value *vp)
>  {
>      CallArgs args = CallArgsFromVp(argc, vp);
> +

nit: no need for extra spaces.

Comment 8

[devillers.nicolas]

Attachment: recover-instruction-random.patch

I started working on this issue by mistake before seeing the bug was already open. Here is a patch, for review. I didn't implement the test case, I'm not sure what we want/can check in such case. Should the test case just check if the random value is a decimal number between 0 and 1 ?

Comment 9

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8519490 [details] [diff] [review]
recover-instruction-random.patch

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

::: js/src/jit/MIR.h
@@ +5471,5 @@
>  
> +    bool writeRecoverData(CompactBufferWriter &writer) const;
> +       bool canRecoverOnBailout() const {
> +           return true;
> +       }

nit: Fix indentation issues.

::: js/src/jit/Recover.cpp
@@ +846,5 @@
>  
>  bool
> +MRandom::writeRecoverData(CompactBufferWriter &writer) const
> +{
> +	MOZ_ASSERT(canRecoverOnBailout());

nit: same here

@@ +858,5 @@
> +
> +bool
> +RRandom::recover(JSContext *cx, SnapshotIterator &iter) const
> +{
> +    RootedValue result(cx);

Use a Value, and not a RootedValue, because of the following:

<nbp> can js::math_random_no_outparam gc?
<mrgiggles> No, |float64 js::math_random_no_outparam(JSContext*)| cannot GC

Comment 10

[devillers.nicolas]

Attachment: random-recover-patch-v1.1.patch

The test case for this patch is wrong, didn't figure out the issue yet.

Comment 11

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8523601 [details] [diff] [review]
random-recover-patch-v1.1.patch

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

::: js/src/jit-test/tests/ion/dce-with-rinstructions.js
@@ +377,5 @@
> +var uceFault_random = eval(uneval(uceFault).replace('uceFault', 'uceFault_random'));
> +function rrandom(i) {
> +    var x = Math.ceil(Math.random() + i);
> +    if (uceFault_random(i) || uceFault_random(i))
> +        assertEq(x, i+1);

I do not think this test matters as Random is not supposed to be predictable.  On the other hand, it would be interesting to use what Quentin is working on (Bug 1092544), in order to assert that we are indeed recovering the instruction.

::: js/src/jit/Recover.cpp
@@ +873,5 @@
> +MRandom::writeRecoverData(CompactBufferWriter &writer) const
> +{
> +    MOZ_ASSERT(canRecoverOnBailout());
> +    writer.writeUnsigned(uint32_t(RInstruction::Recover_Random));
> +    writer.writeByte(type() == MIRType_Float32);

When can the type() be equal to MIRType_Float32.

@@ +878,5 @@
> +    return true;
> +}
> +
> +RRandom::RRandom(CompactBufferReader &reader)
> +{ }

When are you reading the extra bit that you wrote? (if this is needed)

Comment 12

[Taahir Ahmed]

Attachment: random-recover-2016-08-18.patch

It looks like this issue has been dead for a long while, so I will throw another patch into the ring.

Comment 13

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8782745 [details] [diff] [review]
random-recover-2016-08-18.patch

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

Nice work, this patch looks good. :)
Fix the following nits, submit a new patch and request a review.

::: js/src/jit-test/tests/ion/dce-with-rinstructions.js
@@ +1254,5 @@
> +function rrandom_number(i) {
> +    var x = Math.random();
> +    if (uceFault_random_number(i) || uceFault_random_number(i)) {
> +        var y = Math.random();
> +        // No assertion, because random() isn't repeatable

nit: Use setRNGState in debug builds to re-initialize the seed before calling Math.random.  Pseudo-random generator are deterministic and repeatable.  Thus you can as well assert here.

@@ +1270,5 @@
> +    if (uceFault_random_object(i) || uceFault_random_object(i)) {
> +        var y = Math.random();
> +        // No assertion, because random() isn't repeatable
> +    }
> +    assertRecoveredOnBailout(x, false);

Why isn't this true?!

nit: This test case sounds useless as Math.random function does not take `o` as an input.

::: js/src/jit/MIR.h
@@ +6547,5 @@
>  
> +    MOZ_MUST_USE bool writeRecoverData(CompactBufferWriter& writer) const override;
> +
> +    bool canRecoverOnBailout() const override {
> +        return true;

nit: `return false;` if the macro JS_MORE_DETERMINISTIC is set.  Recover instructions are changing the order of evaluation, thus changing the expected result for a given seed, compared to baseline/interpreter execution.

Comment 14

[Taahir Ahmed]

> ::: js/src/jit-test/tests/ion/dce-with-rinstructions.js
> @@ +1254,5 @@
> > +function rrandom_number(i) {
> > +    var x = Math.random();
> > +    if (uceFault_random_number(i) || uceFault_random_number(i)) {
> > +        var y = Math.random();
> > +        // No assertion, because random() isn't repeatable
> 
> nit: Use setRNGState in debug builds to re-initialize the seed before
> calling Math.random.  Pseudo-random generator are deterministic and
> repeatable.  Thus you can as well assert here.

I can see how to call setRNGState from JS, but how do I detect a debug build from JS?

> @@ +1270,5 @@
> > +    if (uceFault_random_object(i) || uceFault_random_object(i)) {
> > +        var y = Math.random();
> > +        // No assertion, because random() isn't repeatable
> > +    }
> > +    assertRecoveredOnBailout(x, false);
> 
> Why isn't this true?!
>
> nit: This test case sounds useless as Math.random function does not take `o`
> as an input.

To be honest, this was a bit of cargo cult testing. When I wrote it, I didn't understand why some of the tests had an *object* version and some didn't.

You're right, it is useless.

Comment 15

[Taahir Ahmed]

Attachment: random-recover-2016-08-19.patch

Feedback applied.

Comment 16

[Nicolas B. Pierron [:nbp]]

(In reply to Taahir Ahmed from comment #14)
> > ::: js/src/jit-test/tests/ion/dce-with-rinstructions.js
> > @@ +1254,5 @@
> > > +function rrandom_number(i) {
> > > +    var x = Math.random();
> > > +    if (uceFault_random_number(i) || uceFault_random_number(i)) {
> > > +        var y = Math.random();
> > > +        // No assertion, because random() isn't repeatable
> > 
> > nit: Use setRNGState in debug builds to re-initialize the seed before
> > calling Math.random.  Pseudo-random generator are deterministic and
> > repeatable.  Thus you can as well assert here.
> 
> I can see how to call setRNGState from JS, but how do I detect a debug build
> from JS?

We have other tests which already have this function, and if the function is not present, then this means you are using an opt-build.

This function is exposed by the builtin/TestingFunctions.cpp to JS global.

Comment 17

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8783065 [details] [diff] [review]
random-recover-2016-08-19.patch

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

Nice work!

I presume you do not yet have commit access, thus you might need someone to take this patch and merge it for you.
For that we would need a patch, with a subject and your name as the author ;)

You can read the following for instructions:
https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 18

[Taahir Ahmed]

Attachment: 0001-Bug-1068605-Add-recovery-instruction-for-MRandom-r-n.patch

Patch (hopefully) suitable for checkin.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8785467 [details] [diff] [review]
0001-Bug-1068605-Add-recovery-instruction-for-MRandom-r-n.patch

For future reference, please use the checkin-needed bug keyword when your patch is ready to land. It works better with the automated bug marking tools.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d5bd7acc2f86
Add recovery instruction for MRandom. r=nbp

Comment 21

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

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