1687973 - Move stencil instantiation of decode task to main thread

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P3)

Description

[Tooru Fujisawa [:arai]]

To revert the change from bug 1674305.

in short, currently single script decode task uses parse global and instantiate off-main-thread, because of the following reasons:

• there are some case that the time taken by on-main-thread instantiation regresses benchmark score
• if decode task finishes early, it affects scheduling and some benchmark score regresses, because the benchmark itself is bimodal and bad case happens more (see bug 1674305 comments)

bug 1667903 will help the latter, and we can look into this bug after that.

Comment 1

[Tooru Fujisawa [:arai]]

(fixed the wrong bug number in above comment also)

Comment 2

[Tooru Fujisawa [:arai]]

bug 1721413 patch can solve the issue partially, that the number of atomization is reduced.

Comment 3

[Tooru Fujisawa [:arai]]

I'll do the following at the same time, for dependency reasons:

• remove parse global
• remove some helper thread handling from zone and runtime
• remove OffThreadPlaceholderObject
• remove mergeRealm
• simplify atom zone handling

Comment 4

[Tooru Fujisawa [:arai]]

just flipping CompileOptions.useOffThreadParseGlobal results in 2-4% regression in browsertime tp6.

I'm going to land some more cleanup and optimization at the same time:
bug 538450 patch should improve the on-main-thread atomization

WIP: https://treeherder.mozilla.org/jobs?repo=try&group_state=expanded&revision=a2dd1f684de9dee70c3e2bfc8f08a2f92781dcf3

Comment 5

[Tooru Fujisawa [:arai]]

perf result for the WIP
https://treeherder.mozilla.org/perfherder/compare?originalProject=mozilla-central&originalRevision=b16763f1da6bcf31f964744c765c4af480ae551c&newProject=try&newRevision=f2ab61ff90b707f3c261019fb3cde259748c947b&framework=13&page=1&showOnlyComparable=1&showOnlyImportant=1

still there's 2-4% regression

Comment 6

[Tooru Fujisawa [:arai]]

interestingly, the notable regression happens mostly in cold load, that means the regression for the decode case is solved by the patch stack,
but the non-decode case (== parse case) gets affected by it.

I'll investigate how the patch affects non-decode case.

Comment 7

[Tooru Fujisawa [:arai]]

reproduced the regression for buzfeed page loadtime locally.
it was slightly bi-modal, and the frequency of the good case is reduced by the patch stack.

will check microsoft page.

Comment 8

[Tooru Fujisawa [:arai]]

microsoft page also has multimodal behavior for loadtime, but I don't observe any notable regression or difference locally on macOS.
it might be more environment dependent/specific issue.

Comment 9

[Tooru Fujisawa [:arai]]

the regression on the cold load still happens with just removing off-thread parse global.
https://treeherder.mozilla.org/perfherder/compare?originalProject=mozilla-central&originalRevision=b16763f1da6bcf31f964744c765c4af480ae551c&newProject=try&newRevision=8b40041d73ed8dfcfaa382b56de0381ea93ed3fb&framework=13&page=1&showOnlyImportant=1

maybe I've done something wrong that affects cold load.

Comment 10

[Tooru Fujisawa [:arai]]

the "cold" performance is regressed even by just flipping the pref
https://treeherder.mozilla.org/perfherder/compare?originalProject=mozilla-central&originalRevision=b16763f1da6bcf31f964744c765c4af480ae551c&newProject=try&newRevision=658522accac4a2dc91819570f4bb2c3a68b80d69&framework=13&page=1&showOnlyComparable=1&showOnlyImportant=1

I wonder if the "cold" there is something different than web content's cache.

Comment 11

[Tooru Fujisawa [:arai]]

the regression happens with the same base revision, between m-c and try
https://treeherder.mozilla.org/perfherder/compare?originalProject=mozilla-central&originalRevision=b16763f1da6bcf31f964744c765c4af480ae551c&newProject=try&newRevision=df12762eef9a5e7b6af281e8a94cbdc03313d066&framework=13&page=1&showOnlyComparable=1&showOnlyImportant=1

so, it's not from the patch, but something in the harness or binary or somewhere

Comment 12

[Tooru Fujisawa [:arai]]

