1244215 - Get rid of EnsureExitFrame and JitFrame_Unwound_* frames

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Jan de Mooij [:jandem]]

We have a bunch of intermittent oranges in JitProfilingFrameIterator::operator++. Based on one stack I looked at, the problem is that the profiler can interrupt the main thread while it's modifying a frame's descriptor in EnsureExitFrame.

EnsureExitFrame (+ the JitFrame_Unwound_* types it requires) are more complicated than necessary. To get rid of it, cdleary wanted to change the frame descriptor to include the frame prefix size, so that fp + descriptor.size always takes us to the next (older) frame, independent of the frame's type, see bug 717297.

I tried this approach today and got it working for Baseline on x86, with almost no overhead when constructing frames. After that, Nicolas and I came up with an even better idea: we can keep the descriptor's size as it is, but store the callee's FrameType in the descriptor, next to the caller's FrameType we currently store in it.

That should let us remove EnsureExitFrame and the JitFrame_Unwound_* types, and we can hopefully simplify JitProfilingFrameIterator quite a bit.

After that, we might be able to remove the other FrameType we store in the descriptor word. So basically we will push the frame's type when we enter the frame, instead of when we leave it (as we do now).

Comment 1

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #0)
> we can keep the descriptor's size as it is, but
> store the callee's FrameType in the descriptor, next to the caller's
> FrameType we currently store in it.

> After that, we might be able to remove the other FrameType we store in the
> descriptor word. So basically we will push the frame's type when we enter
> the frame, instead of when we leave it (as we do now).

Actually that's not possible, because we don't know whether the callee is Baseline or Ion code when we make a JS -> JS call.

What we can do is store the prefix size instead of the FrameType. That won't let us remove the caller's FrameType, but it's still nicer than what we do now.

Comment 2

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #1)
> Actually that's not possible, because we don't know whether the callee is
> Baseline or Ion code when we make a JS -> JS call.

What we *could* do is merge JitFrame_BaselineJS and JitFrame_IonJS into a single type, and then JitFrameIterator can distinguish Baseline and Ion by checking returnAddressToFp_: if returnAddressToFp_ is inside calleeToken->script->baselineScript->code, then it's a Baseline frame. Else, it's an Ion frame.

That's not necessary for this bug though.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Add prefix size to descriptor

This adds the prefix size to the frame descriptor. Pretty straight-forward for the most part.

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Cleanup and refactoring

This patch removes unwound frame types, gets rid of some JitProfilingFrameIterator duplication, and removes code that's now dead.

 9 files changed, 55 insertions(+), 307 deletions(-)

Comment 5

[Jan de Mooij [:jandem]]

I verified this also fixes bug 1238793.

Comment 6

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8714430 [details] [diff] [review]
Part 1 - Add prefix size to descriptor

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

Awesome!

::: js/src/jit/JitFrames.h
@@ +292,5 @@
>  
>  void UpdateJitActivationsForMinorGC(JSRuntime* rt, JSTracer* trc);
>  
>  static inline uint32_t
> +EncodePrefixSize(size_t prefixSize)

Q: "prefix" sounds too generic to be descriptive, what do you think of "frameHeader"?

@@ +297,5 @@
>  {
> +    MOZ_ASSERT((prefixSize % sizeof(uintptr_t)) == 0);
> +
> +    uint32_t prefixSizeWords = prefixSize / sizeof(uintptr_t);
> +    MOZ_ASSERT(prefixSizeWords < PREFIX_SIZE_MASK);

nit: … <= …_MASK

@@ +302,5 @@
> +    return prefixSizeWords;
> +}
> +
> +static inline uint32_t
> +MakeFrameDescriptor(uint32_t frameSize, FrameType type, uint32_t prefixSize)

maybe-follow-up?

While reading the calls, I wondered if we should not swap the first and second arguments, the reason being that if we do so, then the arguments would be ordered in the order of the objects they are referring to on the stack.

  Stack:
    … <-- caller-frame header --> <-- caller-frame --> <-- callee-frame header --> …

  Arguments:
    <-- caller-frame type --> <-- caller-frame size --> <-- callee-frame header size -->

Thus, instead of:

  MakeFrameDescriptor(uint32_t(rectifierFrameSize), JitFrame_Rectifier, JitFrameLayout::Size());

