Open Bug 1003801 Opened 5 years ago Updated 4 months ago

[meta] Recover all non-effectful instructions.

Categories

(Core :: JavaScript Engine: JIT, defect, P5)

defect

Tracking

()

People

(Reporter: nbp, Unassigned)

References

(Depends on 2 open bugs)

Details

(Keywords: leave-open, meta)

Attachments

(1 file)

Recover instructions are used to remove instructions from the normal execution path and to move them to the bailout path which is only executed when we are going back to baseline after failing the execution of the code produced by IonMonkey.

Bug 990106 part 4 (attachment 8408953 [details] [diff] [review]) highlight how we add support for MAdd, but there is a lot of efforts which needs to go on into doing the same for the rest of non-effectful instructions.

BitNot
BitAnd
BitOr
BitXor
Lsh
Rsh
Ursh
Add (done in Bug 990106)
Sub
Mul
Div
Mod
Not
Concat
StringLength
ArgumentsLength
Floor
Round
CharCodeAt
FromCharCode
Pow
PowHalf
MinMax
Abs
Sqrt
Atan2
Hypot
MathFunction
Random
StringSplit
RegExpTest
RegExpExec
RegExpReplace
StringReplace
TypeOf

ToDouble
ToFloat32
ToInt32
TruncateToInt32
ToString
ClampToUint8

All of these MDefinition should be easy to implement and are good candidates for "good first bug", that I am willing to mentor.

Before starting on any of these, make sure you have a working copy of mozilla-central, and that you are able to build and check the JavaScript Shell [1].  Also, have a look at iongraph [2] outputs when running the shell with the test case added for testing the removal of MAdd [3] with (--ion-parallel-compile=off).

I will open each bug on-demand as a dependence of this meta-bug to avoid flooding with bugmail.

[1] https://developer.mozilla.org/en-US/docs/SpiderMonkey/Build_Documentation
[2] https://developer.mozilla.org/en-US/docs/SpiderMonkey/Hacking_Tips#Using_IonMonkey_spew_%28JS_shell%29
[3] http://dxr.mozilla.org/mozilla-central/source/js/src/jit-test/tests/ion/dce-with-rinstructions.js?from=js/src/jit-test/tests/ion/dce-with-rinstructions.js#1
Depends on: 1003802
(Copy of the reply of a private email)

On Fri 02 May 2014 03:03:48 AM PDT, Inanc Seylan wrote:
> I would like to give the mentored bug
>
> https://bugzilla.mozilla.org/show_bug.cgi?id=1003802
>
> a go. I have looked at
>
> https://bugzilla.mozilla.org/show_bug.cgi?id=1003801#c0
>
> for an explanation. I now have a working copy of the mozilla-central and I
> can build and run the tests for the JavaScript shell. I do not have an
> idea on how to look at iongraph outputs. However, I ran
>
> ~/workspace/mozilla/mozilla-central/js/src/build-release/dist/bin/js
> dce-with-rinstructions.js
>
> and it did not output anything.

To look at the iongraph output, you will need a debug version of the JavaScript shell and to run

 IONFLAGS=logs ~/workspace/mozilla/mozilla-central/js/src/build-release/dist/bin/js dce-with-rinstructions.js

Running the shell this way, will produce a file named /tmp/ion.json which is used by iongraph to generate a dot output in png / pdf files.
You might want to take my pull-request (attachment 8415150 [details] [review]) if Sean (sstangl) did not merged it yet, to clearly distinguish the Recover Instructions.

> I have looked at
>
> http://dxr.mozilla.org/mozilla-central/source/js/src/jit/Recover.cpp
>
> I guess one needs to implement two classes, MInst and RInst for each
> instruction Inst on your list and you want to start with BitNot. But in
> order to understand the details of the code (e.g., other classes) in
> Recover.cpp, do I need to know the architecture of SpiderMonkey? For
> example, your explanation

Correct, but MBitNot should already be implemented in jit/MIR.h.
You need to know what this operation is doing, and you can find that either in the Interpreter (vm/Interpreter.cpp), or in the IonBuilder & Lowering & CodeGenerator (jit/IonBuilder.cpp, jit/Lowering.cpp, and jit/CodeGenerator.cpp).

> "Recover instructions are used to remove instructions from the normal
> execution path and to move them to the bailout path which is only executed
> when we are going back to baseline after failing the execution of the code
> produced by IonMonkey.||||"
>
> is a bit vague for me since I do not know the architecture. What is a
> bailout path?

We have 3 modes of executions in SpiderMonkey,
1. The Interpreter (slow, no assumption, limited optim)
2. Baseline (no assumption, generate special path for the different kind of inputs)
3. IonMonkey (make assumptions, generate special path for the known kind of inputs)

When an assumption made by IonMonkey no longer hold and if we are currently executing the code, we have to come back to Baseline.  This is what we call a "bailout".

> I think the most important question at this point is: how should I proceed?

Look at how the interpreter handle BitNot, then do the same for IonBuilder & Lowering & CodeGenerator.
Then you should have a clear overview of what should be done in RBitNot ;)
I'm trying to understand the logic in RAdd::recover in jit/Recover.cpp

In RAdd::recover in jit/Recover.cpp, js::AddValues is called with parameters of type RootedValue although js::AddValues in vm/Interpreter.cpp accepts parameters of type MutableHandleValue. Why is that not a problem?

Can we safely assume that each SnapshotIterator::read call will return us an operand of the operation Inst, whose RInst::recover method we are implementing?
(In reply to inanc.seylan from comment #2)
> I'm trying to understand the logic in RAdd::recover in jit/Recover.cpp
> 
> In RAdd::recover in jit/Recover.cpp, js::AddValues is called with parameters
> of type RootedValue although js::AddValues in vm/Interpreter.cpp accepts
> parameters of type MutableHandleValue. Why is that not a problem?

In order to have a precise GC, we need to index all values/objects which are followed by the GC.
To do so we use RootedValue / RootedObject, which is a linked list of values which are traversed by the GC.

These objects (Rooted*) can be implictly converted into an Handle* which is the equivalent of a reference.  The only difference with a reference is that ensure that we have rooted it previously.

A MutableHandle* is like a pointer to the Rooted, where we can change the content of the Rooted.

you can find more detail on our GC Rooting Guide[1].

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/GC_Rooting_Guide

> Can we safely assume that each SnapshotIterator::read call will return us an
> operand of the operation Inst, whose RInst::recover method we are
> implementing?

Yes, with the operands of the MInst.  The SnapshotIterator::read() reads the value of allocations (= where it is stored) encoded in the snapshot.  When a snapshot is encoded, each MInstruction which is flagged as RecoveredOnBailout would have all its operands allocations encoded sequentially in the snapshot.

When you use the SnapshotIterator::read() in a RInst::recover(), you are reading the value represented by the MDefinition operands of the MInst, in the same order as they are list in the MInst.
Depends on: 1008110
This image is produce by iongraph with support for recover instruction (as linked above):

I ran:
  IONFLAGS=logs ./build-debug/dist/bin/js --ion-parallel-compile=off --ion-eager jit-test/tests/ion/dce-with-rinstructions.js

then in iongraph directory:
  make clean png

This function correspond to the Float32 variant which has multiple additions in it, as you can see the additions appear in gray instead of being black as other instructions, because they are used on the bailout path, as resume points.
Depends on: 1008707
Depends on: 1009967
Depends on: 1009968
Depends on: 1010334
Depends on: 1010339
Depends on: 1011539
Depends on: 1011540
Depends on: 1011541
Depends on: 1012632
Depends on: 1013821
(Copy of the reply of an email)

On Tue 20 May 2014 04:53:28 PM PDT, Amol Mundayoor wrote:
> I have been trying to understand what Recover.cpp does along with the
> other files that were edited in the provided patch (IonAnalysis.cpp,
> MIR.cpp, jsmath.cpp). While I understand that the code within these files
> get executed when the compiler requests a bailout (after executing a non
> successful optimization path), I have some questions:
>
> Is my understanding (of when the code gets executed) correct?

Yes, RInstructions::recover functions are executed when we bailout from IonMonkey to Baseline.  They are using the SnapshotIterator (IonFrameIterator.h) which is basically the central point for giving an interpretation to IonMonkey's allocations (registers, stack offsets, constants, recover instructions).

When we generate code for IonMonkey, we record all the locations into a snapshot for each LInstruction which might bailout.  The SnapshotIterator decodes all these informations which are stored during the compilation. Bailouts are using a SnapshotIterator to read and execute the RInstruction::recover functions.

> I would also like to know how we extract the information from the JIT
> compiler frame (I read this article
> <https://wiki.mozilla.org/Platform/Features/IonMonkey>)

The SnapshotReader (Snapshots.cpp) decodes allocations of Value.  The allocation can either be a register (we get the spill location of it by reading the MachineState), a stack offset (we read from the bottom of the IonFrame), a constant (we read from the constant pool of the compiled script meta data), a recover instruction result (we read from the vector where the result is supposed to be stored).

> I have seen skeleton functions in Recover.cpp under m-c. Is that the only
> file I'll have to edit apart from writing tests? Or is
> dce-with-rinstructions.js the test file that I'm supposed to pass?

You will also have to edit MIR.h to encode the Recover_Instruction byte, and Recover.h to declare the structure of the RInstruction.
Hi Nicolas. I'm studying the Recover.cpp code and I guess I'm able to implement a patch, but I would like to share a question here.

I've made a some changes on RSub::recover(Recover.cpp#355) to force an error(changed 'SubValues' to 'MulValues'. So, I've tested the dce-with-rinstruction.js using ./dist/bin/js --ion-eager ../jit-test/tests/ion/dce-with-rinstructions.js and all test cases passed ok. When I've used the --ion-parallel-compile=off flag, the test test case of 'rsub_number' failed(as i expected). So, I've started to make some guesses, but I would like to understand it correctly. 

This test case gave me another doubt. When the Ion JIT fails, it bailout to the last success Instruction and execute it agin on Baseline level?

Another question is, What is the real behavior of iter.read()?
(In reply to Caio Lima(:caiolima) from comment #6)
> I've made a some changes on RSub::recover(Recover.cpp#355) to force an
> error(changed 'SubValues' to 'MulValues'. So, I've tested the
> dce-with-rinstruction.js using ./dist/bin/js --ion-eager
> ../jit-test/tests/ion/dce-with-rinstructions.js and all test cases passed
> ok. When I've used the --ion-parallel-compile=off flag, the test test case
> of 'rsub_number' failed(as i expected). So, I've started to make some
> guesses, but I would like to understand it correctly. 

When we compile with ion eager, we start the compilation out of the main thread with ion-eager, but the result of the compilation might not arrive in time for catching the failure.

When ran with --ion-parallel-compile=off, as soon as we want to compile we wait for the compilation result to be available before running the function.  This garantee that the compilation would be available as soon as we request it to be compiled.

> This test case gave me another doubt. When the Ion JIT fails, it bailout to
> the last success Instruction and execute it agin on Baseline level?

Yes, which is why we restrict these optimizations on side-effect free instructions.  Which is also the reason why we protect us against the object input case, as an object might be the subject of obervable changes.

> Another question is, What is the real behavior of iter.read()?

You can look at the code at[1], this function is used to iterate over the allocations contained in a snapshot.
The best representation of it is in the slides I made to introduce recover instructions[2].

[1] http://dxr.mozilla.org/mozilla-central/source/js/src/jit/JitFrameIterator.h?from=js::jit::SnapshotIterator::read%28%29&case=true#374
[2] https://nbp.github.io/slides/RInstruction/index.html?full#recover-instruction-rp
Thank you for the explanation!

So, actually, when the Bailout occurs, the Baseline executes since the last ResumePoint instead of return to the last imediate Instruction. Is it right?

By the way, the basic operations was implemented, but we need to implement the remain MDefinitions. May I open new bugs for them?
Depends on: 1020637
Depends on: 1020638
(In reply to Caio Lima(:caiolima) from comment #8)
> So, actually, when the Bailout occurs, the Baseline executes since the last
> ResumePoint instead of return to the last imediate Instruction. Is it right?

This is correct.  we resume the execution in baseline since the last captured resume point.

> By the way, the basic operations was implemented, but we need to implement
> the remain MDefinitions. May I open new bugs for them?

Only the non-effectful one, as we cannot recover after an effectful instruction.  I gathered a list in comment 0.  If there is no more bug available, or if you are interested by one in particular, feel free to open it as we did for other recover instructions.
Depends on: 1024587
Depends on: 1024589
Depends on: 1024609
Depends on: 1024895
Depends on: 1024896
Depends on: 1028556
Depends on: 1028573
Depends on: 1028662
Depends on: 1028704
Clean-up: https://hg.mozilla.org/integration/mozilla-inbound/rev/6ddc7aee37eb
Add a [leave-open], as this is a meta bug.
Whiteboard: [leave-open]
Depends on: 1030699
Depends on: 1028698
Depends on: 1028675
Depends on: 1034665
Depends on: 1036163
Depends on: 1038592
Depends on: 1038593
For more explanation about what are recover instructions and what they allow to do, you can read the blog post published by nbp on the JavaScript blog: https://blog.mozilla.org/javascript/2014/07/15/ionmonkey-optimizing-away/
Depends on: 1050649
Depends on: 1062888
Depends on: 1062893
Depends on: 1066040
Depends on: 1066041
Depends on: 1068605
Depends on: 1076922
Depends on: 1092547
Depends on: 1096129
Depends on: 1104647
Depends on: 1109052
Priority: -- → P5
Depends on: 1415772
Keywords: leave-open
Whiteboard: [leave-open]

The leave-open keyword is there and there is no activity for 6 months.
:sdetar, maybe it's time to close this bug?

Flags: needinfo?(sdetar)

(In reply to Release mgmt bot [:sylvestre / :calixte] from comment #13)

The leave-open keyword is there and there is no activity for 6 months.
:sdetar, maybe it's time to close this bug?

This is a meta bug, and 2 other bugs have to be fixed before closing this one.

Flags: needinfo?(sdetar)
You need to log in before you can comment on or make changes to this bug.