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

REOPENED
Unassigned
(NeedInfo from)

Status

()

REOPENED
3 years ago
a year ago

People

(Reporter: yury, Unassigned, NeedInfo)

Tracking

(Blocks: 1 bug, {perf})

Trunk
mozilla46
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 fixed)

Details

(Whiteboard: [qf:p3])

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

3 years ago
Created attachment 8703216 [details]
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).
(Reporter)

Comment 2

3 years ago
Kannan, if you remember, did you remove this logic only to make b2g emulator tests happy?
Flags: needinfo?(kvijayan)
> Kannan, if you remember, did you remove this logic only to make b2g emulator tests happy?

Yes. Prefetch scripts should work fine otherwise.
Flags: needinfo?(kvijayan)
(Reporter)

Comment 4

3 years ago
Created attachment 8704380 [details] [diff] [review]
Allows scripts from pre-load cache to be off-thread compiled.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=223f1a192ff1&selectedJob=15092093
Attachment #8703218 - Attachment is obsolete: true
(Reporter)

Updated

3 years ago
Attachment #8704380 - Flags: review?(kvijayan)
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?
Attachment #8704380 - Flags: review?(kvijayan) → review+
(Reporter)

Comment 6

3 years ago
(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
(Reporter)

Updated

3 years ago
Keywords: checkin-needed

Comment 8

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/162e40270ada
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox46: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Depends on: 1240099
(Reporter)

Comment 11

3 years ago
Created attachment 8712178 [details] [diff] [review]
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
(Reporter)

Updated

3 years ago
Keywords: checkin-needed
Whiteboard: checking-needed for
(Reporter)

Updated

3 years ago
Whiteboard: checking-needed for → checking-needed for "Revert pre-load cache compilation and tiny script limit."
can we revert this on aurora (v.46), that would help reduce regressions on there.
Flags: needinfo?(ydelendik)
(Reporter)

Comment 16

3 years ago
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
Flags: needinfo?(ydelendik)
Attachment #8712178 - Flags: approval-mozilla-aurora?
Comment on attachment 8712178 [details] [diff] [review]
Revert pre-load cache compilation and tiny script limit.

Backout for perf regression. Please uplift to aurora.
Attachment #8712178 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Reporter)

Updated

3 years ago
Keywords: checkin-needed
Whiteboard: checking-needed for "Revert pre-load cache compilation and tiny script limit." → checking-needed for aurora for "Revert pre-load cache compilation and tiny script limit." patch
 https://hg.mozilla.org/releases/mozilla-aurora/rev/820774173b75
Keywords: checkin-needed
Whiteboard: checking-needed for aurora for "Revert pre-load cache compilation and tiny script limit." patch
(Reporter)

Comment 19

3 years ago
(changes were reverted; re-opening since original bug was not fixed)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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?
Flags: needinfo?(ydelendik)
(Reporter)

Comment 21

3 years ago
(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.
Flags: needinfo?(ydelendik)
Whiteboard: [qf]
Whiteboard: [qf] → [qf:p1]

Comment 22

2 years ago
Yury, what's the status of this? Still perf problems?
Flags: needinfo?(ydelendik)
Whiteboard: [qf:p1] → [qf:p2]
Whiteboard: [qf:p2] → [perf:p3]
Whiteboard: [perf:p3] → [qf:p3]
You need to log in before you can comment on or make changes to this bug.