881902 - rm ContextStack and StackSpace

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

Now that JM has been removed and bug 868437 has added the (Interpreter)Activation infrastructure, we can get rid of ContextStack, StackSpace, StackSegment and friends.

The interpreter stack no longer has to be contiguous and the plan is to use a per-runtime LifoAlloc for interpreter frames. This means we can avoid all the VirtualAlloc/mmap complexity in StackSpace::init.

Comment 1

[Luke Wagner [:luke]]

\o/

Comment 2

[Nicholas Nethercote [inactive]]

Please r? me for the memory reporter parts.  AIUI, for Linux we measure the resident part of the stack and report that in about:memory.  This is reasonable, but on debug builds I think we poison the entire stack and so the stack is measured as always being 4 MiB.  It would be nice if this didn't happen after this is fixed.  I guess we'll see how it works out.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Patch

Sorry for dropping another big patch in your queue, but large parts are code removal or mechanical changes and hopefully this is the last major stack refactoring patch. Nicholas, asking you to review the memory reporter changes per comment 2.

Main changes made by this patch:

* Removes ContextStack, StackSpace etc:

43 files changed, 646 insertions(+), 1622 deletions(-)

* The new InterpreterStack class is just a small wrapper around a LifoAlloc. There's one InterpreterStack per JSRuntime.

* StackFrame has an argv_ pointer, pointing to the caller's arguments. This is necessary because the stack is no longer contiguous.

* A runtime-wide counter is used to catch over-recursion. We assert in a number of places this counter is incremented/decremented correctly.

* InvokeArgsGuard is renamed to InvokeArgs and has an AutoValueVector to store its values.

* ContextStack::currentScript is now JSContext::currentScript.

* Once we allocate a heap StackFrame for generators, we never copy this frame anywhere. This is possible now because the stack no longer has to be contiguous and it simplified a few things.

Passes jit-tests, jstests and runs SS/Kraken/Octane without perf regressions.

Comment 4

[Luke Wagner [:luke]]

cc'ing Andy Wingo re: generator interpreter representation changes.

Comment 5

[Luke Wagner [:luke]]

Comment on attachment 765426 [details] [diff] [review]
Patch

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

Excellent!  Thanks for slogging through it all; we'll all benefit from this.

::: js/src/builtin/Object.cpp
@@ +530,5 @@
>       * Implement [[Prototype]]-getting -- particularly across compartment
>       * boundaries -- by calling a cached __proto__ getter function.
>       */
> +    InvokeArgs nested(cx);
> +    if (!nested.init(0))

For consistency, could you name all the InvokeArgs variables either args or (if there is already an 'args' in scope) args2?

