Closed Bug 925397 Opened 7 years ago Closed 7 years ago

GenerationalGC: parallel/scatter jit-tests fail in threadsafe shell build

Categories

(Core :: JavaScript Engine, defect)

x86
All
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: jonco, Assigned: jonco)

References

Details

Attachments

(1 file, 2 obsolete files)

The following tests currently fail in GGC threadsafe shell builds:

FAIL - parallel/scatter-6.js
FAIL - parallel/scatter-7.js
FAIL - parallel/scatter-8.js
Attached patch parallel-failure (obsolete) — Splinter Review
The problem is that we're adding an entry to the store buffer's generic buffer in a parallel thread that doesn't have access to the runtime.  Such threads should never see nursery allocated things so such a barrier should be unnecessary.

The barrier is added by DenseRangeWriteBarrierPost() for updates to multiple contiguous slots in a dense array.

The patch just checks whether a barrier is necessary by traversing the array until it finds an element that's in the nursery.
Attachment #815453 - Flags: review?(terrence)
Comment on attachment 815453 [details] [diff] [review]
parallel-failure

Review of attachment 815453 [details] [diff] [review]:
-----------------------------------------------------------------

The reason we do the barrier this way rather than barriers on the individual elements is that the callers are extremely hot: moving the method out-of-line, much less adding so much code, is not going to work. I think our options are to make a single check on the |rt| arg or to specialize the PJS code to call a version of the function that does not barrier. How did PJS even get its hands on a full JSRuntime*?
Attachment #815453 - Flags: review?(terrence)
Attached patch parallel-failure (obsolete) — Splinter Review
Ok, this is only a problem when called from ParallelFunctions.cpp so it's simple to make that one call an unbarriered version.
Attachment #815453 - Attachment is obsolete: true
Attachment #815826 - Flags: review?(terrence)
(In reply to Terrence Cole [:terrence] from comment #2)

Oh and to answer your question,

> How did PJS even get its hands on a full JSRuntime*?

by calling Cell::runtimeFromAnyThread()...  I feel this might be a good thing to audit at some point.
Comment on attachment 815826 [details] [diff] [review]
parallel-failure

Review of attachment 815826 [details] [diff] [review]:
-----------------------------------------------------------------

Nice! r=me
Attachment #815826 - Flags: review?(terrence) → review+
Attached patch parallel.patchSplinter Review
So it turns out that PJS also uses the main thread to run a parallel slice, so the previous assertion was failing.

I've replaced this by actually asserting that the data copied in does not contain any nursery things.

Is this ok or do you think it will be too heavyweight? It's only called from jit::InitRestParameterPar() so I guess the array shouldn't be too large.
Attachment #815826 - Attachment is obsolete: true
Attachment #817059 - Flags: review?(terrence)
Comment on attachment 817059 [details] [diff] [review]
parallel.patch

Review of attachment 817059 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, that should be fine. r=me
Attachment #817059 - Flags: review?(terrence) → review+
https://hg.mozilla.org/mozilla-central/rev/1ac241025daf
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.