1364974 - Allow off-thread decoding multiple scripts for a single global in one operation

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P1)

Description

[Kris Maglione [:kmag]]

Bug 1359653 added the ability to off-thread decode scripts at startup prior to use. One of the biggest performance hurdles for that work is the fact that setup for off-thread decoding is quite expensive, and outweighs the benefit of off-thread decoding except for very large scripts.

If we can decode multiple scripts per operation, we should only need to pay the setup and merge costs once, which means we should be able to precompile many more scripts, with much less overhead.

Comment 1

[Kris Maglione [:kmag]]

Attachment: Bug 1364974: Part 1 - Switch to Variant for holding decode task data.

We'll need to be able to support more than two parse data types in order to
support multi-script decode operations. Since MaybeOneOf only supports two
types, that means switching to Variant.

Review commit: https://reviewboard.mozilla.org/r/139930/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/139930/

Comment 2

[Kris Maglione [:kmag]]

Attachment: Bug 1364974: Part 2 - Support returning multiple scripts in a parse task.

In order to support parsing multiple scripts in a single task, we need to be
able to store and trace them and their source objects while the task is
pending. Since there isn't much point in keeping both single-script and Vector
members, this changes all operations to use Vectors with one static slot.

Review commit: https://reviewboard.mozilla.org/r/139932/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/139932/

Comment 3

[Kris Maglione [:kmag]]

Attachment: Bug 1364974: Part 3 - Support decoding multiple scripts in a single parse tasks.

The per-operation overhead for off-thread parse tasks makes them unsuitable
for use with small scripts, since the setup overhead outweighs the benefits of
off-thread decoding. Decoding multiple scripts in a single operation for the
same global means we don't need to pay that cost per-script, and can save a
significant amount of startup overhead by decoding more scripts off-thread.

Review commit: https://reviewboard.mozilla.org/r/139934/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/139934/

Comment 4

[Kris Maglione [:kmag]]

Attachment: Bug 1364974: Part 4 - Replace CachedScript::mStatus and related logic with original and final process sets.

The current logic is fairly hard to follow, and fails to account for changes
in the set of processes a script needs to be executed in when deciding whether
to write a new cache file. These changes simplify that logic considerably,
increase the chances that we'll correctly pre-decode a script that's needed in
a given process during startup.

Review commit: https://reviewboard.mozilla.org/r/139936/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/139936/

Comment 5

[Kris Maglione [:kmag]]

Attachment: Bug 1364974: Part 5 - Perform off-thread decode operations in chunks, rather than singly.

Review commit: https://reviewboard.mozilla.org/r/139938/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/139938/

Comment 6

[Kris Maglione [:kmag]]

So, some initial talos results for how this patch interacts with WebExtension startup time. Landing this patch together with a WebExtension system add-on with only an empty background script:

https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-inbound&originalRevision=8f89d291e303&newProject=try&newRevision=035124e7ab19ff3c84736b1eda421ca1129f875d&framework=1&filter=opt&showOnlyImportant=0

So far, we see a <1.5% ts_paint and sessionrestore regression over baseline. There's still about a 15% tabpaint regression in this run, because the extension content process logic is being loaded into the parent.

With the same add-on loaded at startup, but OOP extensions enabled:

https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-inbound&originalRevision=8f89d291e303&newProject=try&newRevision=6fe2377d56b89521b582370fd38a8f06156462b9&framework=1&filter=opt&showOnlyImportant=0

Combined with this patch, so far we see a significant *improvement* on ts_paint and sessionrestore, and no significant effect on tabpaint.

Comment 7

[:shell escalante]

Hi Shu:,  When will you have time to review this patch?  It's getting close to the line for landing and then fully testing this batch of patches to make Beta.

Comment 8

[Shu-yu Guo [:shu]]

I'll try to get it today.

Comment 9

[Shu-yu Guo [:shu]]

Comment on attachment 8868360 [details]
Bug 1364974: Part 1 - Switch to Variant for holding decode task data.

https://reviewboard.mozilla.org/r/139930/#review144780

Comment 10

[Shu-yu Guo [:shu]]

Comment on attachment 8868361 [details]
Bug 1364974: Part 2 - Support returning multiple scripts in a parse task.

