1083600 - Use inline data for small transparent typed objects

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

Attachment: patch

Since bug 1069688 we now support opaque typed objects with inline data.  It would be nice to do the same thing for transparent typed objects too --- even if opaque objects are made the default and are most commonly used, there will still be cases where we create transparent objects and it would be nice if these didn't have a big performance regression attached to them.

The attached patch allows small transparent typed objects to be created with inline data.  All the complexity here is in allowing these objects to have array buffers constructed for them, which is done using a weak map.  This makes neutering checks more complicated so I just stopped doing them in jitcode entirely, using a new TI object flag to indicate whether any buffers with sized typed object views have been neutered.

Comment 1

[Brian Hackett [Laid off!]]

Oh, ignore the dom/webidl change, that was accidental (leftover in my tree from testing bug 1073842).

Comment 2

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8505903 [details] [diff] [review]
patch

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

Can you also augment the documentation in ABO.h above the ArrayBufferObject class declaration to mention the possibility of foreign storage?

::: js/src/builtin/TypedObject.cpp
@@ +2513,5 @@
> +        typedef HashMap<JSObject *, JSObject *> UnbarrieredMap;
> +        UnbarrieredMap *unbarrieredMap = reinterpret_cast<UnbarrieredMap *>(baseHashMap);
> +
> +        typedef gc::HashKeyRef<UnbarrieredMap, JSObject *> Ref;
> +        cx->runtime()->gc.storeBuffer.putGeneric(Ref(unbarrieredMap, obj));

Bleh. I have a somewhat better thing to use for this, but it mostly stuffs things under the rug. Not for this patch, anyway.

::: js/src/builtin/TypedObject.h
@@ +73,5 @@
> + * OutlineTypedObject: Typed objects whose data is owned by another object,
> + *   which can be either an array buffer or an inline typed object. Outline
> + *   typed objects may be attached or unattached. An unattached typed object
> + *   has no memory associated with it. When first created, objects are always
> + *   attached, but they can become unattached if their buffer is neutered.

The part about the shape is stale.

The distinction between "data" and "memory" is unclear here. I think you separated them so that an object could have no memory, but still say that its data is stored in another object. But when reading this, I immediately wondered what the difference between data and memory was.

Maybe: an OutlineTypedObject is one whose data, when attached, is stored in either an array buffer or an InlineTypedObject. And then maybe describe how to detect neutering (detachment?) for OutlineTypedObjects and InlineTypedObjects? The last sentence here is good.

@@ +800,5 @@
> +    static InlineTypedObject *create(JSContext *cx, HandleTypeDescr descr);
> +};
> +
> +// Class for a transparent typed object with inline data, which may have a
> +// lazily allocated array buffer stored in its shape.

Stale comment.

@@ +1080,5 @@
> +    // Keys in this map are InlineTransparentTypedObjects and values are
> +    // ArrayBufferObjects, but we don't enforce this in the type system due to
> +    // the extra marking code goop that requires.
> +    typedef WeakMap<PreBarrieredObject, RelocatablePtrObject> Map;
> +    Map map;

I'm kind of thinking that we should make a 

  WeakMap< PreBarriered<T>, RelocatablePtr<U> >

specialization that defines an asUnbarriered() that returns a WeakMap<T, U>

::: js/src/gc/Nursery.cpp
@@ +396,3 @@
>          // Figure out the size of this object, from the prototype's TypeDescr.
>          // The objects we are traversing here are all tenured, so we don't need
>          // to check forwarding pointers.

Can that be asserted? (If somebody wanted to change that in the future, this would be a good place to get an assert.)

::: js/src/jit/IonBuilder.cpp
@@ +9282,5 @@
>  
> +    // The typed object cannot be neutered.
> +    types::TypeObjectKey *globalType = types::TypeObjectKey::get(&script()->global());
> +    if (globalType->hasFlags(constraints(), types::OBJECT_FLAG_SIZED_OBJECT_NEUTERED))
> +        return true;

How about:

// A sized typed object in this compartment has been neutered, so we require full neuter checks that we do not JIT.

or if you want to stick closer to your original

// The typed object cannot be neutered by setting its length to zero, so we may require a slow path.

Or something. The current comment is too brief for my limited brain to grasp.

::: js/src/jscompartment.cpp
@@ +56,5 @@
>      regExps(runtime_),
>      globalWriteBarriered(false),
>      propertyTree(thisForCtor()),
>      selfHostingScriptSource(nullptr),
> +    lazyBuffers(nullptr),

At the very least, this should be lazyArrayBuffers.

