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)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla28
Tracking Status
firefox28 --- verified

People

(Reporter: luke, Assigned: luke)

References

Details

Attachments

(2 files)

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.
Attached patch fix-cpu-count (deleted) — Splinter Review
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.
Assignee: nobody → luke
Status: NEW → ASSIGNED
Attachment #8336444 - Flags: review?(bhackett1024)
Blocks: 942276
Attachment #8336444 - Flags: review?(bhackett1024) → review+
Attached patch fix-cpu-count v.2 (deleted) — Splinter Review
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 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+
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Depends on: 945189
Depends on: 950513
Assuming verified based on the testing in comment 3.
Status: RESOLVED → VERIFIED
No longer blocks: 987556
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: