933317 - PJS: Improve write-barrier to consider out pointers

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Niko Matsakis [:nmatsakis]]

The current write barrier code checks whether the target object is allocated in the current thread. This has been fine until now but is inadequate for typed objects / handles -- in the case of a handle, the time when the handle was allocated is not relevant; rather what's relevant is when the *underlying owner* of the memory was allocated.

Out pointers introduce another wrinkle, since the normal rule of "you can write to the things you allocated" would presumably rule out writes through the out pointer, since the final result was allocated before par exec began. Therefore, we need to extend the par section with the notion of a *target object* (or possibly *target memory range*, see below).

The full write barrier logic could then be written as follows:
 - Full generality:
    if (target is handle) {
        target = target.owner;
    }
    if (target == threadLocal.destObject || target.isThreadLocal()) {
        /* ok */
    } else {
        error
    }

However, using TI we will almost always be able to narrow that down to something much simpler:

  - If TI predicts that the target of the write is a handle, we can specialize to:
    if (target.owner == threadLocal.destObject || target.owner.isThreadLocal())
  - if TI predicts it is DEFINITELY NOT a handle:
    if (target.isThreadLocal()) { ... }

Note: if we wind up removing handles from the API, or we do not make handles nullable, then a single destination object is inadequate. We must have a "target memory range". The reason is that, during sequential fallback, the user could allow a handle to escape into a global object, and then during par execution could obtain that handle and try to use it. This would permit reads/writes to some portion of the array that was already filled in by the seq code. Reads are ok but writes introduce races. Limiting writes to a target memory range, which is a subportion of the output buffer, rather than the full buffer, would address this problem so that even if users obtained a stray handle, they can't make writes through it.

Comment 1

[Niko Matsakis [:nmatsakis]]

Attachment: Bug933317.diff

Note though that this patch doesn't handle typed arrays.
We should really make typed arrays be treated the same way.

Comment 2

[Shu-yu Guo [:shu]]

Comment on attachment 8356942 [details] [diff] [review]
Bug933317.diff

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

r=me with comments addressed, mainly the name and account for the size of the handle.

::: js/src/jit/MIR.h
@@ +8505,5 @@
>  };
>  
> +// Guard on an object being safe for writes by current parallel slice.
> +// Must be either thread-local or else a handle into the destination array.
> +class MGuardParallelWrite

I don't think the ParallelWrite name is descriptive enough. Perhaps GuardThreadExclusiveDestination? Too long?

::: js/src/jit/ParallelFunctions.cpp
@@ +54,5 @@
> +            return true;
> +
> +        // Also check whether owner is thread-local.
> +        TypedDatum *owner = datum.owner();
> +        return owner && IsThreadLocalObject(slice, owner);

I would like a more general comment describing the logic than these two separate comments. Something like: Typed data are considered writable if they are within the memory region designated for the current slice or if their owner is thread local.

@@ +67,5 @@
>  jit::IsThreadLocalObject(ForkJoinSlice *slice, JSObject *object)
>  {
>      JS_ASSERT(ForkJoinSlice::Current() == slice);
>      return slice->isThreadLocal(object);
>  }

This function should be folded into ParallelWriteGuard since nothing else calls it anymore, and users should be using slice->isThreadLocal anyways.

@@ +69,5 @@
>      JS_ASSERT(ForkJoinSlice::Current() == slice);
>      return slice->isThreadLocal(object);
>  }
>  
> +// Check that `object` (which must be a typed datum) maps

Nit: code quotes are |s here and elsewhere.

