989934 - Remove dependency on RecoverBuffer::frameCount

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Nicolas B. Pierron [:nbp]]

Currently the frameCount count the number of frames and it will soon be replaced in Bug 989930 by a counter of instructions.

The frameCount is only used in the inline frame iterator.  We can compute the frameCount while looking for the innermost frame.

Comment 1

[Nicolas B. Pierron [:nbp]]

Attachment: Do not rely on SnapshotIterator::frameCount to iterate over inline frames.

Comment 2

[Kannan Vijayan [:djvj]]

Comment on attachment 8399383 [details] [diff] [review]
Do not rely on SnapshotIterator::frameCount to iterate over inline frames.

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

::: js/src/jit/IonFrameIterator-inl.h
@@ +22,5 @@
>  InlineFrameIteratorMaybeGC<allowGC>::InlineFrameIteratorMaybeGC(
>                                                  JSContext *cx, const IonBailoutIterator *iter)
>    : frame_(iter),
>      framesRead_(0),
> +    frameCount_(uint32_t(-1)),

use UINT32_MAX or the std::numeric_limits specialization.

::: js/src/jit/IonFrames.cpp
@@ +1554,5 @@
>  InlineFrameIteratorMaybeGC<allowGC>::resetOn(const IonFrameIterator *iter)
>  {
>      frame_ = iter;
>      framesRead_ = 0;
> +    frameCount_ = uint32_t(-1);

Use UINT32_MAX or the C++ std::numeric_limits specialization.

@@ +1559,5 @@
>  
>      if (iter) {
>          start_ = SnapshotIterator(*iter);
>          findNextFrame();
>      }

This seems problematic here.  In the original code, after this execution on a non-null iter, |InlineFrameIteratorMaybeGC::frameCount()| would return a valid value, since it uses |start_.frameCount()|, and that gets initialized above.

In the new code, |InlineFrameIteratorMaybeGC::frameCount()| would return UINT32_MAX, since the new local field |frameCount_| is no longer initialized.

Especially given the call to |findNextFrame| here, which uses the local field |frameCount_| which will be UINT32_MAX now.

@@ +1582,5 @@
>  #endif
>  
>      // This unfortunately is O(n*m), because we must skip over outer frames
>      // before reading inner ones.
> +    size_t remaining = frameCount_ - framesRead_ - 1;

See above comment.  How is |frameCount_| expected to be initialized here when arrived at through the path described above?

@@ +1584,5 @@
>      // This unfortunately is O(n*m), because we must skip over outer frames
>      // before reading inner ones.
> +    size_t remaining = frameCount_ - framesRead_ - 1;
> +    size_t i = 1;
> +    for (; i <= remaining && si_.moreFrames(); i++) {

Ah, I think I see what's happening.  You're allowing |remaining| to be uninitialized here, and adding the |moreFrames()| check to allow it to be initialized the first time around.

In that case, I'd suggest adding comments clearly explaining this approach.  Also, the above initialization of |remaining| should look more like:

  size_t remaining = (frameCount_ == UINT32_MAX) ? SIZE_MAX : frameCount_ - framesRead_ - 1;

@@ +1627,5 @@
>  
> +    // The first time we do not know the number of frames, we only settle on the
> +    // last frame, and update the number of frames based on the number of
> +    // iteration that we have done.
> +    if (frameCount_ == uint32_t(-1)) {

use UINT32_MAX, or the C++ std::numeric_limits specialization.

Comment 3

[Kannan Vijayan [:djvj]]

Comment on attachment 8399383 [details] [diff] [review]
Do not rely on SnapshotIterator::frameCount to iterate over inline frames.

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

See review comments above.  Should be good to go once they're addressed.

Comment 4

[Nicolas B. Pierron [:nbp]]

(In reply to Kannan Vijayan [:djvj] from comment #2)
> @@ +1559,5 @@
> >  
> >      if (iter) {
> >          start_ = SnapshotIterator(*iter);
> >          findNextFrame();
> >      }
> 
> This seems problematic here.  In the original code, after this execution on
> a non-null iter, |InlineFrameIteratorMaybeGC::frameCount()| would return a
> valid value, since it uses |start_.frameCount()|, and that gets initialized
> above.
> 
> In the new code, |InlineFrameIteratorMaybeGC::frameCount()| would return
> UINT32_MAX, since the new local field |frameCount_| is no longer initialized.

There is no |InlineFrameIteratorMaybeGC::frameCount()|, only a |InlineFrameIteratorMaybeGC::frameNo()|, but in any case, this is not supposed to be used with a non-initialized Iterator, which first action is to settle on the inner most frame, which also means updating the frameCount_.

There is a |SnapshotIterator::frameCount()| (start_ and si_ are SnapshotIterators), but I am doing this patch to remove usage of it, as I am replacing it by |SnapshotIterator::numInstructions()| in the blocked bug. (Bug 989930)

Comment 5

[Nicolas B. Pierron [:nbp]]

Attachment: Do not rely on SnapshotIterator::frameCount to iterate over inline frames.

Comment 6

[Kannan Vijayan [:djvj]]

Comment on attachment 8401308 [details] [diff] [review]
Do not rely on SnapshotIterator::frameCount to iterate over inline frames.

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

::: js/src/jit/IonFrameIterator.h
@@ +451,3 @@
>          callee_(cx),
>          script_(cx)
>      {

Would be good to add a comment here describing how |frameCount_| behaves (UINT3_MAX at first, initialized on first call to |findNextFrame()|).

Comment 7

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b756fe5f27d8

Comment 8

[Ryan VanderMeulen [:RyanVM]]

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