::: js/src/vm/Interpreter.cpp
@@ +1055,5 @@
> +FrameGuard::~FrameGuard()
> +{
> +    if (state_.isGenerator()) {
> +        JSGenerator *gen = state_.asGenerator()->gen();
> +        gen->fp->unsetPushedSPSFrame();

This reminds me, you might want to test the firefox Profiler and SPS (using the cleopatra plugin) with this patch applied.

::: js/src/vm/Stack-inl.h
@@ +846,5 @@
> +    // Pop all inline frames.
> +    while (current_ != entry_)
> +        popInlineFrame(current_);
> +
> +    JS_ASSERT(oldFrameCount_ == cx_->runtime()->interpreterStack().frameCount_);

Could you also add a
  JS_ASSERT_IF(oldFrameCount_ == 0, cx_->runtime()->interpreterStack().allocator_.used() == 0);
?

::: js/src/vm/Stack.h
@@ +42,5 @@
> +// activation are contiguous: whenever C++ calls back into JS, a new activation is
> +// pushed.
> +//
> +// Every activation is tied to a single JSContext and JSCompartment. This means we
> +// can construct a given context's stack by skipping activations belonging to other

s/construct/reconstruct/?

@@ +261,5 @@
>          RUNNING_IN_JIT     =    0x10000,
>  
>          /* Miscellaneous state. */
> +        USE_NEW_TYPE       =    0x20000,  /* Use new type for constructed |this| object. */
> +        SUSPENDED          =    0x40000   /* Suspended generator frame. */

Can you put this flag next to YIELDING above, give them a /* Generator frame state */ comment on the preceding line and, in comments, differentiate between the two (they only diverge after creation (before first run) and after the generator returns but before it is GC, yes?).

@@ +280,5 @@
>      Value               rval_;          /* if HAS_RVAL, return value of the frame */
>      StaticBlockObject   *blockChain_;   /* if HAS_BLOCKCHAIN, innermost let block */
>      ArgumentsObject     *argsObj_;      /* if HAS_ARGS_OBJ, the call's arguments object */
>      jsbytecode          *prevpc_;       /* if HAS_PREVPC, pc of previous frame*/
> +    Value               *prevsp_;       /* if HAS_PREVPC, sp of previous frame*/

s/HAS_PREVPC/HAS_PREVSP/

@@ -364,4 @@
>       * the raw memory for the frame:
>       */
>  
> -    /* Used for Invoke, Interpret, trace-jit LeaveTree, and method-jit stubs. */

hehe, LeaveTree

@@ +850,5 @@
>          JS_ASSERT(isConstructing());
>          return flags_ & USE_NEW_TYPE;
>      }
>  
> +    void setSuspended() {

Could you put these next to the YIELDING helpers?

@@ +1038,5 @@
> +    // Maximum supported value of arguments.length. This bounds the maximum
> +    // number of arguments that can be supplied to Function.prototype.apply.
> +    // This value also bounds the number of elements parsed in an array
> +    // initialiser.
> +    static const unsigned ARGS_LENGTH_MAX = 500 * 1000;

I think this constant isn't special to the interpreter.  Could you hoist it into the global scope... somewhere (like jscntxt.h maybe?)

Comment 6

[Nicholas Nethercote [inactive]]

Comment on attachment 765426 [details] [diff] [review]
Patch

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

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +2010,5 @@
>                    nsIMemoryReporter::KIND_NONHEAP, rtStats.runtime.regexpData,
>                    "Memory used by the regexp JIT to hold data.");
>  
> +    RREPORT_BYTES(rtPath + NS_LITERAL_CSTRING("runtime/interpreter-stack"),
> +                  nsIMemoryReporter::KIND_NONHEAP, rtStats.runtime.interpreterStack,

This should be KIND_HEAP now, because it's allocated via malloc() instead of mmap().

Comment 7

[Jan de Mooij [:jandem]]

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

Try: https://tbpl.mozilla.org/?tree=Try&rev=98d07cafb945

(In reply to Luke Wagner [:luke] from comment #5)
> s/HAS_PREVPC/HAS_PREVSP/

There's no HAS_PREVSP, but I realized we don't need the HAS_PREVPC flag either so I removed it and added a comment. Every *inline* frame has prev_, prevpc_ and prevsp_ set. Every non-inline/entry frame has these values set to NULL.

(In reply to Nicholas Nethercote [:njn] from comment #6)
> This should be KIND_HEAP now, because it's allocated via malloc() instead of
> mmap().

Good catch! I thought NONHEAP meant non-GC-heap or something, should have looked closer.

Comment 8

[Jan de Mooij [:jandem]]

This seems to have broken the (hidden) GGC build on TBPL. There are a few new jit-test failures, all with --no-baseline --no-ion.

I will look into it today.

Comment 9

[Jan de Mooij [:jandem]]

Attachment: Follow-up fix for GGC orange

Small patch that should fix the GGC orange, reviewed by terrence on IRC. The tree is closed now and I probably won't be able to push this later today. Attaching it here so that somebody else can.

Comment 10

[Jan de Mooij [:jandem]]

Setting checkin-needed for the second patch.

Comment 12

[Jan de Mooij [:jandem]]

Follow-up patch:

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

Comment 13

[Phil Ringnalda (:philor)]

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

Comment 14

[Phil Ringnalda (:philor)]

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