898342 - [binary data] add support for binary data handles

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Niko Matsakis [:nmatsakis]]

The current binary data support does not include the relocatable Handle types. A Handle is effectively a pointer; it is assigned an initial type and points into another binary data object. Unlike normal binary data objects, handles can (a) point at scalar values and (b) be relocated. See specification on ECMA wiki (forthcoming as of this moment, I think) for full details.

Comment 1

[Niko Matsakis [:nmatsakis]]

Attachment: Bug898342-Part1-Dehandle.diff

Rather rote refactoring to use JSObject& for infallible functions that cannot GC

Comment 2

[Niko Matsakis [:nmatsakis]]

Attachment: Bug898342-Part2-Refactor.diff

Settle on a new indentation style for rooted things

Comment 3

[Niko Matsakis [:nmatsakis]]

Attachment: Bug898342-Part3-Handle.diff

Refactor typed objects to include support for movable handles. Create `TypedDatum` as a common supertype of `TypedObject` and `Handle`. Move reification logic to self-hosted code.

Comment 4

[Niko Matsakis [:nmatsakis]]

Some brief details about handle API:

- Handles are created with `TypeObject.handle([obj, path...])`. If no obj/path is given, the handle is initially null. Handle must always point at memory of some type equivalent to `TypeObject`.
- Handles can be used like typed objects, meaning you can access properties (if this is a handle to a struct) or array elements (if a handle to an array).
- Handles can be moved with `Handle.move(handle, obj, path...)`.
- `Handle.get(handle)` and `Handle.set(handle, value)` permit you to read a handle (`*handle`) and set its value (`*handle = value`) respectively.

Comment 5

[Till Schneidereit [:till]]

Comment on attachment 819258 [details] [diff] [review]
Bug898342-Part1-Dehandle.diff

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

Yes

Comment 6

[Till Schneidereit [:till]]

Comment on attachment 819260 [details] [diff] [review]
Bug898342-Part2-Refactor.diff

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

sure

Comment 7

[Till Schneidereit [:till]]

Comment on attachment 819261 [details] [diff] [review]
Bug898342-Part3-Handle.diff

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

This looks great overall. Some comments:

I'm surprised by API for Handles: IIUC, TC39 now wants to move away from implicit construction for objects. I.e., you have to write `new Proxy(..)` instead of `Proxy(..)`. The exception to that are immutable value objects such as symbols and the proposed value objects like int64. ISTM that Handles aren't like those, and thus should be explicitly created using `new FooType.handle()`.


I'd like to see tests for a few more things:

- handles with non-index paths
- Handle.set and Handle.get
- Handle.isHandle
- valid handle moves in addition to the current ones for invalid moves
- Handle.move with a target that has the same canonical type, but not the same type object


Can you please add license headers to the test files? CC0/Public Domain is used in many cases, and is my personal preference, but you can also just use the standard license header, if you prefer.

r=me with the requested tests added and other comments addressed.

::: js/src/builtin/TypedObject.cpp
@@ +166,5 @@
>      return TypeRepresentation::fromOwnerObject(*typeRepresentationOwnerObj(typeObj));
>  }
>  
>  static inline JSObject *
>  GetType(JSObject &block)

s/block/datum/