https://reviewboard.mozilla.org/r/139932/#review144782

::: js/src/vm/HelperThreads.h:611
(Diff revision 1)
>      void* callbackData;
>  
> -    // Holds the final script between the invocation of the callback and the
> +    // Holds the final scripts between the invocation of the callback and the
>      // point where FinishOffThreadScript is called, which will destroy the
>      // ParseTask.
> -    JSScript* script;
> +    mozilla::Vector<JSScript*, 1> scripts;

You should be able to use GCVector and save yourself manual iteration for tracing.

::: js/src/vm/HelperThreads.h:614
(Diff revision 1)
>      // point where FinishOffThreadScript is called, which will destroy the
>      // ParseTask.
> -    JSScript* script;
> +    mozilla::Vector<JSScript*, 1> scripts;
>  
> -    // Holds the ScriptSourceObject generated for the script compilation.
> -    ScriptSourceObject* sourceObject;
> +    // Holds the ScriptSourceObjects generated for the script compilation.
> +    mozilla::Vector<ScriptSourceObject*, 1> sourceObjects;

Ditto GCVector.

::: js/src/vm/HelperThreads.cpp:310
(Diff revision 1)
>      callback(callback), callbackData(callbackData),
> -    script(nullptr), sourceObject(nullptr),
>      overRecursed(false), outOfMemory(false)
>  {
> +    Unused << scripts.reserve(scripts.capacity());
> +    Unused << sourceObjects.reserve(sourceObjects.capacity());

These must be placeholders since these reserve calls are nops.

::: js/src/vm/HelperThreads.cpp:1339
(Diff revision 1)
> +
> +    JS::RootedScript script(cx);
> +    if (parseTask->scripts.length() > 0)
> +        script = parseTask->scripts[0];
> +
> +    for (auto& script : parseTask->scripts)

Rename this iteration variable to avoid confusion with the RootedScript above it please.

::: js/src/vm/HelperThreads.cpp:1339
(Diff revision 1)
> +
> +    JS::RootedScript script(cx);
> +    if (parseTask->scripts.length() > 0)
> +        script = parseTask->scripts[0];
> +
> +    for (auto& script : parseTask->scripts)

Rename this iteration variable to avoid refactoring hazard with the RootedScript above it please.

Comment 11

[Kris Maglione [:kmag]]

Comment on attachment 8868361 [details]
Bug 1364974: Part 2 - Support returning multiple scripts in a parse task.

https://reviewboard.mozilla.org/r/139932/#review144782

> These must be placeholders since these reserve calls are nops.

I thought they were no-ops, too, but apparently we track reserved size and capacity separately. So reserve() always succeeds if it's <= capacity, but if we don't call it, infallibleAppend causes assertion failures :(

Comment 12

[Shu-yu Guo [:shu]]

Comment on attachment 8868362 [details]
Bug 1364974: Part 3 - Support decoding multiple scripts in a single parse tasks.

https://reviewboard.mozilla.org/r/139934/#review144786

Glad to see this worked out straightforwardly.

::: js/src/jsapi.h:6193
(Diff revision 1)
>  typedef mozilla::Vector<uint8_t> TranscodeBuffer;
>  typedef mozilla::Range<uint8_t> TranscodeRange;
>  
> +struct TranscodeSource
> +{
> +    TranscodeSource(const TranscodeRange& range_, const char* file, unsigned line)

uint32_t for line

::: js/src/jsapi.h:6199
(Diff revision 1)
> +        : range(range_), filename(file), lineno(line)
> +    {}
> +
> +    const TranscodeRange range;
> +    const char* filename;
> +    const int lineno;

uint32_t

::: js/src/vm/HelperThreads.h:57
(Diff revision 1)
>  enum class ParseTaskKind
>  {
>      Script,
>      Module,
> -    ScriptDecode
> +    ScriptDecode,
> +    ScriptsDecode

Edit distance too small! Maybe MultiScriptsDecode.

::: js/src/vm/HelperThreads.h:269
(Diff revision 1)
>      }
>  
> +    template <
> +        typename F,
> +        typename = typename mozilla::EnableIf<
> +            mozilla::IsSame<bool, decltype((*(F*)nullptr)((ParseTask*)nullptr))>::value

A comment would be appreciated that, as you explained to me on IRC, this is hacking the C++ type system to check that F is ParseTask* -> bool.

::: js/src/vm/HelperThreads.h:272
(Diff revision 1)
> +        typename F,
> +        typename = typename mozilla::EnableIf<
> +            mozilla::IsSame<bool, decltype((*(F*)nullptr)((ParseTask*)nullptr))>::value
> +        >::Type
> +    >
> +    bool finishParseTask(JSContext* cx, ParseTaskKind kind, void* token, F finish);

I wonder if the extra flexibility from taking a lambda is necessary. This method could be made to always take a MutableHandle<ScriptVector>, for instance, and asserts be made on the length (which is already done anyways).

Do you foresee future variants of OMT parsing?

::: js/src/vm/HelperThreads.h:691
(Diff revision 1)
>                       const JS::TranscodeRange& range,
>                       JS::OffThreadCompileCallback callback, void* callbackData);
>      void parse(JSContext* cx) override;
>  };
>  
> +struct ScriptsDecodeTask : public ParseTask

Ditto on MultiScripts or something.

::: js/src/vm/HelperThreads.cpp:336
(Diff revision 1)
> +    parseGlobal(parseGlobal),
> +    callback(callback), callbackData(callbackData),
> +    overRecursed(false), outOfMemory(false)
> +{
> +    Unused << scripts.reserve(scripts.capacity());
> +    Unused << sourceObjects.reserve(sourceObjects.capacity());

If these aren't nops, should use MOZ_ALWAYS_TRUE instead of Unused.

::: js/src/vm/HelperThreads.cpp:336
(Diff revision 1)
> +    parseGlobal(parseGlobal),
> +    callback(callback), callbackData(callbackData),
> +    overRecursed(false), outOfMemory(false)
> +{
> +    Unused << scripts.reserve(scripts.capacity());
> +    Unused << sourceObjects.reserve(sourceObjects.capacity());

Should use MOZ_ALWAYS_TRUE if these aren't nops.

::: js/src/vm/HelperThreads.cpp:478
(Diff revision 1)
> +ScriptsDecodeTask::parse(JSContext* cx)
> +{
> +    auto sources = data.as<JS::TranscodeSources*>();
> +
> +    if (!scripts.reserve(sources->length()) ||
> +        !sourceObjects.reserve(sources->length()))

Style nit: JS for multi-line conditionals is:

if (foo ||
    bar)
{
    ...
}

::: js/src/vm/HelperThreads.cpp:478
(Diff revision 1)
> +ScriptsDecodeTask::parse(JSContext* cx)
> +{
> +    auto sources = data.as<JS::TranscodeSources*>();
> +
> +    if (!scripts.reserve(sources->length()) ||
> +        !sourceObjects.reserve(sources->length()))

As it stands now, scripts' and sourceObjects' AllocPolicy wouldn't report OOM on cx, so you should manually set outOfMemory = true.

I think that would be unnecessary if you switched to GCVector, though, since the default AllocPolicy for those is js::TempAllocPolicy and takes a cx.

::: js/src/vm/HelperThreads.cpp:488
(Diff revision 1)
> +        opts.setFileAndLine(source.filename, source.lineno);
> +
> +        RootedScript resultScript(cx);
> +        Rooted<ScriptSourceObject*> sourceObject(cx);
> +
> +        XDROffThreadDecoder decoder(cx, alloc, &opts, &sourceObject.get(), source.range);

All these places that take ScriptSourceObject** should just take a MutableHandle<ScriptSourceObject>.

::: js/src/vm/HelperThreads.cpp:736
(Diff revision 1)
> +bool
> +js::StartOffThreadDecodeScripts(JSContext* cx, const ReadOnlyCompileOptions& options,
> +                                JS::TranscodeSources& sources,
> +                                JS::OffThreadCompileCallback callback, void* callbackData)
> +{
> +    auto functor = [&](JSObject* global) -> ScriptsDecodeTask* {

Please explicitly capture all variables. Have had very subtle bugs with auto capturing 'this' before.

::: js/src/vm/HelperThreads.cpp:1378
(Diff revision 1)
>      MOZ_CRASH("Invalid ParseTask token");
>  }
>  
> -JSScript*
> -GlobalHelperThreadState::finishParseTask(JSContext* cx, ParseTaskKind kind, void* token)
> +template <typename F, typename>
> +bool
> +GlobalHelperThreadState::finishParseTask(JSContext* cx, ParseTaskKind kind, void* token, F finish)

Naming nit: would prefer finishHook or finishCallback over just finish.

::: js/src/vm/HelperThreads.cpp:1422
(Diff revision 1)
> +JSScript*
> +GlobalHelperThreadState::finishParseTask(JSContext* cx, ParseTaskKind kind, void* token)
> +{
> +    JS::RootedScript script(cx);
> +
> +    bool ok = finishParseTask(cx, kind, token, [&] (ParseTask* parseTask) {

Explicitly capture all vars.

::: js/src/vm/HelperThreads.cpp:1468
(Diff revision 1)
> +
> +    if (!ok)
> +        return false;
> +
> +    if (scripts.length() != expectedLength) {
> +        // No error was reported, but no script produced. Assume we hit out of

Nit: comment inaccurate. More like "didn't produce as many scripts as expected"

::: js/src/vm/HelperThreads.cpp:1478
(Diff revision 1)
> +
> +    // The Debugger only needs to be told about the topmost script that was compiled.
> +    for (auto& script : scripts) {
> +        MOZ_ASSERT(script->isGlobalCode());
> +
> +        JS::RootedScript rooted(cx, script);

General rule of thumb is manually hoist Rooteds outside of loops and assign to the Rooted each iteration, to avoid the push/pop cost from rooter lists.

Comment 13

[Kris Maglione [:kmag]]

Comment on attachment 8868362 [details]
Bug 1364974: Part 3 - Support decoding multiple scripts in a single parse tasks.

https://reviewboard.mozilla.org/r/139934/#review144786

> Edit distance too small! Maybe MultiScriptsDecode.

Yeah, I was a bit worried about that too. MultiScriptsDecode sounds good to me.

> I wonder if the extra flexibility from taking a lambda is necessary. This method could be made to always take a MutableHandle<ScriptVector>, for instance, and asserts be made on the length (which is already done anyways).
> 
> Do you foresee future variants of OMT parsing?

I thought about having it always take a ScriptVector, but I didn't much like the extra overhead of the vector for the single script case. And with the different set of checks we need for the two cases, the lambda version turned out to be simpler. But I don't have a particularly strong opinion.

As for whether I expect future variants... Yeah, it occurred to me that we have the same amount of extra per-script overhead for script and module loads in web content, and it would be nice if we could find a way to coalesce several async script preloads for a single document the way we do for startup pre-loads. But I don't know how big of a difference it would really make, so I'm mostly just keeping it in the back of my mind.

> As it stands now, scripts' and sourceObjects' AllocPolicy wouldn't report OOM on cx, so you should manually set outOfMemory = true.
> 
> I think that would be unnecessary if you switched to GCVector, though, since the default AllocPolicy for those is js::TempAllocPolicy and takes a cx.

Well, at the moment, we report OOM whenever the task fails to return scripts, and doesn't report any errors, and this bails out early if we fail to reserve space for either of the vectors. So we do report OOM if either of these fails.

That said... The fallback to reporting OOM confused the hell out of me a while back when I had tasks failing for unrelated reasons, so I definitely wouldn't mind making it explicit.

Comment 14

[Shu-yu Guo [:shu]]

Comment on attachment 8868364 [details]
Bug 1364974: Part 5 - Perform off-thread decode operations in chunks, rather than singly.

https://reviewboard.mozilla.org/r/139938/#review144800

To make sure I understand the algorithm here:

1. All scripts are now attempted to be pre-decoded off-thread, unless a) it errors out during OMT decoding b) requested on main thread and is small (as before) c) wasn't used in same process type previous session
2. The next batch is scheduled only when a previous patch successfully finishes.
3. If an error occurs during any batch, no more OMT batches will be scheduled and all scripts will be decoded on the main thread.

I was initially concerned about 3, but since these aren't scripts from the wild but XDR'd internal scripts, errors probably only happen on OOM, which should be systemically catastrophic.

Thanks for your work on this!

::: js/xpconnect/loader/ScriptPreloader.h:333
(Diff revision 1)
> -    // script than several small scripts, since the setup is per-script, and the
> -    // OMT compile is almost always complete by the time we need a given script.
> -    static constexpr int MIN_OFFTHREAD_SIZE = 20 * 1024;
> +    // decode operation and the time the first script is needed, so that chunk
> +    // needs to be fairly small. After the first chunk is finished, we have
> +    // some buffered scripts to fall back on, and a lot more breathing room,
> +    // so the chunks can be a bit bigger, but still not too big.
> +    static constexpr int OFF_THREAD_FIRST_CHUNK_SIZE = 128 * 1024;
> +    static constexpr int OFF_THREAD_CHUNK_SIZE = 512 * 1024;

Were these constants determined by experimentation?

::: js/xpconnect/loader/ScriptPreloader.h:343
(Diff revision 1)
> +    // we're better off processing them as a single chunk.
> +    //
> +    // In order to guarantee that the JS engine will process a chunk
> +    // off-thread, it needs to be at least 100K (which is an implementation
> +    // detail that can change at any time), so make sure that we always hit at
> +    // least that size, with a bit of breating room to be safe.

Typo: breathing room

::: js/xpconnect/loader/ScriptPreloader.h:344
(Diff revision 1)
> +    //
> +    // In order to guarantee that the JS engine will process a chunk
> +    // off-thread, it needs to be at least 100K (which is an implementation
> +    // detail that can change at any time), so make sure that we always hit at
> +    // least that size, with a bit of breating room to be safe.
> +    static constexpr int MIN_OFF_THREAD_DECODE_SIZE = 128 * 1024;

So this min is not in the sense of "must be at least this size to be decoded offthread", but "must be at least this size to be decoded in the next chunk, when out of space in the current chunk". I'd rename it SMALL_SCRIPT_CHUNK_THRESHOLD.

::: js/xpconnect/loader/ScriptPreloader.h:420
(Diff revision 1)
> +    LinkedList<CachedScript> mPendingScripts;
> +
> +    // The lists of scripts and their sources that are currently being decoded
> +    // off-thread.
> +    JS::TranscodeSources mParsingSources;
> +    Vector<CachedScript*> mParsingScripts;

Just to make sure I understand: currently here meaning in the current chunk.

::: js/xpconnect/loader/ScriptPreloader.cpp:730
(Diff revision 1)
> +    // Check for finished operations before locking so that we can move onto
> +    // decoding the next batch as soon as possible after the pending batch is
> +    // ready. If we wait until we hit an unfinished script, we wind up having at
> +    // most one batch of buffered scripts, and occasionally under-running that
> +    // buffer.
> +    if (mToken) {

Unnecessary if (mToken)

::: js/xpconnect/loader/ScriptPreloader.cpp:804
(Diff revision 1)
> +{
> +    if (!mToken) {
> +        return;
> +    }
> +
> +    auto cleanup = MakeScopeExit([&] () {

As before, please make this-capturing explicit. It can lead to some really subtle bugs.

::: js/xpconnect/loader/ScriptPreloader.cpp:809
(Diff revision 1)
> +    auto cleanup = MakeScopeExit([&] () {
> +        mToken = nullptr;
> +        mParsingSources.clear();
> +        mParsingScripts.clear();
> +
> +        DecodeNextBatch();

I'd like to see the default value removed and the size passed in explicitly.

Comment 15

[Kris Maglione [:kmag]]

Comment on attachment 8868364 [details]
Bug 1364974: Part 5 - Perform off-thread decode operations in chunks, rather than singly.

https://reviewboard.mozilla.org/r/139938/#review144800

Correct on all points.

Re point 3, yeah, it should only ever happen during OOM, and at that point, if we somehow manage to finish startup without crashing, we can live with main thread decoding the rest of the scripts. It should be a rare enough corner case that it didn't seem worth trying to handle.

> Were these constants determined by experimentation?

Yeah, these sizes seemed to give the most reliably good results from local testing. I also tested various sizes on talos on try, but without the separate first-chunk-size, and somewhere between 256K and 512K seemed to generally give the best results.

I may wind up doing some more tuning once I get a quantum reference laptop to test on, but for now, these seem to give consistently good enough results to land as is.

> So this min is not in the sense of "must be at least this size to be decoded offthread", but "must be at least this size to be decoded in the next chunk, when out of space in the current chunk". I'd rename it SMALL_SCRIPT_CHUNK_THRESHOLD.

Right. I'll rename.

> Just to make sure I understand: currently here meaning in the current chunk.

Right. I'll expand the comment.

> Unnecessary if (mToken)

Yeah, probably. I was just trying to avoid a function call branch in the case where there's no token, but the compiler is probably smart enough to handle that on its own, anyway.

> As before, please make this-capturing explicit. It can lead to some really subtle bugs.

We generally default to `[&]` for MakeScopeExit because it's guaranteed to be destroyed at the end of the current scope, and most captures are expected. I don't really have an objection to explicit captures, though.

Comment 16

[:shell escalante]

Hi Eric,  I hate to ask, but when do you expect to have time to review Patch 4?  We're trying to land everything this week so it can be fully tested to be included in Beta.

Comment 17

[Eric Rahm [:erahm]]

(In reply to :shell escalante from comment #16)
> Hi Eric,  I hate to ask, but when do you expect to have time to review Patch
> 4?  We're trying to land everything this week so it can be fully tested to
> be included in Beta.

I can finish it today, sorry for the delay!

Comment 18

[Eric Rahm [:erahm]]

Comment on attachment 8868363 [details]
Bug 1364974: Part 4 - Replace CachedScript::mStatus and related logic with original and final process sets.

https://reviewboard.mozilla.org/r/139936/#review145258

Looks okay, just a few questions. r+ either way.

::: js/xpconnect/loader/ScriptPreloader.cpp:539
(Diff revision 1)
> -        if (childScript) {
> +        if (childScript && !childScript->mProcessTypes.isEmpty()) {
> -            if (childScript->mStatus == ScriptStatus::Saved) {
> -                childScript->UpdateLoadTime(script->mLoadTime);
> +            childScript->UpdateLoadTime(script->mLoadTime);
> -                childScript->mProcessTypes += script->mProcessTypes;
> +            childScript->mProcessTypes += script->mProcessTypes;
> -            } else {
> -                childScript = nullptr;
> +            script.Remove();
> +        } else if (!(script->mProcessTypes == script->mOriginalProcessTypes)) {

This is weird enough it could use a comment.

Also it would be nice if EnumSet had an `operator !=` and/or an `operator bool` :( Not your fault of course!

::: js/xpconnect/loader/ScriptPreloader.cpp:550
(Diff revision 1)
> +        mSaveComplete = true;
> +        return;
> +    }
> +
> +    AutoSafeJSAPI jsapi;
> +    for (auto& script : IterHash(mScripts, Match<ScriptStatus::Saved>())) {

Is there a reason we need to double-loop now?

::: js/xpconnect/loader/ScriptPreloader.cpp:816
(Diff revision 1)
>  ScriptPreloader::CachedScript::CachedScript(ScriptPreloader& cache, InputBuffer& buf)
>      : mCache(cache)
> -    , mStatus(ScriptStatus::Restored)
>  {
>      Code(buf);
> +    mOriginalProcessTypes = mProcessTypes;

What's going on here? Is it that `Code` writes to `mProcessTypes` and we need to swap them? Maybe add a comment?

Comment 19

[Kris Maglione [:kmag]]

Comment on attachment 8868363 [details]
Bug 1364974: Part 4 - Replace CachedScript::mStatus and related logic with original and final process sets.

https://reviewboard.mozilla.org/r/139936/#review145258

> This is weird enough it could use a comment.
> 
> Also it would be nice if EnumSet had an `operator !=` and/or an `operator bool` :( Not your fault of course!

Yeah, I thought about adding a != operator but I didn't want to hold up this patch on it :( I'll file a follow-up for it.

> Is there a reason we need to double-loop now?

I guess not. The idea was to avoid encoding any scripts if we weren't going to write a new cache file, but the fact that we have a script to encode probably implies that we need to write a new cache file.

> What's going on here? Is it that `Code` writes to `mProcessTypes` and we need to swap them? Maybe add a comment?

Yeah, `Code` encodes/decodes the `mProcessTypes` field, but in a new session, we want to start out with an empty set of processes loaded into during this session, and then compare against the original.

Comment 20

[Kris Maglione [:kmag]]

Comment on attachment 8868362 [details]
Bug 1364974: Part 3 - Support decoding multiple scripts in a single parse tasks.

https://reviewboard.mozilla.org/r/139934/#review144786

> All these places that take ScriptSourceObject** should just take a MutableHandle<ScriptSourceObject>.

I tried to do this, but the build failed on Windows because MSVC thinks ScriptSourceObject is an incomplete type, and the error messages are so bad that I gave up. I'll file a follow-up bug for this.

Comment 21

[Kris Maglione [:kmag]]

Comment on attachment 8868364 [details]
Bug 1364974: Part 5 - Perform off-thread decode operations in chunks, rather than singly.

https://reviewboard.mozilla.org/r/139938/#review144800

> We generally default to `[&]` for MakeScopeExit because it's guaranteed to be destroyed at the end of the current scope, and most captures are expected. I don't really have an objection to explicit captures, though.

I tried to do this, too, but the clang plugin doesn't allow capturing `this` from refcounted types except by `[&]`

Comment 22

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/946976ce19167bc7d032a6eb277cd94c2039a820
Bug 1364974: Part 1 - Switch to Variant for holding decode task data. r=shu

https://hg.mozilla.org/integration/mozilla-inbound/rev/6a01c2fd2eb7b7d38c9692ff169cecdf2342db5e
Bug 1364974: Part 2 - Support returning multiple scripts in a parse task. r=shu

https://hg.mozilla.org/integration/mozilla-inbound/rev/41d306bebc065c64db754c896a50f4fd60c5adbb
Bug 1364974: Part 3 - Support decoding multiple scripts in a single parse tasks. r=shu

https://hg.mozilla.org/integration/mozilla-inbound/rev/3f85c56d48a0b7006a0c85f636247054ee748619
Bug 1364974: Part 4 - Replace CachedScript::mStatus and related logic with original and final process sets. r=erahm

https://hg.mozilla.org/integration/mozilla-inbound/rev/5a0655f29081298bcad111da0a8fd02655a2abc9
Bug 1364974: Part 5 - Perform off-thread decode operations in chunks, rather than singly. r=shu

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/946976ce1916
https://hg.mozilla.org/mozilla-central/rev/6a01c2fd2eb7
https://hg.mozilla.org/mozilla-central/rev/41d306bebc06
https://hg.mozilla.org/mozilla-central/rev/3f85c56d48a0
https://hg.mozilla.org/mozilla-central/rev/5a0655f29081

Comment 24

[Kris Maglione [:kmag]]

Attachment: Windows Perfherder

From the performance alert:

  5%  ts_paint osx-10-10 opt e10s     1,235.08 -> 1,178.08
  3%  ts_paint windows8-64 opt e10s   877.79 -> 854.58

But I think this probably deserves a graph.

Comment 25

[Kris Maglione [:kmag]]

Attachment: Windows/OS-X Perfherder

Or with OS-X

Comment 26

[Joel Maher ( :jmaher ) (UTC -8)]

and some autophone android improvements:
== Change summary for alert #6794 (as of May 23 2017 16:36 UTC) ==

Improvements:

  4%  remote-blank summary android-7-1-armv8-api15 opt      878.85 -> 840.81
  3%  remote-blank summary android-4-4-armv7-api15 opt      1,228.06 -> 1,186.14
  3%  remote-twitter summary android-6-0-armv8-api15 opt    912.95 -> 883.62
  3%  remote-twitter summary android-7-1-armv8-api15 opt    981.81 -> 951.99
  3%  remote-twitter summary android-4-4-armv7-api15 opt    1,390.16 -> 1,349.76
  3%  remote-nytimes summary android-7-1-armv8-api15 opt    1,239.78 -> 1,204.48
  3%  remote-blank summary android-4-2-armv7-api15 opt      2,012.17 -> 1,954.92

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=6794