875125 - Parse javascript off main thread to benefit from delay between parsing & execution

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[(dormant account)]

This will help with chrome code startup(make it easier for us to get rid of startup cache) and <script async>

Comment 1

[Boris Zbarsky [:bzbarsky]]

We'd still throw the syntax error at execute time in case of syntax error, right?

Comment 2

[(dormant account)]

(In reply to Boris Zbarsky (:bz) from comment #1)
> We'd still throw the syntax error at execute time in case of syntax error,
> right?

This should preserve existing behavior.

Comment 3

[Brian Hackett [Laid off!]]

Attachment: patch (949582d150c2)

This patch allows JS to be parsed/emitted off thread.  A string and compile options are supplied, then a new compartment/zone are created and handed off to a worker thread to proceed.  GC activity pauses the parse and doesn't collect from the parse zone, but isn't forced to block on or cancel the parse.

Right now this is only exposed via a threadsafe shell function, offThreadCompileScript().  Once the parse finishes the resulting script is put in a list and never actually consumed by the main thread --- I'd like to do that as followup, in concert with browser changes to use off thread parsing.

Locks are needed when in the atoms compartment or accessing a couple other per-runtime things.  These locks are only actually required when another thread is active, otherwise they are just a couple branches.  With no other thread this has no effect on parse times on the main thread; if I pretend there is another thread and always use the lock, the score on octane-codeload is hurt by about 10% with all the atomization going on.  This isn't terrible, and in any case the initial uses for off thread parsing are during browser startup when we're not running benchmarks.

Comment 4

[Brian Hackett [Laid off!]]

Attachment: boring parts

Splitting the above patch up for review, a bit.  This patch has some boring changes for adding additional places where an ExclusiveContext is needed rather than a JSContext.

Comment 5

[Brian Hackett [Laid off!]]

Attachment: fun parts

The rest of the changes.  I don't see a great way to split this up further.  This does the following things:

- Extends JS worker threads (the ones used for Ion/Asm.js compilation) to also do off thread parsing/emitting.

- Adds functionality to the worker threads so that they can be paused for GC activity.

- Adds a lock to the runtime to protect runtime-wide data that can be read/written during parsing.

- Keeps a list of PerThreadData instances on the runtime for use by the GC.  This does not currently include PJS PerThreadData instances (which are created/destroyed off thread and would race) but these don't currently interact with the GC anyways.

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 776472 [details] [diff] [review]
boring parts

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

::: js/src/frontend/BytecodeCompiler.cpp
@@ +156,5 @@
>      RootedString source(cx, source_);
>      SkipRoot skip(cx, &chars);
>  
> +    if (cx->isJSContext())
> +        MaybeCallSourceHandler(cx->asJSContext(), options, chars, length);

Can you file a bug for calling this for off-thread compilations, and put a comment here with the bug number and a short explanation?

@@ +183,5 @@
> +    // Saving source is not yet supported when parsing off thread.
> +    JS_ASSERT_IF(!cx->isJSContext(), !extraSct && options.sourcePolicy == CompileOptions::NO_SOURCE);
> +
> +    SourceCompressionToken *sct = extraSct;
> +    Maybe<SourceCompressionToken> mysct;

It seems cleaner to me to pass maybeJSContext() into the mysct constructor and drop the Maybe<>. It's up to you though.

@@ +189,5 @@
> +        mysct.construct(cx->asJSContext());
> +        sct = mysct.addr();
> +    }
> +
> +    if (sct) {

Why do you need this branch?

@@ +312,5 @@
>  
>                  // Destroying the parse context will destroy its free
>                  // variables, so check if any deoptimization is needed.
> +                if (cx->isJSContext()) {
> +                    if (!MaybeCheckEvalFreeVariables(cx->asJSContext(), evalCaller, scopeChain, parser, pc.ref()))

Can you add an assert that we don't use eval during off-thread compilation, and then comment here that we assert against it?

@@ +337,5 @@
>          }
>  
>          if (!FoldConstants(cx, &pn, &parser))
>              return NULL;
> +        if (cx->isJSContext() && !NameFunctions(cx->asJSContext(), pn))

Same thing about filing a bug for this one.

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 776477 [details] [diff] [review]
fun parts

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

This patch is really nice. There are a few issues with ownership though, and some nits.

::: js/src/gc/Zone.h
@@ +238,5 @@
>  
>      bool                         isSystem;
>  
> +    /* Whether this zone is being used by a thread with an ExclusiveContext. */
> +    bool isExclusive;

Maybe this could be isMainThread or something? It might be a little clearer what it really means.

::: js/src/jsapi.cpp
@@ +686,5 @@
>  {
>      if (dtoaState)
>          js_DestroyDtoaState(dtoaState);
> +
> +    if (isInList())

This doesn't seem to be defined.

@@ +705,5 @@
> +PerThreadData::addToThreadList()
> +{
> +    // PerThreadData which are created/destroyed off the main thread do not
> +    // show up in the runtime's thread list.
> +    runtime_->assertValidThread();

I'm wondering if we should now change this assertion to assertMainThread or something. Valid is pretty ambiguous nowadays.

::: js/src/jsatom.cpp
@@ +294,5 @@
>       * unchanged, we need to re-lookup the table position because a last-ditch
>       * GC will potentially free some table entries.
>       */
>  
> +    AutoLockForExclusiveAccess lock(cx);

It looks like maybe you missed a use of cx->atoms() in js::AtomizeString.

::: js/src/jscntxtinlines.h
@@ +521,5 @@
> +    JS_ASSERT_IF(!isJSContext() && comp != runtime_->atomsCompartment,
> +                 comp->zone()->isExclusive);
> +
> +    // Normal JSContexts cannot enter exclusive zones.
> +    JS_ASSERT_IF(isJSContext() && comp && comp->zone(),

I don't think a compartment's zone will ever be null.

@@ +529,5 @@
> +    JS_ASSERT_IF(comp == runtime_->atomsCompartment,
> +                 runtime_->currentThreadHasExclusiveAccess());
> +
> +    // Make sure that the atoms compartment has its own zone.
> +    JS_ASSERT_IF(comp && comp->zone() && comp != runtime_->atomsCompartment,

Same here about the zone.

::: js/src/jspubtd.h
@@ +304,5 @@
>    public:
>      explicit ContextFriendFields(JSRuntime *rt)
>        : runtime_(rt), compartment_(NULL), zone_(NULL), autoGCRooters(NULL)
>      {
> +#ifdef JSGC_TRACK_EXACT_ROOTS

I don't think this belongs in this patch.

::: js/src/jsscript.cpp
@@ +1575,1 @@
>              // FIXME bug 875125 this should check all instances of PerThreadData.

Take out the comment.

::: js/src/jsworkers.cpp
@@ +191,5 @@
> +js::StartOffThreadParseScript(JSContext *cx, const CompileOptions &options,
> +                              const jschar *chars, size_t length)
> +{
> +    JSRuntime *rt = cx->runtime();
> +    if (!EnsureParallelCompilationInitialized(rt))

Maybe this name should be changed.

@@ +547,5 @@
> +
> +    parseTask = state.parseWorklist.popCopy();
> +    parseTask->cx->setWorkerThread(this);
> +
> +    state.unlock();

Can you use AutoUnlockWorkerThreadState here, scoped to the block? It's a little less error-prone. It looks like the same could be done for the other handleX methods.

@@ +568,4 @@
>  WorkerThread::threadLoop()
>  {
>      WorkerThreadState &state = *runtime->workerThreadState;
>      state.lock();

I'd feel more comfortable if this were switched to AutoLockWorkerThreadState and the unlock call removed.

@@ +624,5 @@
> +    needsUnpause = true;
> +
> +    for (size_t i = 0; i < state.numThreads; i++) {
> +        JS_ASSERT(!state.threads[i].shouldPause);
> +        state.threads[i].shouldPause = 1;

Is there any reason for having one shouldPause var per thread? It seems like we could get away with a single one for everybody. I guess it might make maybePause a little slower?

@@ +630,5 @@
> +
> +    while (true) {
> +        bool allPaused = true;
> +        for (size_t i = 0; i < state.numThreads; i++)
> +            allPaused &= state.threads[i].paused;

For the paused flag, it seems better to me to have a single counter of how many threads are paused. We always manipulate it under a lock, so there's no worry about racing. And using a counter means that the worker threads can easily be changed to notify the main thread only when they're all finished pausing. i.e.,

if (state.pausedCount == state.numThreads)
    state.notify(WorkerThreadState::MAIN);

I think this might save us some needless context switches.

::: js/src/jsworkers.h
@@ +159,5 @@
> +    /*
> +     * Whether this thread should pause its activity. This acts like the
> +     * runtime's interrupt field and may be read without locking.
> +     */
> +    size_t shouldPause;

Please make this volatile.

@@ +315,5 @@
> +struct ParseTask
> +{
> +    JSRuntime *runtime;
> +    ExclusiveContext *cx;
> +    CompileOptions options;

You probably know that CompileOptions is all messed up right now, and it definitely doesn't work in the heap. Can you talk to Bobby and Luke and Jim about it? I don't know if anyone has a plan.

::: js/src/shell/js.cpp
@@ +3242,5 @@
> +    options.setFileAndLine("<string>", 1)
> +           .setCompileAndGo(true)
> +           .setSourcePolicy(CompileOptions::NO_SOURCE);
> +
> +    const jschar *chars = JS_GetStringCharsZ(cx, scriptContents);

I don't see anything holding these chars alive. We pull them out of the string and pass them to the ParseTask without copying them, as far as I can tell. I'd suggest copying them in the ParseTask, but maybe there's a better way.

::: js/src/vm/Runtime.h
@@ +733,5 @@
> +     * main thread with an ExclusiveContext which could access such data.
> +     */
> +    PRLock *exclusiveAccessLock;
> +#ifdef DEBUG
> +    PRThread *exclusiveAccessOwner;

Can you use DebugOnly for this?

@@ +746,5 @@
> +#endif // JS_THREADSAFE
> +
> +    bool currentThreadHasExclusiveAccess() {
> +#if defined(JS_THREADSAFE) && defined(DEBUG)
> +        return !numExclusiveThreads || exclusiveAccessOwner == PR_GetCurrentThread();

I think it would be nice if, even when numExclusiveThreads == 0, we could check that there's an AutoLockForExclusiveAccess in scope. Could we have a DEBUG-only flag like mainThreadHasExclusiveAccess or something, and check it here?

@@ +1762,5 @@
> +    PerThreadData *iter;
> +
> +public:
> +    explicit ThreadDataIter(JSRuntime *rt) {
> +        iter = rt->threadList.getFirst();

It might be nice to assert that threads are stopped when we use this thing.

Comment 8

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/868ce514bba7

Comment 9

[Brian Hackett [Laid off!]]

Followup fix.  The first push had a typo that disabled off thread compilation.

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

Comment 10

[(no longer active)]

Regression: Mozilla-Inbound-Non-PGO - Kraken Benchmark - WINNT 6.2 x64 - 6.56% increase
---------------------------------------------------------------------------------------
    Previous: avg 1708.400 stddev 34.819 of 12 runs up to revision 03d0dc9e1cb8
    New     : avg 1820.433 stddev 4.589 of 12 runs since revision 868ce514bba7
    Change  : +112.033 (6.56% / z=3.218)
    Graph   : http://mzl.la/18sYjlP

Changeset range: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=03d0dc9e1cb8&tochange=868ce514bba7

Changesets:
  * http://hg.mozilla.org/integration/mozilla-inbound/rev/868ce514bba7
    : Brian Hackett <[e-mail]> - Bug 875125 - Allow scripts to be parsed/emitted off the main thread, r=billm.
    : http://bugzilla.mozilla.org/show_bug.cgi?id=875125

Bugs:
  * http://bugzilla.mozilla.org/show_bug.cgi?id=875125 - Parse javascript off main thread to benefit from delay between parsing & execution

Comment 11

[Brian Hackett [Laid off!]]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #10)
> 
> Regression: Mozilla-Inbound-Non-PGO - Kraken Benchmark - WINNT 6.2 x64 -
> 6.56% increase

This should be fixed by the patch in comment 9.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/868ce514bba7
https://hg.mozilla.org/mozilla-central/rev/d4d9f284b3d0
https://hg.mozilla.org/mozilla-central/rev/a012e1232912

Comment 13

[Boris Zbarsky [:bzbarsky]]

The patch in comment 9 seems to have caused a significant regression on Dromaeo-DOM, but only on Mac 10.6.  See graph at http://mzl.la/1dKRqJz and compare-talos data at http://perf.snarkfest.net/compare-talos/index.html?oldRevs=67661dd96576&newRev=d4d9f284b3d0&submit=true and http://perf.snarkfest.net/compare-talos/breakdown.html?oldTestIds=27659395,27677965,27677985,27677999,27678011,27678031&newTestIds=27660573,27677895,27677915,27677923,27677941,27677957&testName=dromaeo_dom