Open
Bug 1236104
Opened 9 years ago
Updated 2 years ago
Allow scripts from pre-load cache to be off-thread compiled.
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
Tracking | Status | |
---|---|---|
firefox46 | --- | fixed |
People
(Reporter: yury, Unassigned)
References
(Blocks 1 open bug)
Details
(Keywords: perf)
Attachments
(3 files, 1 obsolete file)
404.60 KB,
image/png
|
Details | |
2.08 KB,
patch
|
djvj
:
review+
|
Details | Diff | Splinter Review |
2.98 KB,
patch
|
lizzard
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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 1•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=924d878d8155&selectedJob=14987521
Reporter | ||
Comment 2•9 years ago
|
||
Kannan, if you remember, did you remove this logic only to make b2g emulator tests happy?
Flags: needinfo?(kvijayan)
Comment 3•9 years ago
|
||
> 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•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=223f1a192ff1&selectedJob=15092093
Attachment #8703218 -
Attachment is obsolete: true
Reporter | ||
Updated•9 years ago
|
Attachment #8704380 -
Flags: review?(kvijayan)
Comment 5•9 years ago
|
||
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•9 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•9 years ago
|
Keywords: checkin-needed
Comment 8•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/162e40270ada
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Comment 10•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/42538c16776c
Reporter | ||
Comment 11•9 years ago
|
||
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•9 years ago
|
Keywords: checkin-needed
Whiteboard: checking-needed for
Reporter | ||
Updated•9 years ago
|
Whiteboard: checking-needed for → checking-needed for "Revert pre-load cache compilation and tiny script limit."
Comment 12•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b4955c69be7
Keywords: checkin-needed
Comment 13•9 years ago
|
||
can we revert this on aurora (v.46), that would help reduce regressions on there.
Flags: needinfo?(ydelendik)
Comment 14•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4b4955c69be7
Comment 15•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4b4955c69be7
Reporter | ||
Comment 16•9 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 17•9 years ago
|
||
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•9 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
Comment 18•9 years ago
|
||
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•9 years ago
|
||
(changes were reverted; re-opening since original bug was not fixed)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 20•8 years ago
|
||
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•8 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)
Updated•7 years ago
|
Whiteboard: [qf]
Updated•7 years ago
|
Whiteboard: [qf] → [qf:p1]
Comment 22•7 years ago
|
||
Yury, what's the status of this? Still perf problems?
Flags: needinfo?(ydelendik)
Updated•7 years ago
|
Whiteboard: [qf:p1] → [qf:p2]
Updated•7 years ago
|
Whiteboard: [qf:p2] → [perf:p3]
Updated•7 years ago
|
Whiteboard: [perf:p3] → [qf:p3]
Reporter | ||
Updated•3 years ago
|
Flags: needinfo?(ydelendik)
Updated•2 years ago
|
Performance Impact: --- → P3
Whiteboard: [qf:p3]
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•