615723 - Turn on methodjit for DOM workers

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[David Mandelin [:dmandelin]]

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.

Comment 1

[David Mandelin [:dmandelin]]

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.

Comment 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

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.

Comment 3

[David Mandelin [:dmandelin]]

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.

Comment 4

[David Mandelin [:dmandelin]]

Attachment: Patch

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).

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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?

Comment 6

[David Mandelin [:dmandelin]]

Attachment: v2, don't use prefs service

No problem, that's how I did it at first but I figured I'd try to be more "correct".

Comment 7

[David Mandelin [:dmandelin]]

Attachment: v3, just use default brower config

We decided to do it this way after talking on IRC. This is how we want to run it eventually anyway.

Comment 8

[David Mandelin [:dmandelin]]

http://hg.mozilla.org/tracemonkey/rev/ba1a2eb997b3
http://hg.mozilla.org/mozilla-central/rev/23e08ae80f49

Comment 9

[bomfog]

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.

Comment 10

[David Mandelin [:dmandelin]]

(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".

Comment 11

[LegNeato]

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)