PJS: Improve write-barrier to consider out pointers

RESOLVED FIXED in mozilla29

Status

()

Core
JavaScript Engine
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: nmatsakis, Assigned: nmatsakis)

Tracking

unspecified
mozilla29
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(2 attachments)

(Assignee)

Description

4 years ago
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.
(Assignee)

Updated

4 years ago
Assignee: nobody → nmatsakis
(Assignee)

Updated

4 years ago
Blocks: 956281
(Assignee)

Comment 1

4 years ago
Created attachment 8356942 [details] [diff] [review]
Bug933317.diff

Note though that this patch doesn't handle typed arrays.
We should really make typed arrays be treated the same way.
Attachment #8356942 - Flags: review?(shu)

Comment 2

4 years ago
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.
Attachment #8356942 - Flags: review?(shu) → review+
(Assignee)

Comment 3

4 years ago
(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...
(Assignee)

Comment 4

4 years ago
> GuardThreadExclusiveDestination

How about just "GuardThreadExclusive"?

Comment 5

4 years ago
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...
(Assignee)

Comment 6

4 years ago
(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

4 years ago
(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.
(Assignee)

Comment 8

4 years ago
https://tbpl.mozilla.org/?tree=Try&rev=26ee7738c141
(Assignee)

Comment 9

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/d633e3ff2013
https://hg.mozilla.org/mozilla-central/rev/d633e3ff2013
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
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?
Status: RESOLVED → REOPENED
Flags: needinfo?(nmatsakis)
Resolution: FIXED → ---
(Assignee)

Comment 12

4 years ago
Created attachment 8361257 [details] [diff] [review]
Include dummy definitions for use in non-thread-safe builds.
Flags: needinfo?(nmatsakis)

Updated

4 years ago
Attachment #8361257 - Flags: review+
(Assignee)

Updated

4 years ago
Attachment #8361257 - Attachment description: Bug933317.diff → Include dummy definitions for use in non-thread-safe builds.
https://hg.mozilla.org/mozilla-central/rev/bcbe93f41547
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED

Updated

4 years ago
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.