Closed Bug 615723 Opened 9 years ago Closed 9 years ago

Turn on methodjit for DOM workers

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- betaN+
status2.0 --- wanted

People

(Reporter: dmandelin, Assigned: dmandelin)

References

Details

(Whiteboard: [cib-workers][fixed-in-tracemonkey])

Attachments

(1 file, 2 obsolete files)

Writing the patch is trivial. The real bug here is deciding what jit options to use with workers. The main problem is that without the jits on and performing as designed, using <= 4 workers may give a perf regression vs. running on the main thread. This seems bad.

Current issues are:

1. methodjit is turned off entirely for workers. 

I think it should just work, but dvander points out there is an unknown number of compartment bugs that may bite us. The key properties we need are:

 (a) A given JSScript may not be run in 2 threads concurrently. I believe the current design and implementation of dom workers reliably enforces this.

 (b) Correctly connecting scripts to compartments. I'm not sure what the fundamental property we need is, but the basic issue is this: we allocate scripts from an ExcecutableAllocator that hangs off a compartment. It's not thread-safe, so we need to be sure that we don't get races on the allocator in this scheme. There may be other, more subtle issues involved as well.

2. tracejit puts its data in JSThreadData. 

There is no affinity between JSThreadData and dom worker. After a thread switch, we can come back to a worker using a different JSThreadData. That means we've left behind our cached code and oracle results, and are now stuffing traces into a new JSThreadData, which we might fill up (mem or globals per trace) forcing flushes. The result is bad tracing perf, as in bug 562455.

Bug 584860 is the fix for that problem.
After talking to bent and dvander about this, my initial thoughts are that we should:

- after beta8 goes out, turn on the methodjit for workers (in nightlies) and see what happens. Hopefully, it just works. Or maybe we fix a few bugs. Or maybe it's a disaster and we know we can't do this for Fx4.

- I think we should turn off tracejit for workers at that time so we don't confound issues.

- Then expidite fixing bug 584860 so we can turn on the tracer in time to get beta coverage on that.

But we should discuss.
blocking2.0: --- → ?
I like this plan, let's do it!

We've been bitten by compartments bugs before, at least one still unknown (cx->compartment is wrong) and one in JSD with similar symptoms and consequences. If there are more of these, the test coverage will be good.
blocking2.0: ? → -
status2.0: --- → wanted
This must block if web workers are to be particularly useful for improving performance. I think we talked about this at the retriage sessions. We should reopen discussion if this seems wrong.

My first planned action is to turn this on in the nightlies. If that blows up we should strongly consider canceling this bug.
blocking2.0: - → beta9+
Whiteboard: [cib-workers]
Assignee: general → dmandelin
Blocks: 621581
Attached patch Patch (obsolete) — Splinter Review
This turns on the methodjit for workers, if mjit is enabled for content in the prefs. Turning off tracejit for now, so that we know mjit is to blame if problems start occurring. We'll turn tracejit back on once this is stable (or we back it out).
Attachment #499920 - Flags: review?(bent.mozilla)
No longer blocks: 562455
Comment on attachment 499920 [details] [diff] [review]
Patch

Sadly we can't use the pref service off the main thread, and I'm not sure about the XUL runtime service. Let's just hardcode the methodjit to always be enabled, as the tracer is?
Attachment #499920 - Flags: review?(bent.mozilla) → review-
Attached patch v2, don't use prefs service (obsolete) — Splinter Review
No problem, that's how I did it at first but I figured I'd try to be more "correct".
Attachment #499920 - Attachment is obsolete: true
We decided to do it this way after talking on IRC. This is how we want to run it eventually anyway.
Attachment #499921 - Attachment is obsolete: true
Attachment #499924 - Flags: review?(bent.mozilla)
Attachment #499924 - Flags: review?(bent.mozilla) → review+
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [cib-workers] → [cib-workers][fixed-in-tracemonkey]
I've had this pref flipped true (all four of method/trace & content/chrome, actually) for several months now with no problems noticed. Was that a non-functional change until now, or have I been pre-testing? I don't code, but it looks to me like the patch just flips the default, in which case the latter?

This is on nightlies/hourlies, vista, FWIW.
(In reply to comment #9)
> I've had this pref flipped true (all four of method/trace & content/chrome,
> actually) for several months now with no problems noticed. Was that a
> non-functional change until now, or have I been pre-testing? 

Pre-testing, but testing something different. We have had all those on, except that the methodjit was off for chrome, which is the browser internal JS. It's good to hear that it basically works!

> I don't code, but it looks to me like the patch just flips the default, in 
> which case the latter?

The patch in this bug flips on the methodjit for web workers. I learned yesterday that the prefs service cannot be used off the main thread, so which jits are used for web workers must be hardcoded. I changed the hardcoding from "tracejit on only" to "tracejit+methodjit on with profiler to control which is used".
Depends on: 621716
No longer depends on: 621716
As per today's meeting, beta 9 will be a time-based release. Marking these all betaN+. Please move it back to beta9+ if you believe it MUST be in the next beta (ie: trunk is in an unshippable state without this)
No longer blocks: 621581
blocking2.0: beta9+ → betaN+
No longer depends on: 606420
Blocks: 621581
Depends on: 621716, 606420
You need to log in before you can comment on or make changes to this bug.