::: js/src/jscompartment.h
@@ +113,5 @@
>  
>  namespace js {
>  class AutoDebugModeInvalidation;
>  class DebugScopes;
> +class LazyBufferTable;

I keep pondering it, and I'm still not sure about this name. Laziness is important, but I don't know if it's the most relevant thing to call out. And "buffer" doesn't necessarily imply "array buffer".

Hm... how about LazyArrayBuffers? Or ExternalArrayBuffers?

@@ +282,3 @@
>      js::InnerViewTable innerViews;
>  
> +    // Map from typed objects to array buffers lazily created for them.

// Map from inline typed objects to array buffers lazily created for them, which will point to the typed objects' inline memory.

::: js/src/jsinfer.h
@@ +485,5 @@
>      /*
> +     * For a global object, whether any array buffers in this compartment with
> +     * sized typed object views have been neutered.
> +     */
> +    OBJECT_FLAG_SIZED_OBJECT_NEUTERED = 0x00400000,

This setup is subtle enough that I'd like a mention of why this is tracked (ie, the length check is inadequate.)

::: js/src/vm/ArrayBufferObject.cpp
@@ +314,5 @@
> +    // accesses such views needs to be deoptimized so that neuter checks are
> +    // performed.
> +    if (buffer->hasSizedObjectViews()) {
> +        // Make sure the object's type has been instantiated, so the flag
> +        // change is reflected in type information.

Maybe something like:

// When neutering a buffer with sized typed object views, any jitcode that
// accesses such views needs to be deoptimized so that neuter checks are
// performed. This is done by setting a compartment-wide
// OBJECT_FLAG_SIZED_OBJECT_NEUTERED flag.

...then...

// Make sure the global object's type has been instantiated so the flag change will be observed.

Or something. On first reading, I found the above comment totally cryptic.

@@ +332,5 @@
> +    if (buffer->firstView()) {
> +        if (buffer->firstView()->is<InlineTransparentTypedObject>()) {
> +            // The buffer points to inline data in its first view, so to keep
> +            // this pointer alive we don't clear out the first view.
> +            MOZ_ASSERT(buffer->viewListIncomplete());

This feels magical and brittle. Is it just as good to say

if (buffer->firstView()) {
  if (buffer->isDataOwnedByFirstView()) {
    MOZ_ASSERT(buffer->firstView()->is<InlineTransparentTypedObject>());
  } else {
    ...
  }
}

?

@@ +810,5 @@
> +ArrayBufferObject::trace(JSTracer *trc, JSObject *obj)
> +{
> +    // If this buffer is associated with an inline transparent typed object,
> +    // fix up the data pointer if the typed object was moved.
> +    ArrayBufferObject &buf = obj->as<ArrayBufferObject>();

I think isDataOwnedByFirstView() would read better.

::: js/src/vm/ArrayBufferObject.h
@@ +112,5 @@
>      enum BufferKind {
>          PLAIN_BUFFER        =   0, // malloced or inline data
>          ASMJS_BUFFER        = 0x1,
>          MAPPED_BUFFER       = 0x2,
>          KIND_MASK           = ASMJS_BUFFER | MAPPED_BUFFER

This is going to have to be rebased past luke's changes now.

@@ +135,5 @@
> +        // missing views will be sized typed objects.
> +        VIEW_LIST_INCOMPLETE = 0x10,
> +
> +        // Views of this buffer might include sized typed objects.
> +        SIZED_OBJECT_VIEWS  = 0x20

How about:

// This ArrayBuffer points to data stored inline in its firstView(). The list of views will be missing any sized typed objects based on the owning view. This flag is used for assertions and to prevent asm.js from using such buffers.
FIRST_VIEW_OWNS_DATA = 0x...,

// Views of this buffer might include sized typed objects. This flag is used to decide whether checking length==0 is adequate for an is-detached check. If any object with this flag is neutered, then the whole compartment is marked as requiring full neuter checks.
SIZED_OBJECT_VIEWS = 0x...,

I tried to come up with a general name for the flag, but ended up deciding that it was easier to understand with the specific example of sized typed objects. It's unfortunate that a reader would have to know about typed objects, and sized vs unsized, and transparent vs opaque, and inline vs outline data, in order to understand what this flag means and why it's here. "May have views that would require looking at the buffer to detect neutering" is hard to condense into a FLAG_NAME.

Comment 3

[Niko Matsakis [:nmatsakis]]

Comment on attachment 8505903 [details] [diff] [review]
patch

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

Sorry again for the delay. I second most everything sfink wrote: in every case where I got confused, I found that reading his review comments made it all make sense.

Two things that may be missing are:

1. a comment in ArrayBufferObject.h explaining the overall ownership strategy: it didn't seem to be written down in a central place. 
2. a comment (perhaps in the text in TypedObject.h) explaining the jit barrier strategy

It's tough to comment some of these things centrally, I suppose, since they seem to spread across several modules.

::: dom/webidl/Node.webidl
@@ -25,5 @@
>    const unsigned short DOCUMENT_NODE = 9;
>    const unsigned short DOCUMENT_TYPE_NODE = 10;
>    const unsigned short DOCUMENT_FRAGMENT_NODE = 11;
>    const unsigned short NOTATION_NODE = 12; // historical
> -  [Constant]

(I don't actually know anything about webidl or what is going on here.)

::: js/src/builtin/TypedObject.cpp
@@ +2355,5 @@
>  {
>  #ifdef DEBUG
> +    // Compute offset of private data based on OutlineTransparentTypedObject;
> +    // both OpaqueOutlineTypedObject and OutlineTransparentTypedObject have the
> +    // same number of slots, so no problem there.

Can we assert this?

::: js/src/jit/IonBuilder.cpp
@@ +7231,5 @@
> +        // If we are not loading the length from the object itself, only
> +        // optimize if the array buffer can't have been neutered.
> +        types::TypeObjectKey *globalType = types::TypeObjectKey::get(&script()->global());
> +        if (globalType->hasFlags(constraints(), types::OBJECT_FLAG_SIZED_OBJECT_NEUTERED))
> +            return false;

I was never crazy about the "canBeNeutered" logic. This compromise (no opt for neutering) makes sense at the moment. I imagine we may want to improve this case later.

@@ +9282,5 @@
>  
> +    // The typed object cannot be neutered.
> +    types::TypeObjectKey *globalType = types::TypeObjectKey::get(&script()->global());
> +    if (globalType->hasFlags(constraints(), types::OBJECT_FLAG_SIZED_OBJECT_NEUTERED))
> +        return true;

Agreed, the "cannot" in this comment sounds very final, when in fact this is just a current limitation on what we choose to jit.

Comment 4

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4ec33eddc6fc

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4ec33eddc6fc