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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jon Coppeard (:jonco)]

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

Comment 1

[Jon Coppeard (:jonco)]

Attachment: parallel-failure

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.

Comment 2

[Terrence Cole [: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*?

Comment 3

[Jon Coppeard (:jonco)]

Attachment: parallel-failure

Ok, this is only a problem when called from ParallelFunctions.cpp so it's simple to make that one call an unbarriered version.

Comment 4

[Jon Coppeard (:jonco)]

(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 5

[Terrence Cole [:terrence]]

Comment on attachment 815826 [details] [diff] [review]
parallel-failure

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

Nice! r=me

Comment 6

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c52934a52f94

Comment 7

[Carsten Book [:Tomcat]]

backed this out for test failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=29077828&tree=Mozilla-Inbound

Comment 8

[Jon Coppeard (:jonco)]

Attachment: parallel.patch

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.

Comment 9

[Terrence Cole [:terrence]]

Comment on attachment 817059 [details] [diff] [review]
parallel.patch

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

Yeah, that should be fine. r=me

Comment 10

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1ac241025daf

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1ac241025daf