Closed Bug 941827 Opened 6 years ago Closed 6 years ago

Use off-main-thread parsing even if GetCPUCount() == 1

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

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-countSplinter 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+
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+
https://hg.mozilla.org/mozilla-central/rev/f4a802140bc7
Status: ASSIGNED → RESOLVED
Closed: 6 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.