Closed
Bug 989930
Opened 11 years ago
Closed 11 years ago
SnapshotIterator should read Instructions instead of Frames.
Categories
(Core :: JavaScript Engine: JIT, defect)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: nbp, Assigned: nbp)
References
Details
Attachments
(1 file)
13.53 KB,
patch
|
h4writer
:
review+
|
Details | Diff | Splinter Review |
As the RecoverBuffer is moving from reading only Frame to reading any kind of Instructions, we need to convert the SnapshotIterator to use Instructions as a default and reconstruct the Frame behavior based on the fact that we settle on ResumePoint recover instruction.
Assignee | ||
Comment 1•11 years ago
|
||
This patch does not yet implement the BailoutBaseline part as we do not yet have any Run functions to test with. In the mean time it changes the iteration layout to walk over instructions and not frames, and assert that we are only seeing RResumePoint.
The InlineFrameIterator can walk frames without executing any of the instructions, as JSFunctions are encoded as constants when we are inlining. Note that RInstruction result is not be available to InlineFrameIterator without a bailout. Also note, that by its nature, InlineFrameIterator is made to be non-mutating, which might be the case of the recover instructions.
Attachment #8399391 -
Flags: review?(hv1989)
Assignee | ||
Comment 2•11 years ago
|
||
Comment on attachment 8399391 [details] [diff] [review]
Convert from reading frames to reading instructions.
Review of attachment 8399391 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/jit/BaselineBailouts.cpp
@@ +1347,5 @@
> RootedScript topCaller(cx);
> jsbytecode *topCallerPC = nullptr;
>
> while (true) {
> + MOZ_ASSERT(snapIter.instructions()->isResumePoint());
fixed: typo (no 's') and missing include of "Recover.h"
Comment 3•11 years ago
|
||
Comment on attachment 8399391 [details] [diff] [review]
Convert from reading frames to reading instructions.
Review of attachment 8399391 [details] [diff] [review]:
-----------------------------------------------------------------
Awesome, this patch feels like we are getting closer :D
::: js/src/jit/BaselineBailouts.cpp
@@ +1347,5 @@
> RootedScript topCaller(cx);
> jsbytecode *topCallerPC = nullptr;
>
> while (true) {
> + MOZ_ASSERT(snapIter.instructions()->isResumePoint());
Looking forward to see this removed. Since this will get removed as soon as we handle more instructions \0/. Well this will probably stay, since we need a resumepoint at the top of inlining. But this and the following line, assume only MResumePoint instructions in between now ;).
::: js/src/jit/IonFrameIterator.h
@@ +288,5 @@
> MOZ_ASSERT(moreAllocations());
> return snapshot_.readAllocation();
> }
> Value skip() {
> + snapshot_.skipAllocation();
I think I don't really understand this change here. So I know it behaves exactly the same. I'm just wondering how this makes a difference? Is this change needed for later patches, or just because you were uncomfortable with "readAllocation" here in a function that is called skip()?
Attachment #8399391 -
Flags: review?(hv1989) → review+
Assignee | ||
Comment 4•11 years ago
|
||
(In reply to Hannes Verschore [:h4writer] from comment #3)
> ::: js/src/jit/IonFrameIterator.h
> @@ +288,5 @@
> > MOZ_ASSERT(moreAllocations());
> > return snapshot_.readAllocation();
> > }
> > Value skip() {
> > + snapshot_.skipAllocation();
>
> I think I don't really understand this change here. So I know it behaves
> exactly the same. I'm just wondering how this makes a difference? Is this
> change needed for later patches, or just because you were uncomfortable with
> "readAllocation" here in a function that is called skip()?
The later, as we now have a seek in the allocation table, it make more sense to have a skipAllocation, which is really skipping the read of the allocation.
Assignee | ||
Comment 5•11 years ago
|
||
Comment 6•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in
before you can comment on or make changes to this bug.
Description
•