976848 - PJS: Add a fast thread-local PRNG for the workstealing scheduler

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Shu-yu Guo [:shu]]

Needed for 1) the workstealing scheduler and 2) Math.random()

Comment 1

[Shu-yu Guo [:shu]]

Attachment: Move rngState from JSCompartment to PerThreadData.

I didn't do the MRandom fixes. Should be easy, perhaps another good first-ish bug for lth?

Comment 2

[Jan de Mooij [:jandem]]

You should talk to jorendorff and Jesse about this as well. Bug 820180 moved rngState from the context to the compartment, with this patch the RNG state is effectively shared across all compartments again.

Comment 3

[Niko Matsakis [:nmatsakis]]

Comment on attachment 8381853 [details] [diff] [review]
Move rngState from JSCompartment to PerThreadData.

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

This looks fine to me. I would give r+ but for jandem's comment, which makes me think I lack the authority to r+ this change on my own.

Comment 4

[Niko Matsakis [:nmatsakis]]

I just read over bug 820180. Indeed I think this change re-opens the side channel attack mentioned there. I'm not sure what's the best fix -- we don't, at present, have a notion of "per compartment and per thread", do we? (I'll have to go review; these things have changed a few times and I forget the current state.)

Comment 5

[Shu-yu Guo [:shu]]

After discussion w/ jorendorff, we will have a ForkJoinContext rngState for PJS and not touch the the compartment rngState.

Comment 6

[Shu-yu Guo [:shu]]

Attachment: Add a per-ThreadPoolWorker RNG state.

Comment 7

[Shu-yu Guo [:shu]]

Attachment: Add a 32-bit xorshift to ThreadPoolWorker for thread-local PRNG for workstealing.

Comment 8

[Niko Matsakis [:nmatsakis]]

Comment on attachment 8382590 [details] [diff] [review]
Add a 32-bit xorshift to ThreadPoolWorker for thread-local PRNG for workstealing.

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

Generally good but I'm wondering about the initial seed generation (as spelled out below). And of course I wonder how we can test this random function. I guess we just take it on faith :)

::: js/src/vm/ThreadPool.cpp
@@ +317,2 @@
>      for (uint32_t workerId = 0; workerId < numWorkers(); workerId++) {
> +        uint32_t rngSeed = uint32_t(random_next(&rngState, 32));

If I understand correctly, the seeds for each worker will always be the same, and hence they will always steal in the same pattern. This seems unfortunate. Can we use the state from the compartment instead or something?

Comment 9

[Shu-yu Guo [:shu]]

(In reply to Niko Matsakis [:nmatsakis] from comment #8)
> Comment on attachment 8382590 [details] [diff] [review]
> Add a 32-bit xorshift to ThreadPoolWorker for thread-local PRNG for
> workstealing.
> 
> Review of attachment 8382590 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Generally good but I'm wondering about the initial seed generation (as
> spelled out below). And of course I wonder how we can test this random
> function. I guess we just take it on faith :)
> 
> ::: js/src/vm/ThreadPool.cpp
> @@ +317,2 @@
> >      for (uint32_t workerId = 0; workerId < numWorkers(); workerId++) {
> > +        uint32_t rngSeed = uint32_t(random_next(&rngState, 32));
> 
> If I understand correctly, the seeds for each worker will always be the
> same, and hence they will always steal in the same pattern. This seems
> unfortunate. Can we use the state from the compartment instead or something?

The seeds will not be the same. A rngState == 0 is a sentinel state that tells the random_next function to initialize the seed according to the platform. On Unix, this is a read from /dev/urandom. On Windows, this is rand_s. I pass it a fresh seed to initialize to not mess with the one that lives in the current compartment.

Comment 10

[Shu-yu Guo [:shu]]

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

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c1218ef1628e