we would have something like:

  MakeFrameDescriptor(JitFrame_Rectifier, uint32_t(rectifierFrameSize), JitFrameLayout::Size());

Comment 7

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8714435 [details] [diff] [review]
Part 2 - Cleanup and refactoring

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

Nice work!
Thanks for removing dead code. :)

The frame conversion into a Bare exit frame is a bit scary, but sounds ok with a robust assertion as suggested below.

::: js/src/jit/JitFrames.cpp
@@ +923,5 @@
> +
> +    if (cx->runtime()->jitTop == (uint8_t*)frame) {
> +        // If we already called this function for the current frame, do
> +        // nothing.
> +        MOZ_ASSERT(exitFrame->isBareExit());

This assertion would be triggered when called via:
  BaselineCompiler::emit_JSOP_FINALYIELDRVAL
  js::jit::FinalSuspend
  js::jit::DebugEpilogue

as there is already an exit frame pushed for calling into FinalSuspend, thus the token is not a bare token.  The same can happen with a return where the "ok" flag is flipped by the debugger hook.

I think we can safely remove this assertion, if the assertion suggested below is moved above this condition.  The difference would be that we would mark the original content of the exit frame instead of ignoring it, which does not sounds alarming.

@@ +931,5 @@
> +    MOZ_ASSERT((uint8_t*)exitFrame->footer() >= cx->runtime()->jitTop,
> +               "Must have space for ExitFooterFrame before jitTop");
> +
> +    cx->runtime()->jitTop = (uint8_t*)frame;
> +    *exitFrame->footer()->addressOfJitCode() = ExitFrameLayout::BareToken();

I am worried about this write, as it effectively erase stack space which does not belong to the original frame.

The above assertion which use the jitTop should be fine, as long as the jitTop is sane, which should be the case iff the caller is called under callVM.  The problem with jitTop is that it is not reset when we exit either fakeExitFrame, or when we exit VMCalls.  Thus the current assertion effectively provides no guarantee that this is safe.

I suggest we change the current assertion by creating a JitFrameIterator from the jitTop and assert that the current frame is reachable through the JitFrameIterator.

@@ -1339,5 @@
>  MarkJitExitFrame(JSTracer* trc, const JitFrameIterator& frame)
>  {
> -    // Ignore fake exit frames created by EnsureExitFrame.
> -    if (frame.isFakeExitFrame())
> -        return;

self-note:

This is the major difference with the removal of Unwounded frames variant, which is that we now convert the frames to bare exit frames.  The difference being that bare exit frame do need a footer to identify them as such.

Comment 8

[Jan de Mooij [:jandem]]

(In reply to Nicolas B. Pierron [:nbp] from comment #7)
> This assertion would be triggered when called via:
>   BaselineCompiler::emit_JSOP_FINALYIELDRVAL
>   js::jit::FinalSuspend
>   js::jit::DebugEpilogue
> 
> as there is already an exit frame pushed for calling into FinalSuspend, thus
> the token is not a bare token.  The same can happen with a return where the
> "ok" flag is flipped by the debugger hook.

No, because |frame| is a JitFrameLayout* and we're inside:

    if (cx->runtime()->jitTop == (uint8_t*)frame)

So this can only happen if we already turned the JitFrameLayout into an ExitFrameLayout, and when we do that we always make it a bare exit frame.

> I suggest we change the current assertion by creating a JitFrameIterator
> from the jitTop and assert that the current frame is reachable through the
> JitFrameIterator.

Okay, I'll add this assert. I'd like to keep the original assert as well; the more checks we have here the better.

Comment 9

[Jan de Mooij [:jandem]]

(In reply to Nicolas B. Pierron [:nbp] from comment #6)
> Q: "prefix" sounds too generic to be descriptive, what do you think of
> "frameHeader"?

OK, frameHeaderSize (+ headerSize) sound good to me; I'll use that everywhere instead of prefix size.

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fdaa29338b29
https://hg.mozilla.org/integration/mozilla-inbound/rev/f852c7bbd5fc

Comment 11

[Jan de Mooij [:jandem]]

Attachment: Part 3 - Remove JitFrame_LazyLink

isFakeExitFrame is gone, so we no longer need JitFrame_LazyLink.

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fdaa29338b29
https://hg.mozilla.org/mozilla-central/rev/f852c7bbd5fc

Comment 13

[Pulsebot]

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

Comment 14

[Carsten Book [:Tomcat]]

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