Closed Bug 976848 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: shu, Assigned: shu)

References

Details

Attachments

(1 file, 2 obsolete files)

Needed for 1) the workstealing scheduler and 2) Math.random()
Blocks: 969722
I didn't do the MRandom fixes. Should be easy, perhaps another good first-ish bug for lth?
Attachment #8381853 - Flags: review?(nmatsakis)
Assignee: nobody → shu
Status: NEW → ASSIGNED
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 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.
Attachment #8381853 - Flags: review?(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.)
After discussion w/ jorendorff, we will have a ForkJoinContext rngState for PJS and not touch the the compartment rngState.
Summary: PJS: Move rngState from compartment into PerThreadData → PJS: Have an additional rngState in ForkJoinContext
Attachment #8382487 - Flags: review?(nmatsakis)
Attachment #8381853 - Attachment is obsolete: true
Attachment #8382487 - Attachment is obsolete: true
Attachment #8382487 - Flags: review?(nmatsakis)
Summary: PJS: Have an additional rngState in ForkJoinContext → PJS: Add a fast thread-local PRNG for the workstealing scheduler
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?
Attachment #8382590 - Flags: review?(nmatsakis) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/c1218ef1628e
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.