1236104 - Allow scripts from pre-load cache to be off-thread compiled.

Status: REOPENED

(Core :: JavaScript Engine, defect)

Description

[Yury Delendik (:yury)]

Attachment: prefetched script (w/ and wo/ off-thead)

There was an attempt to do that, but it was reverted (see bug 1084009 comment 6). There was no exact reason provided why we cannot do that, but e.g. that's a reasonfor 50-80ms main thread BytecodeCompiler jank during editing of a document at doc.google.com (see also 1154987). I tried to enable off-thread compilation similar to first patch in bug 1084009 and this removes this delay (see the attachment).

Comment 1

[Yury Delendik (:yury)]

Attachment: [WIP] allow off-thread compilation for pre-loaded scripts

https://treeherder.mozilla.org/#/jobs?repo=try&revision=924d878d8155&selectedJob=14987521

Comment 2

[Yury Delendik (:yury)]

Kannan, if you remember, did you remove this logic only to make b2g emulator tests happy?

Comment 3

[Kannan Vijayan [:djvj]]

> Kannan, if you remember, did you remove this logic only to make b2g emulator tests happy?

Yes. Prefetch scripts should work fine otherwise.

Comment 4

[Yury Delendik (:yury)]

Attachment: Allows scripts from pre-load cache to be off-thread compiled.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=223f1a192ff1&selectedJob=15092093

Comment 5

[Kannan Vijayan [:djvj]]

Comment on attachment 8704380 [details] [diff] [review]
Allows scripts from pre-load cache to be off-thread compiled.

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

::: dom/base/nsScriptLoader.cpp
@@ +645,5 @@
>        // The request has already been loaded and there are no pending style
>        // sheets. If the script comes from the network stream, cheat for
>        // performance reasons and avoid a trip through the event loop.
>        if (aElement->GetParserCreated() == FROM_PARSER_NETWORK) {
> +        // Attempt to compile script off-thread first -- it reduces locking of

This looks fine.  The only suggestion I have is that for small scripts, the overhead of scheduling it off thread might be too much.  FOr a 5k script for example, just pumping it through the main thread is probably the best idea.

Elsewhere when we schedule an off-main-thread script parse, we check its size before scheduling it for off-thread compile.

Can we do that here?

Comment 6

[Yury Delendik (:yury)]

(In reply to Kannan Vijayan [:djvj] from comment #5)
> Comment on attachment 8704380 [details] [diff] [review]
> Allows scripts from pre-load cache to be off-thread compiled.
> 
> Review of attachment 8704380 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/base/nsScriptLoader.cpp
> @@ +645,5 @@
> >        // The request has already been loaded and there are no pending style
> >        // sheets. If the script comes from the network stream, cheat for
> >        // performance reasons and avoid a trip through the event loop.
> >        if (aElement->GetParserCreated() == FROM_PARSER_NETWORK) {
> > +        // Attempt to compile script off-thread first -- it reduces locking of
> 
> This looks fine.  The only suggestion I have is that for small scripts, the
> overhead of scheduling it off thread might be too much.  FOr a 5k script for
> example, just pumping it through the main thread is probably the best idea.
> 
> Elsewhere when we schedule an off-main-thread script parse, we check its
> size before scheduling it for off-thread compile.
> 
> Can we do that here?

The AttemptAsyncScriptCompile function, that is called a line below, uses JS::CanCompileOffThread to make this check (see https://dxr.mozilla.org/mozilla-central/source/js/src/jsapi.cpp#4068). The small scripts will be executed on the main thread as expected.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fa29af245357

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/162e40270ada

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/162e40270ada

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/42538c16776c

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/42538c16776c

Comment 11

[Yury Delendik (:yury)]

Attachment: Revert pre-load cache compilation and tiny script limit.

Reverts patches:
https://hg.mozilla.org/mozilla-central/rev/42538c16776c
https://hg.mozilla.org/mozilla-central/rev/162e40270ada

Until Talos regressions and off-thread compilation performance will be investigated. See also bug 1239834 comment 20.

Try run https://treeherder.mozilla.org/#/jobs?repo=try&revision=90dc47a5016c

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4b4955c69be7

Comment 13

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

can we revert this on aurora (v.46), that would help reduce regressions on there.

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4b4955c69be7

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4b4955c69be7

Comment 16

[Yury Delendik (:yury)]

Comment on attachment 8712178 [details] [diff] [review]
Revert pre-load cache compilation and tiny script limit.

Approval Request Comment
[Feature/regressing bug #]: bug 1236104 
[User impact if declined]: possible performance regression for some sites with large scripts
[Describe test coverage new/current, TreeHerder]: landed on m-c
[Risks and why]: none, this is reverse patch of the previously landed features
[String/UUID change made/needed]: none

Comment 17

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8712178 [details] [diff] [review]
Revert pre-load cache compilation and tiny script limit.

Backout for perf regression. Please uplift to aurora.

Comment 18

[Carsten Book [:Tomcat]]

 https://hg.mozilla.org/releases/mozilla-aurora/rev/820774173b75

Comment 19

[Yury Delendik (:yury)]

(changes were reverted; re-opening since original bug was not fixed)

Comment 20

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

I see these changes fixed on inbound and now on Aurora, the problem is for tests like damp, we see the fix on aurora, then on uplift we have what looks to be this backed out.  And on inbound, I don't see signs of this landing again.

:yury, do you have thoughts on this?  maybe I am overlooking something else?

Comment 21

[Yury Delendik (:yury)]

(In reply to Joel Maher (:jmaher) (PTO- Back April 4th) from comment #20)
> I see these changes fixed on inbound and now on Aurora, the problem is for
> tests like damp, we see the fix on aurora, then on uplift we have what looks
> to be this backed out.  And on inbound, I don't see signs of this landing
> again.
> 
> :yury, do you have thoughts on this?  maybe I am overlooking something else?

I think the issue above is not related - the code from this bug is not present in the trees.

Comment 22

[Shu-yu Guo [:shu]]

Yury, what's the status of this? Still perf problems?