@@ +179,5 @@
>   */
>  static bool
>  ConvertAndCopyTo(JSContext *cx,
>                   HandleObject typeObj,
>                   HandleObject block,

s/block/datum/

@@ +208,3 @@
>  
>  static bool
>  ConvertAndCopyTo(JSContext *cx, HandleObject block, HandleValue val)

s/block/datum/

@@ +214,5 @@
>  }
>  
> +/*
> + * Overwrites the contents of `block` at offset `offset` with `val`
> + * converted to the type `typeObj`

This comment doesn't seem to describe what the function is doing at all. I guess you maybe didn't copy it correctly from above? In fixing that, change the "block" mentions to "datum".

@@ +1445,5 @@
>  
>      // Tag the type object for this instance with the type
>      // representation, if that has not been done already.
> +    if (cx->typeInferenceEnabled() && !IsNumericTypeObject(*type)) {
> +        // FIXME                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~

Does this still need fixing? If so, either fix it, or create a bug and refer to it, here.

::: js/src/builtin/TypedObject.h
@@ +71,5 @@
> + * Typed datums may be attached or unattached. An unattached typed
> + * datum has no memory associated with it; it is basically a null
> + * pointer.  This can only happen when a new handle is created, since
> + * typed object instances are always associated with memory at the
> + * point of creation.

Can you explain that this could happen internally, but in practice only happens for handles? The comment for `createUnattached` is somewhat irritating without such an explanation.

@@ +89,5 @@
> + * pointer, and the owner slot for the derived object is set to the
> + * owner object, thus ensuring that the owner is not collected while
> + * the derived object is alive. We always maintain the invariant that
> + * JS_TYPEDOBJ_SLOT_OWNER is the true owner of the memory, meaning
> + * that there is a shall tree. This prevents an access pattern like

"shallow tree", I assume?

@@ +194,5 @@
>  };
>  
> +/*
> + * Base type for typed objects and handles. Basically any type whose
> + * contents consists of typed memory.

"content consists" or "contents consist"

@@ +281,5 @@
>      // Returns the offset in bytes within the object where the `void*`
>      // pointer can be found.
>      static size_t dataOffset();
>  
>      static bool isBlock(HandleObject val);

Is this still used? I can't find any uses. If not, remove. If yes, rename to `isDatum`.

@@ +297,5 @@
> +    // - type: type object for resulting object
> +    template<class T>
> +    static T *createUnattached(JSContext *cx, HandleObject type);
> +
> +    // creates a block that aliases the memory pointed at by `owner`

s/block/datum/

@@ +298,5 @@
> +    template<class T>
> +    static T *createUnattached(JSContext *cx, HandleObject type);
> +
> +    // creates a block that aliases the memory pointed at by `owner`
> +    // at the given offset

. at the end

@@ +417,1 @@
>  bool Memcpy(ThreadSafeContext *cx, unsigned argc, Value *vp);

We should have this for content script. I guess I'd have to talk to dherman about that, maybe?

::: js/src/builtin/TypedObject.js
@@ +174,5 @@
> +// Getting values
> +//
> +// The methods in this section read from the memory pointed at
> +// by `this` and produce JS values. This process is called *reification*
> +// spec.

"in the spec."

@@ +407,5 @@
> +///////////////////////////////////////////////////////////////////////////
> +// Handles
> +//
> +// Note: these methods are directly invokable by users and so must be
> +// defensive.

It would be nice for these methods to have comments allowing one to directly map their steps to the spec, like e.g. the Array methods do.

Can you create a bug for this and add a FIXME me here to do that once the spec has stabilized enough?

Comment 8

[Niko Matsakis [:nmatsakis]]

Till -- regarding `new Handle()` vs `T.handle()`, the precise API is still under discussion. In fact Dave/Dmitry initially proposed `new Handle(T)`. I counterproposed `T.handle()` because of the specific prototype semantics (`new Handle(T)` gets `T.prototype`, not `Handle.prototype`) which I thought might be mildly misleading. However, I've had second thoughts since then and before things are finalized I imagine this might revert back to `new Handle(T)`. That said, there is some chance that handles will be removed altogether.

Comment 9

[Niko Matsakis [:nmatsakis]]

Till -- I am not sure what value there would be to adding a Memcpy() call into the spec. Any type safe usage can be accomplished with an `=` sign -- and the JIT will (eventually) optimize that appropriately. If the goal is to copy an entire array, or subrange of an array, in one step, something like `Handle.set` might be useful:

    Handle.set(destArray, sourceArray)

should basically result in a memcpy. To this end, it might be nice to make "get" and "set" something that not only operate on handles but rather all typed objects.

Comment 10

[Till Schneidereit [:till]]

(In reply to Niko Matsakis [:nmatsakis] from comment #9)
> If the goal is to
> copy an entire array, or subrange of an array, in one step, something like
> `Handle.set` might be useful:
> 
>     Handle.set(destArray, sourceArray)
> 
> should basically result in a memcpy. To this end, it might be nice to make
> "get" and "set" something that not only operate on handles but rather all
> typed objects.

This is exactly the use case I had in mind. However, I realized after writing this that Memcpy actually does exist in the form of TypedArray#set(source, start, end).

Comment 11

[Niko Matsakis [:nmatsakis]]

Attachment: Bug898342.diff

Merge, incorporate Till's comments.

Comment 12

[Niko Matsakis [:nmatsakis]]

Try run: https://tbpl.mozilla.org/?tree=Try&rev=adebe0ff5250

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/35a83682c173

Comment 14

[Niko Matsakis [:nmatsakis]]

I would not document this or other aspects of typed objects yet-- the spec is in flux and in fact handles are going away.