Closed Bug 989930 Opened 8 years ago Closed 8 years ago

SnapshotIterator should read Instructions instead of Frames.

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: nbp, Assigned: nbp)

References

Details

Attachments

(1 file)

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.
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)
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 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+
(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.
https://hg.mozilla.org/mozilla-central/rev/029a75143f6d
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.