Attachment: Bug 1687973 - Part 1: Remove CompileOptions.useOffThreadParseGlobal. r?tcampbell!

Changed all off-thread tasks not to use parse global.
Removed bug-1138390.js because off-thread script decode no longer wait for GC.

• ParseTask::parseGlobal is removed in Part 2
• OffThreadParsingMustWaitForGC is removed in Part 3
• GlobalHelperThreadState::parseWaitingOnGC is removed in Part 3
• GlobalHelperThreadState::generateLCovSources is removed in Part 5
• ParseTask::scripts is removed in Part 6
• ModuleObject::fixEnvironmentsAfterRealmMerge is removed in Part 7
• Zone::{setCreatedForHelperThread,clearUsedByHelperThread} are removed in
  bug 538450
• RealmCreationOptions::setMergeable is removed in bug 1655768
• MergeRealms is removed in bug 1655768

Comment 13

[Tooru Fujisawa [:arai]]

Attachment: Bug 1687973 - Part 2: Remove ParseTask::parseGlobal. r?tcampbell!

• Zone::{setHelperThreadOwnerContext,clearUsedByHelperThread} are removed in
  bug 538450
  • JSRuntime::{incOffThreadParsesRunning,decOffThreadParsesRunning} are removed
    in bug 538450

Depends on D131354

Comment 14

[Tooru Fujisawa [:arai]]

Attachment: Bug 1687973 - Part 3: Remove the pending off-thread parse tasks waiting on GC. r?tcampbell!

Depends on D131355

Comment 15

[Tooru Fujisawa [:arai]]

Attachment: Bug 1687973 - Part 4: Replace LeaveParseTaskZone with ParseTask::deactivate. r?tcampbell!

Depends on D131356

Comment 16

[Tooru Fujisawa [:arai]]

Attachment: Bug 1687973 - Part 5: Remove GlobalHelperThreadState::generateLCovSources. r?tcampbell!

Depends on D131358

Comment 17

[Tooru Fujisawa [:arai]]

Attachment: Bug 1687973 - Part 6: Remove ParseTask::scripts. r?tcampbell!

Instead of storing scripts in vector, directly use CompilationGCOutput.

Depends on D131359

Comment 18

[Tooru Fujisawa [:arai]]

Attachment: Bug 1687973 - Part 7: Remove ModuleObject::fixEnvironmentsAfterRealmMerge. r?tcampbell!

Depends on D131360

Comment 19

[Tooru Fujisawa [:arai]]

Attachment: Bug 1687973 - Part 8: Cleanup ParseTask::parse code flow. r?tcampbell!

Depends on D131361

Comment 20

[Tooru Fujisawa [:arai]]

so there are 2 issues, and both are not specific to this patch:

• m-c push and try push aren't comparable, that's the reason why we were seeing "regression" above
• browsertime tp6 warm on automation doesn't cover bytecode cache

then, given I don't see much notable regression in local runs (except the minor change in buzzfeed score),
I'd like to go with the current stack

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cc1447c123a6
Part 1: Remove CompileOptions.useOffThreadParseGlobal. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/c2b3f1fbe182
Part 2: Remove ParseTask::parseGlobal. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/961d4175ecd0
Part 3: Remove the pending off-thread parse tasks waiting on GC. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/018beff38705
Part 4: Replace LeaveParseTaskZone with ParseTask::deactivate. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/607a5cf2b64f
Part 5: Remove GlobalHelperThreadState::generateLCovSources. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/84e011cd9c89
Part 6: Remove ParseTask::scripts. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/3ca43499f72f
Part 7: Remove ModuleObject::fixEnvironmentsAfterRealmMerge. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/3d7de0571335
Part 8: Cleanup ParseTask::parse code flow. r=tcampbell

Comment 22

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/cc1447c123a6
https://hg.mozilla.org/mozilla-central/rev/c2b3f1fbe182
https://hg.mozilla.org/mozilla-central/rev/961d4175ecd0
https://hg.mozilla.org/mozilla-central/rev/018beff38705
https://hg.mozilla.org/mozilla-central/rev/607a5cf2b64f
https://hg.mozilla.org/mozilla-central/rev/84e011cd9c89
https://hg.mozilla.org/mozilla-central/rev/3ca43499f72f
https://hg.mozilla.org/mozilla-central/rev/3d7de0571335