@@ +77,5 @@
> +{
> +    JS_ASSERT(IsTypedDatum(*datum)); // in case JIT supplies something bogus
> +    uint8_t *typedMem = datum->typedMem();
> +    return (typedMem >= slice->targetRegionStart &&
> +            typedMem <  slice->targetRegionEnd);

What is the size of the data written by the handle? I feel like that needs to play a role here. If |typedMem| is just 1 byte before |slice->targetRegionEnd| and I write a uint32 or a float64 or something, wouldn't that be unsafe?

::: js/src/vm/ForkJoin.cpp
@@ +2228,5 @@
>  
> +bool
> +js::intrinsic_SetForkJoinTargetRegion(JSContext *cx, unsigned argc, Value *vp)
> +{
> +    return true; // when not in.

I don't understand this comment.

Comment 3

[Niko Matsakis [:nmatsakis]]

(In reply to Shu-yu Guo [:shu] from comment #2)
> @@ +67,5 @@
> >  jit::IsThreadLocalObject(ForkJoinSlice *slice, JSObject *object)
> >  {
> >      JS_ASSERT(ForkJoinSlice::Current() == slice);
> >      return slice->isThreadLocal(object);
> >  }
>
> This function should be folded into ParallelWriteGuard since nothing else
> calls it anymore, and users should be using slice->isThreadLocal anyways.

Yes, ok. I left it separate because I figured that in a later patch we will optimize the code to specialize the case of a non-typed-object, but we can pull the check into a separate routine at that time.

> What is the size of the data written by the handle? I feel like that needs
> to play a role here. If |typedMem| is just 1 byte before
> |slice->targetRegionEnd| and I write a uint32 or a float64 or something,
> wouldn't that be unsafe?

I believe it is ok as is, essentially because we never create handles that straddle the target region. For example, if we create an out pointer for element i of the array, the target region will be the start of element i (inclusive) through the start of i+1 (exclusive). You cannot create a handle that covers some portion of element i and some portion of element i+1. But I should mention this invariant when describing the target region -- that it must always correspond to complete elements/properties.

> ::: js/src/vm/ForkJoin.cpp
> @@ +2228,5 @@
> >  
> > +bool
> > +js::intrinsic_SetForkJoinTargetRegion(JSContext *cx, unsigned argc, Value *vp)
> > +{
> > +    return true; // when not in.
> 
> I don't understand this comment.

Me either. :) Looks like I never finished it. I'll try to figure out what I was trying to say...

Comment 4

[Niko Matsakis [:nmatsakis]]

> GuardThreadExclusiveDestination

How about just "GuardThreadExclusive"?

Comment 5

[Shu-yu Guo [:shu]]

To recap discussion on IRC: my worry about overlapping bounds is guaranteed impossible by higher level semantics than typed object handles. Namely, it's up to the ForkJoin API itself to ensure that all its slices are disjoint.

I like ThreadExclusive. I do worry that it might be confused with ExclusiveContext, but maybe that's more of ExclusiveContext's fault...

Comment 6

[Niko Matsakis [:nmatsakis]]

(In reply to Shu-yu Guo [:shu] from comment #5)
> I like ThreadExclusive. I do worry that it might be confused with
> ExclusiveContext, but maybe that's more of ExclusiveContext's fault...

I kind of figured you were deliberately making the names similar.

Comment 7

[Shu-yu Guo [:shu]]

(In reply to Niko Matsakis [:nmatsakis] from comment #6)
> (In reply to Shu-yu Guo [:shu] from comment #5)
> > I like ThreadExclusive. I do worry that it might be confused with
> > ExclusiveContext, but maybe that's more of ExclusiveContext's fault...
> 
> I kind of figured you were deliberately making the names similar.

No, I just wanted a more descriptive name than ParallelWrite. Something with a hint as to the condition it is guarding.

Comment 8

[Niko Matsakis [:nmatsakis]]

https://tbpl.mozilla.org/?tree=Try&rev=26ee7738c141

Comment 9

[Niko Matsakis [:nmatsakis]]

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

Comment 10

[Ed Morley [:emorley]]

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

Comment 11

[Christian Holler (:decoder)]

This broke non-threadsafe debug builds:

js/src/debug64/js/src/Unified_cpp_js_src10.o:Unified_cpp_js_src10.cpp:function intrinsic_functions: error: undefined reference to 'js::intrinsic_SetForkJoinTargetRegionInfo'
collect2: ld returned 1 exit status

The solution should be to wrap the use of intrinsic_SetForkJoinTargetRegionInfo into the proper ifdef?

Comment 12

[Niko Matsakis [:nmatsakis]]

Attachment: Include dummy definitions for use in non-thread-safe builds.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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