Closed
Bug 941827
Opened 11 years ago
Closed 11 years ago
Use off-main-thread parsing even if GetCPUCount() == 1
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
VERIFIED
FIXED
mozilla28
Tracking | Status | |
---|---|---|
firefox28 | --- | verified |
People
(Reporter: luke, Assigned: luke)
References
Details
Attachments
(2 files)
(deleted),
patch
|
bhackett1024
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
nbp
:
feedback+
|
Details | Diff | Splinter Review |
We don't create worker threads if GetCPUCount() == 1 mainly, I think, to avoid hurting benchmark performance where we're not really winning extra CPU cycles and we're losing from the increased job latency and the lower compilation threshold. However, for off-main-thread parsing, I think it makes sense to parse off the main thread, even if GetCPUCount() == 1, as this would still improve responsiveness for large async scripts and there isn't the same latency requirement as there is with Ion compilation jobs.
The rough idea is to have a separate "number of total JS worker threads" and "number of JS worker threads available for Ion compilation", the former being GetCPUCount() (unless explicitly overridden as it can be now), the latter being the current value.
Assignee | ||
Comment 1•11 years ago
|
||
This patch also cleans up the bits in JSRuntime and adds some comments so it's easier to follow what means what.
One thing I tripped on is that, with only one helper thread, it's easy to get into a deadlock between a off-main-thread parse task (which, with caching, waits to dispatch a runnable to the main thread) and the source compression (which, when run on the main thread, waits for the compression task to complete in a helper thread).
A general fix I thought about would be to have an atomic counter in the WorkerThreadState indicating the number of outstanding tasks that *might* block on the main thread. This would allow ScriptSource::setSourceCopy to know that it wouldn't deadlock by dispatching/waiting a compression task. This would also allow us to remove the one-at-a-time limitation in canStartParseTask.
Instead, to keep this patch simple, I just had WorkerThreadState create 2 worker threads when GetCPUCount() == 1. I'd be happy to do the more general solution in a later bug if it seems like a good idea.
Updated•11 years ago
|
Attachment #8336444 -
Flags: review?(bhackett1024) → review+
Assignee | ||
Comment 2•11 years ago
|
||
This patch fixes the unintended change in behavior of the original patch where we'd use background compression even if GetCPUCount() == 1, which I suspect is what caused the regression nbp was seeing on Octane on a 1-core FFOS phone.
Nicolas: with only this patch applied, do you see any regression over trunk?
Attachment #8339078 -
Flags: feedback?(nicolas.b.pierron)
Comment 3•11 years ago
|
||
Comment on attachment 8339078 [details] [diff] [review]
fix-cpu-count v.2
Review of attachment 8339078 [details] [diff] [review]:
-----------------------------------------------------------------
With the patch applied:
Shell-like octane results:
Richards: 1168
DeltaBlue: 240
Crypto: 1230
RayTrace: 202
EarleyBoyer: 434
RegExp: 113
Splay: 269
NavierStokes: 1238
PdfJS: 283
Mandreel: 255
Gameboy: 783
CodeLoad: 1108
Box2D: 279
Score: 435
End of shell-like results.
I also ran without any patch applied (after noticing that I did not had the exact same configuration as AWFY):
Shell-like octane results:
Richards: 1151
DeltaBlue: 228
Crypto: 1227
RayTrace: 199
EarleyBoyer: 434
RegExp: 112
Splay: 297
NavierStokes: 1287
PdfJS: 272
Mandreel: 252
Gameboy: 710
CodeLoad: 1045
Box2D: 295
Score: 431
End of shell-like results.
Attachment #8339078 -
Flags: feedback?(nicolas.b.pierron) → feedback+
Assignee | ||
Comment 4•11 years ago
|
||
Thanks for measuring!
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4a802140bc7
Comment 5•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Assuming verified based on the testing in comment 3.
status-firefox28:
--- → verified
You need to log in
before you can comment on or make changes to this bug.
Description
•