898362 - [binary data] self-host where possible

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Niko Matsakis [:nmatsakis]]

I've given up on my original dream of implementing ALL the binary data semantics using self-hosting, but it should still be possible to self-host most or all of the various utility functions. This will require at least a new memcpy intrinsic that allows us to move data around, and possibly a few others. Self-hosting will of course make execution more efficient (no C++ to JS barrier, etc) but also permits those functions to be used in parallel execution, presuming we implement the intrinsics properly.

Comment 1

[Niko Matsakis [:nmatsakis]]

Attachment: Bug898362-Part1-ExposeConstants.diff

Convert from enums to #defines.
Also convert from JSObject* and HandleObject to JSObject& in many cases.
These two changes are both rote and somewhat entangled with respect to the lines of code they touch.

Comment 2

[Niko Matsakis [:nmatsakis]]

Attachment: Bug898362-Part2-SelfHost.diff

Move ConvertAndCopyTo and binary fill into self-hosted code. More to come.

Comment 3

[Niko Matsakis [:nmatsakis]]

Attachment: Bug898362-Part3-AdjustTests.diff

Adjust tests. There is a discrepancy between our current typed arrays, which convert `[]` to `NaN`, and the spec function `ToNumber()`, which seems to convert `[]` to `0` (or at least `+[]` is `0`). I have to consult with dherman, but I believe our typed arrays are not obeying the spec in this regard and should (eventually) be adjustd.

Comment 4

[Niko Matsakis [:nmatsakis]]

Adding dependency on bug 914220 because the patches here are rebased on top of those, not because of a logical dependency.

Comment 5

[Till Schneidereit [:till]]

Comment on attachment 802361 [details] [diff] [review]
Bug898362-Part1-ExposeConstants.diff

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

For my sanity's sake, I mostly just skimmed the changes in TypedObject.cpp. I gather the vast majority of them would just cause compiler errors if anything went wrong.

::: js/src/builtin/TypedObjectConstants.h
@@ +56,5 @@
> +#define JS_TYPEREPR_SLOTS          4
> +
> +// These constants are for use exclusively in JS code.  In C++ code,
> +// prefer TypeRepresentation::Scalar etc, since that allows you to
> +// write a switch which will receive a warning if you omit a case.

s/receive/cause/?

@@ +63,5 @@
> +#define JS_TYPEREPR_ARRAY_KIND  2
> +
> +// These constants are for use exclusively in JS code.  In C++ code,
> +// prefer ScalarTypeRepresentation::TYPE_INT8 etc, since that allows
> +// you to write a switch which will receive a warning if you omit a

and here

@@ +79,5 @@
> +///////////////////////////////////////////////////////////////////////////
> +// Slots for typed objects
> +
> +#define JS_TYPEDOBJ_SLOT_TYPE_OBJ 0  // Type object for a given typed object
> +#define JS_TYPEDOBJ_SLOT_OWNER    2  // Owner of data (if null, this is owner)

Why no slot 1?

Comment 6

[Till Schneidereit [:till]]

Comment on attachment 802363 [details] [diff] [review]
Bug898362-Part2-SelfHost.diff

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

This looks pretty nice!

::: js/src/builtin/TypedObject.cpp
@@ +197,5 @@
>      return atom->asPropertyName();
>  }
>  
> +static JSFunction *
> +SelfHostedFunction(JSContext *cx, const char *name)

This should really take a PropertyName, with most or all of the names added to the atom table. Re-atomizing the names every time they're used will probably have meaningful overhead.

Also, now that I think about it, would you mind moving this into SelfHosting and adding the declaration to jscntxt.h (or adding a SelfHosting.h, perhaps)? This would be useful in more places right now (I know the Intl stuff could use it) and will become more so over time.

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

Full stop at the end, please.

@@ +1750,5 @@
> +        cx,
> +        &type->getReservedSlot(JS_TYPEOBJ_SLOT_STRUCT_FIELD_TYPES).toObject());
> +    RootedValue fieldTypeVal(cx);
> +    if (!JSObject::getElement(cx, fieldTypes, fieldTypes,
> +                              fieldIndex, &fieldTypeVal))

Either don't wrap the if, or put braces around the then.

::: js/src/builtin/TypedObject.h
@@ +219,5 @@
> +//               size)
> +//
> +// Intrinsic function. Copies size bytes from the data for
> +// `sourceTypedObj` at `sourceOffset` into the data for
> +// `targetTypedObj` at `targetOffset`.

Nit: please keep the in-file style for function documentation. I.e. /*\n * comment\n */

@@ +228,5 @@
> +// Usage: StoreScalar(targetTypedObj, targetOffset, value)
> +//
> +// Intrinsic function. Stores value (which must be an int32 or uint32)
> +// by `scalarTypeRepr` (which must be a type repr obj) and stores the
> +// value at the memory for `targetTypedObj` at offset `targetOffset`.

Same here

::: js/src/builtin/TypedObject.js
@@ +48,5 @@
> +//
> +// Most `TypedObjectPointers` methods are written in a "chaining"
> +// style, meaning that they return `this`. This is true even though
> +// they mutate the receiver in place, because it makes for prettier
> +// code.

While I generally don't care too much about comment style, this one threw me off. It makes the comment look like it introduces a new section of code, not an individual function.

Maybe lose the first line of many "/"s and remove the empty line between the comment and the function?

@@ +57,5 @@
> +  this.owner = owner;
> +  this.offset = offset;
> +}
> +
> +MakeConstructible(TypedObjectPointer, {});

Not that it matters too much, but I'd probably initialize the prototype like so:

MakeConstructible(TypedObjectPointer,
{
  copy: function[..],
  reset: function[..],
  kind: function[..],
#ifdef DEBUG
  toString: function[..],
#endif
  [..]
});

@@ +86,5 @@
> +///////////////////////////////////////////////////////////////////////////
> +// moveTo(propName)
> +//
> +// Adjusts `this` in place so that it points at the property
> +// `propName`.  Throws if there is no such property. Returns `this`.

See my comment on comment style above.

@@ +160,5 @@
> +// set(fromValue)
> +//
> +// Assigns `fromValue` to the memory pointed at by `this`, adapting it
> +// to `typeRepr` as needed. This is the most general entry point and
> +// works for any type.

See my comment on comment style above.

@@ +215,5 @@
> +  }
> +
> +  ThrowError(JSMSG_CANT_CONVERT_TO,
> +             typeof(fromValue),
> +             this.typeRepr.toSource())

No need to wrap this, plus missing semicolon.

@@ +263,5 @@
> +function ConvertAndCopyTo(destTypeRepr,
> +                          destTypeObj,
> +                          destTypedObj,
> +                          destOffset,
> +                          fromValue) {

Brace on new line, please.

Comment 7

[Niko Matsakis [:nmatsakis]]

Attachment: Bug898362.diff

Carrying over r+ from till

Comment 8

[Niko Matsakis [:nmatsakis]]

Clean try run: https://tbpl.mozilla.org/?tree=Try&rev=134073866259

Comment 9

[Niko Matsakis [:nmatsakis]]

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

Comment 10

[Phil Ringnalda (:philor)]

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