939715 - PJS: Move polyfill into self-hosted code

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Niko Matsakis [:nmatsakis]]

The current PJS API is described by a polyfill built on typed objects:

https://github.com/nikomatsakis/pjs-polyfill

This is purely sequential execution, naturally. In order to enable parallel execution, we need to move the polyfill into self-hosted code.

Comment 1

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: patch A v1: typedobjects polyfill (semi-mechanical port)

(just stashing current status.)

Comment 2

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: patch B v1: extended polyfill with unsized array support (for build)

just stashing current work status.

hypothetically this might also handle map for unsized arrays, but i have not yet tested that.  (I need to fix map anyway to not take an explicit type.)

Comment 3

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: patch C v1:unsized map support

Comment 4

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: bug939715-pAv2-typedobject-polyfill.patch

squashed the mechanical port in with the support for unsized arrays (on build and from class methods and the map instance method).  Fixed some bugs in the earlier v1 patches posted earlier; so p{A,B,C}_v1 + improvements = pA_v2.

renamed the polyfill implementations to not end with "Par" since they are not currently parallel implementations.  the names are hidden so we can rename them back if we like if we make them parallel.

I revised my own copy of the pjs-polyfill test code to work with these selfhosted versions (necessary since the map interface changed), but I have not yet attempted to port those tests into the spidermonkey infrastructure.  For now you can find them here:
  https://github.com/pnkfelix/pjs-polyfill/tree/bdbeaad22e604c1fae673ba354a3818b11df02bd

Comment 5

[Niko Matsakis [:nmatsakis]]

Comment on attachment 8345860 [details] [diff] [review]
bug939715-pAv2-typedobject-polyfill.patch

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

Can we also add the tests from the polyfill?

::: js/src/builtin/TypedObject.js
@@ +831,5 @@
> +
> +///////////////////////////////////////////////////////////////////////////
> +// TypedObject surface API methods (sequential implementations).
> +
> +function TypedObjectArrayTypeBuild(a,b,c) {

Can you add "// Warning: user-exposed!" as these functions may be directly called by users. It's just my own convention but I find it helpful to remember to be extra defensive.

@@ +833,5 @@
> +// TypedObject surface API methods (sequential implementations).
> +
> +function TypedObjectArrayTypeBuild(a,b,c) {
> +  var len;
> +  var kind = REPR_KIND(TYPE_TYPE_REPR(this));

Before using these macros, we must check that `this` is a type object. Something like:

if (!IsObject(this) || !ObjectIsTypeObject(this))
    ThrowError(...)

@@ +868,5 @@
> +}
> +
> +function TypedArrayMap(a, b) {
> +  // Arguments: [depth], func
> +  var intype = TypeOfTypedDatum(this);

Must validate IsObject && ObjectIsTypedDatum

@@ +896,5 @@
> +}
> +
> +// placeholders
> +function TypedObjectArrayTypeBuildPar(a,b) {
> +  return TypedObjectArrayTypeBuild.call(this, a, b);

Use `callFunction(TypedObjectArrayTypeBuild, this, a, b)`, which is hardwired into the jit.

@@ +899,5 @@
> +function TypedObjectArrayTypeBuildPar(a,b) {
> +  return TypedObjectArrayTypeBuild.call(this, a, b);
> +}
> +function TypedObjectArrayTypeFromPar(a,b,c) {
> +  return TypedObjectArrayTypeFrom.call(this, a, b, c);

Again here.

@@ +903,5 @@
> +  return TypedObjectArrayTypeFrom.call(this, a, b, c);
> +}
> +
> +function TypedArrayMapPar(a, b) {
> +  return TypedArrayMap.call(this, a, b);

Again here.

@@ +906,5 @@
> +function TypedArrayMapPar(a, b) {
> +  return TypedArrayMap.call(this, a, b);
> +}
> +function TypedArrayReducePar(a, b) {
> +  return TypedArrayReduce.call(this, a, b);

Again here.

@@ +909,5 @@
> +function TypedArrayReducePar(a, b) {
> +  return TypedArrayReduce.call(this, a, b);
> +}
> +function TypedArrayScatterPar(a, b, c, d) {
> +  return TypedArrayScatter.call(this, a, b, c, d);

Again here.

@@ +912,5 @@
> +function TypedArrayScatterPar(a, b, c, d) {
> +  return TypedArrayScatter.call(this, a, b, c, d);
> +}
> +function TypedArrayFilterPar(func) {
> +  return TypedArrayFilter.call(this, func);

Again here. Except that I wonder if it's better to just remove these placeholders and map the "fooPar" directly to "foo" in the JS_SELF_HOSTED_FN decls. Doesn't matter I guess.

@@ +915,5 @@
> +function TypedArrayFilterPar(func) {
> +  return TypedArrayFilter.call(this, func);
> +}
> +
> +// should eventually become macros

Maybe -- or maybe we should just change the capitalization, move them to Utilties.js, and set the "force inline" hint.

@@ +930,5 @@
> +  var mask = 1 << (index & 0x7);
> +  return (data[word] & mask) != 0;
> +}
> +
> +function isArrayType(t) {

Please name this something like "TypeObjectIsArrayType", which indicates that it is only safe to call on things you know are type objects. Add an assertion:

    assert(IsObject(t) && ObjectIsTypeObject(t), "TypeObjectIsArrayType called on non-type-object");

@@ +948,5 @@
> +    return ThrowError(JSMSG_TYPEDOBJECT_HANDLE_BAD_ARGS, 1, "unknown kind of typed object");
> +  }
> +}
> +
> +function isScalarType(t) {

As above. Also, I think the convention is to capitalize functions in self-hosted code -- or at least it was at one point.

@@ +966,5 @@
> +    return ThrowError(JSMSG_TYPEDOBJECT_HANDLE_BAD_ARGS, 1, "unknown kind of typed object");
> +  }
> +}
> +
> +function buildExplicit(arrayType, len, depth, func) {

Add assertions regarding the expected type of each argument.

assert(IsObject(arrayType) && ObjectIsTypeObject(arrayType))

and so on.

@@ +967,5 @@
> +  }
> +}
> +
> +function buildExplicit(arrayType, len, depth, func) {
> +  if (depth <= 0 || (depth|0) !== depth)

There is a macro "TO_INT32" that is preferred to "depth|0"

@@ +982,5 @@
> +
> +  // Create a zeroed instance with no data
> +  var result = arrayType.variable ? new arrayType(len) : new arrayType();
> +
> +  var indices = [];

Maybe we should convert this to "new Uint32Array(depth)" -- or at least NewDenseArray

@@ +986,5 @@
> +  var indices = [];
> +  for (var i = 0; i < depth; i++)
> +    indices.push(0);
> +
> +  // FIXME add redimension, rewrite using that

Remove this FIXME, it doesn't apply if we use handles that cut through the abstraction barriers (as I'm about to suggest below...)

@@ +987,5 @@
> +  for (var i = 0; i < depth; i++)
> +    indices.push(0);
> +
> +  // FIXME add redimension, rewrite using that
> +  // FIXME test for Handle.move

I have no idea what this FIXME meant but I think we can remove it.

@@ +992,5 @@
> +
> +  var handle = grainType.handle();
> +  for (var i = 0; i < totalLength; i++) {
> +    // Position handle to point at &result[...indices]
> +    HandleMoveInternal(handle, result, indices);

If we're going to cut away abstraction barriers, we can do better. Let's just use `AttachHandle(handle, result, offset)` where `offset` is an int32 that starts at 0 and is incremented by `REPR_SIZE(TYPE_TYPE_REPR(grainType))` after each element.

@@ +995,5 @@
> +    // Position handle to point at &result[...indices]
> +    HandleMoveInternal(handle, result, indices);
> +
> +    // Invoke func(...indices, out)
> +    var r = func.apply(null, indices.concat([handle]));

we should use std_Function_apply here

@@ +1024,5 @@
> +  }
> +  return [iterationSpace, grainType, totalLength];
> +}
> +
> +function increment(indices, iterationSpace) {

rename to IncrementIterationSpace so as not to claim such a generic name

@@ +1071,5 @@
> +  var indices = [];
> +  for (var i = 0; i < depth; i++)
> +    indices.push(0);
> +
> +  // FIXME add redimension, rewrite using that

as above.

@@ +1072,5 @@
> +  for (var i = 0; i < depth; i++)
> +    indices.push(0);
> +
> +  // FIXME add redimension, rewrite using that
> +  // FIXME test for Handle.move

as above.

@@ +1084,5 @@
> +
> +  for (var i = 0; i < totalLength; i++) {
> +    // Position handle to point at &result[...indices]
> +    HandleMoveInternal(inHandle, inArray, indices);
> +    HandleMoveInternal(outHandle, result, indices);

As above, prefer AttachHandle

@@ +1129,5 @@
> +
> +    return value;
> +  }
> +
> +  value = new outputType();

It's still unclear to me how we should handle `reduce`. I think we were vaguely moving towards treating all values similar to the `isScalarType` case above and never supply a handle to the callback, despite that being inefficient to the "add a bunch of vectors" case.

@@ +1152,5 @@
> +
> +  return value;
> +}
> +
> +function Scatter(array, outputType, indices, defaultValue, conflictFunc) {

assertions for input argument types

@@ +1160,5 @@
> +  var bitvec = new ByteArray();
> +  var elemType = outputType.elementType;
> +  var i, j;
> +  if (defaultValue !== elemType(undefined)) {
> +    for (i = 0; i < result.length; i++) { result[i] = defaultValue; }

indent

@@ +1172,5 @@
> +    } else if (conflictFunc === undefined) {
> +      ThrowError(JSMSG_PAR_ARRAY_SCATTER_CONFLICT);
> +    } else {
> +      // FIXME should we pass an outptr? handle into result[j] here?
> +      result[j] = conflictFunc(result[j], elemType(array[i]));

Remove the FIXME, I think we just won't offer a handle here. Or, at least, that's what the strawman says. The right thing is unclear to me but in any case we don't need a FIXME -- or else we could open a bug to resolve the desired semantics and add the bug # here.

@@ +1178,5 @@
> +  }
> +  return result;
> +}
> +
> +function Filter(array, func) {

I think we should give this a less generic name.

Comment 6

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 8345860 [details] [diff] [review]
bug939715-pAv2-typedobject-polyfill.patch

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

::: js/src/builtin/TypedObject.js
@@ +868,5 @@
> +}
> +
> +function TypedArrayMap(a, b) {
> +  // Arguments: [depth], func
> +  var intype = TypeOfTypedDatum(this);

TypeOfTypedDatum already has that guard; it is user-exposed and equivalent to |TypedObject.objectType|, which is what we used in the original polyfill here I think.

Or is your point that I should not be reusing TypeOfTypedDatum in the code I am adding, and instead I should be (1) checking those guards, (2) throwing if it fails the guard check, and (3) using DATUM_TYPE_OBJ if it passes the guard?

@@ +912,5 @@
> +function TypedArrayScatterPar(a, b, c, d) {
> +  return TypedArrayScatter.call(this, a, b, c, d);
> +}
> +function TypedArrayFilterPar(func) {
> +  return TypedArrayFilter.call(this, func);

We're hoping to put in the parallel variants soon enough that I figure why bounce back-and-forth on edits to the c++ code?  Especially since this way one can even substitute in new selfhosted.js on a Nightly build to try out parallel variants there.

@@ +1160,5 @@
> +  var bitvec = new ByteArray();
> +  var elemType = outputType.elementType;
> +  var i, j;
> +  if (defaultValue !== elemType(undefined)) {
> +    for (i = 0; i < result.length; i++) { result[i] = defaultValue; }

what?  Or do you mean "put the body on its own line" ?

@@ +1178,5 @@
> +  }
> +  return result;
> +}
> +
> +function Filter(array, func) {

Like FilterTypedSeqImpl ?  (And likewise {Build,Map,Reduce,From}TypedSeqImpl?)

Comment 7

[Felix S. Klock II [:pnkfelix, :fklock]]

(p.s. thanks for the careful review :)

Comment 8

[Niko Matsakis [:nmatsakis]]

(In reply to Felix S. Klock II [:pnkfelix, :fklock] from comment #6)
> TypeOfTypedDatum already has that guard; it is user-exposed and equivalent
> to |TypedObject.objectType|, which is what we used in the original polyfill
> here I think.

Ah, my mistake. I don't see a problem using the user-exposed version in particular though I prefer to avoid it if we have more information (like, we know -- or should know -- that the checks aren't needed). It might be good to adopt a naming convention that makes it clearer when something is user-exposed I guess. That would obviate the need those "// User-exposed!" comments. Not sure what that convention should be. Maybe `UserBuildMapPar` or whatever? Not entirely obvious.

> We're hoping to put in the parallel variants soon enough that I figure why
> bounce back-and-forth on edits to the c++ code?  Especially since this way
> one can even substitute in new selfhosted.js on a Nightly build to try out
> parallel variants there.

+1

> what?  Or do you mean "put the body on its own line" ?

Yes.

> Like FilterTypedSeqImpl ?  (And likewise
> {Build,Map,Reduce,From}TypedSeqImpl?)

Sounds reasonable.

Comment 9

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 8345860 [details] [diff] [review]
bug939715-pAv2-typedobject-polyfill.patch

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

::: js/src/builtin/TypedObject.js
@@ +982,5 @@
> +
> +  // Create a zeroed instance with no data
> +  var result = arrayType.variable ? new arrayType(len) : new arrayType();
> +
> +  var indices = [];

Well, unfortunately we need to be able to push the handle onto the end of whatever we feed into the |func.apply| call down below.  So if we turned |indices| into a |Uint32Array|, we would need to make a copy of it at the callsite.  But maybe |NewDenseArray| can work with the current setup.  Will check.

Comment 10

[Felix S. Klock II [:pnkfelix, :fklock]]

A puzzle: While following up on my previous comment and bumming certain parts of the code by replacing normal array constructions with uses of NewDenseArray (or, if possible, new Uint32Array), I found that the following attempt at an optimization broke:

    // Invoke func(element, ...indices, collection, out)
    var args;
    if (true) { // working
      args = [element];
      callFunction(std_Function_apply, std_Array_push, args, indices);
      callFunction(std_Array_push, args, inArray, outHandle);
    } else { // not-working
      args = NewDenseArray(2 + indices.length);
      args[0] = element;
      for (var i=0; i < indices.length; i++) {
        args[i+1] = indices[i];
      }
      args[indices.length + 1] = outHandle;
    }
    var r = callFunction(std_Function_apply, func, null, args);

If I replace the |if (true)| above with |if (false)| as to exercise the working branch, my test suite fails with scary errors like:

  939715: method type.from js(21609,0x7fff79a58180) malloc: *** error for object 0x103501a20: pointer being freed was not allocated
  *** set a breakpoint in malloc_error_break to debug
  Abort trap: 6

Or:

  939715: method instance.map
  Assertion failure: thing, at /Users/fklock/Dev/Mozilla/mozilla-inbound/js/src/gc/Marking.cpp:127
  REGRESSION - ecma_6/TypedObject/method_map.js

Scary, eh?  I feel like I must be overlooking some obvious mistake in the rewrite above, but I'm not sure what it is.  (Perhaps it is simply illegal to use a NewDenseArray for the args argument to std_Function_apply?)

Well, for the time being I will leave this particular arguments array as a normal Javascript array.

Comment 11

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: patch T v1: typedobject-polyfill-tests

the tests, ported from our polyfill repo over into the ecma6 area.  they won't work without some variant of patch A in place.

(And currently I think they invoke the parallel variants; for the ecma_6 directory, those calls should be replaced with invocations of the sequential methods; I'll fix that in a follow-up patch, but I wanted to get this posted now, since it actively exercises the patch that I want reviewed.)

Comment 12

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: patch A v3: typedobject-polyfill.patch

okay, I think I have addressed all of niko's comments.

Niko: Please review both this and patch T.  But review patch T under the assumption that I will rename the invocations to call the sequential versions, not the parallel ones.  (Of course you can feel free to note all the places where that mistake takes place :)

Cheers.

Comment 13

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 8347339 [details] [diff] [review]
patch T v1: typedobject-polyfill-tests

(as noted in previous comment, you can assume I'll fix the invocations to call the sequential methods.)

Comment 14

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: patch P v1: puzzle-from-comment10 (goes atop patch A v3)

Here's a patch for the puzzle noted in comment 10.  You apply it atop patch A v3 (attachment 8347341 [details] [diff] [review]).

Comment 15

[Felix S. Klock II [:pnkfelix, :fklock]]

shu solved the puzzle.  There was a repeated use of the same loop variable for the loop initializing the NewDenseArray in patch P.  :(  (facepalm)

shu also pointed out that the issues this exposed point out a need for range-checking assertions in e.g. TypeDatum::attach.

Comment 16

[Niko Matsakis [:nmatsakis]]

Comment on attachment 8347341 [details] [diff] [review]
patch A v3: typedobject-polyfill.patch

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

Making progress :)

::: js/src/builtin/TypedObject.js
@@ +33,5 @@
>  #define HAS_PROPERTY(obj, prop) \
>      callFunction(std_Object_hasOwnProperty, obj, prop)
>  
> +#define ASSERT_EQ(x, y) \
> +    assert(x === y, ""+x+" != "+y)

Maybe move to Utilities.js and rename to "assertEq" to be more inline with assert? I'm not sure why that particular macro is not capitalized.

@@ +839,5 @@
> +    ThrowError(JSMSG_TYPEDOBJECT_HANDLE_BAD_ARGS, "this", "type object");
> +  var kind = REPR_KIND(TYPE_TYPE_REPR(this));
> +  switch (kind) {
> +  case JS_TYPEREPR_SIZED_ARRAY_KIND:
> +    len = false;

Why assign false here? It doesn't appear to be used...

@@ +849,5 @@
> +      return ThrowError(JSMSG_TYPEDOBJECT_HANDLE_BAD_ARGS, "2", "function");
> +    else
> +      return ThrowError(JSMSG_TYPEDOBJECT_HANDLE_BAD_ARGS, "1", "function");
> +  case JS_TYPEREPR_UNSIZED_ARRAY_KIND:
> +    len = a;

...I'd prefer to just move var decl to here.

@@ +863,5 @@
> +    return ThrowError(JSMSG_TYPEDOBJECT_HANDLE_BAD_ARGS, "this", "type object");
> +  }
> +}
> +
> +function TypedObjectArrayTypeFrom(a, b, c) {

// Warning: user-exposed

@@ +881,5 @@
> +  else
> +    return ThrowError(JSMSG_TYPEDOBJECT_HANDLE_BAD_ARGS, "2", "function");
> +}
> +
> +function TypedArrayMap(a, b) {

// Warning: user-exposed

@@ +883,5 @@
> +}
> +
> +function TypedArrayMap(a, b) {
> +  // Arguments: [depth], func
> +  var intype = TypeOfTypedDatum(this);

Check that IsObject(this) && ObjectIsTypedDatum(this)

@@ +894,5 @@
> +  else
> +    return ThrowError(JSMSG_TYPEDOBJECT_HANDLE_BAD_ARGS, "2", "function");
> +}
> +
> +function TypedArrayReduce(a, b) {

// Warning: user-exposed

@@ +896,5 @@
> +}
> +
> +function TypedArrayReduce(a, b) {
> +  // Arguments: func, [initial]
> +  // FIXME typeof uint8 === "function", need a better way

Remove this FIXME, I think.

@@ +897,5 @@
> +
> +function TypedArrayReduce(a, b) {
> +  // Arguments: func, [initial]
> +  // FIXME typeof uint8 === "function", need a better way
> +  var outputType = TypeOfTypedDatum(this).elementType;

Need to check that this.IsObject() && ObjectIsTypedDatum(this)

@@ +901,5 @@
> +  var outputType = TypeOfTypedDatum(this).elementType;
> +  return ReduceTypedSeqImpl(this, outputType, a, b);
> +}
> +
> +function TypedArrayScatter(a, b, c, d) {

// Warning: user-exposed

@@ +903,5 @@
> +}
> +
> +function TypedArrayScatter(a, b, c, d) {
> +  // Arguments: outputArrayType, indices, defaultValue, conflictFunction
> +  return ScatterTypedSeqImpl(this, a, b, c, d);

Check types of this and outputArrayType, perhaps other arguments too

@@ +906,5 @@
> +  // Arguments: outputArrayType, indices, defaultValue, conflictFunction
> +  return ScatterTypedSeqImpl(this, a, b, c, d);
> +}
> +
> +function TypedArrayFilter(func) {

// Warning: user-exposed

@@ +907,5 @@
> +  return ScatterTypedSeqImpl(this, a, b, c, d);
> +}
> +
> +function TypedArrayFilter(func) {
> +  return FilterTypedSeqImpl(this, func);

Check type of this, perhaps other arguments too

@@ +972,5 @@
> +  case JS_TYPEREPR_X4_KIND:
> +  case JS_TYPEREPR_STRUCT_KIND:
> +    return false;
> +  default:
> +    return ThrowError(JSMSG_TYPEDOBJECT_HANDLE_BAD_ARGS, 1, "unknown kind of typed object");

Static analysis FTW!

@@ +976,5 @@
> +    return ThrowError(JSMSG_TYPEDOBJECT_HANDLE_BAD_ARGS, 1, "unknown kind of typed object");
> +  }
> +}
> +
> +function TypeObjectIsScalarType(t) {

I've been using the term "simple" type for these "non-compound" types.

@@ +1025,5 @@
> +    // Position handle to point at &result[...indices]
> +    AttachHandle(handle, result, offset);
> +
> +    // Invoke func(...indices, out)
> +    callFunction(std_Array_push, indices, handle);

I wonder if it's faster to pollute the type set for indices or to copy to a different array. Doesn't really matter, I guess, since I think we'll make high perf versions of these that are tailored to 1, 2, and 3 dimensions.

@@ +1030,5 @@
> +    var r = callFunction(std_Function_apply, func, null, indices);
> +    callFunction(std_Array_pop, indices);
> +
> +    if (r !== undefined)
> +      HandleSet(handle, r); // *handle = r

This is awkward because we can't assume that the callee didn't move outHandle. So we probably have to repeat the AttachHandle call. (repeated below)

@@ +1040,5 @@
> +
> +  return result;
> +}
> +
> +function computeIterationSpace(arrayType, depth, len) {

capitalize

@@ +1043,5 @@
> +
> +function computeIterationSpace(arrayType, depth, len) {
> +  assert(IsObject(arrayType) && ObjectIsTypeObject(arrayType), "computeIterationSpace called on non-type-object");
> +
> +  var iterationSpace = [];

Pointless Perf Nit: Use NewDenseArray(depth) rather than pushing, I think.

@@ +1051,5 @@
> +    if (TypeObjectIsArrayType(grainType)) {
> +      callFunction(std_Array_push, iterationSpace, len);
> +      totalLength *= len;
> +      grainType = grainType.elementType;
> +      len = grainType.length;

This left me scratching my head for a bit. I guess it's not harmful to read the length property from non-array grainTypes, but it's a bit confusing. I think you should unroll the loop, since in the first iteration we know that an array type was supplied in, and we also potentially need the length supplied from the caller. After that it should be regular.

@@ +1085,5 @@
> +  }
> +}
> +
> +function MapTypedSeqImpl(inArray, depth, outputType, func) {
> +  assert(IsObject(outputType) && ObjectIsTypeObject(outputType), "Map called on non-type-object outputType");

Add: assert(isObject(inArray) && ObjectIsTypedDatum(inArray)) since you assume that in the body

@@ +1094,5 @@
> +
> +  // Compute iteration space for input and output and compare
> +  var inputType = TypeOfTypedDatum(inArray);
> +  var [inIterationSpace, inGrainType, _] =
> +    computeIterationSpace(inputType, depth, inputType.variable ? inArray.length : inputType.length);

You can unconditionally use inArray.length here, even if inputType is sized

@@ +1096,5 @@
> +  var inputType = TypeOfTypedDatum(inArray);
> +  var [inIterationSpace, inGrainType, _] =
> +    computeIterationSpace(inputType, depth, inputType.variable ? inArray.length : inputType.length);
> +  var [iterationSpace, outGrainType, totalLength] =
> +    computeIterationSpace(outputType, depth, outputType.variable ? inArray.length : outputType.length);

...But I guess not here, since we haven't validated outputType.length

@@ +1111,5 @@
> +  var outHandle = callFunction(HandleCreate, outGrainType);
> +
> +  if (!IsObject(inGrainType) || !ObjectIsTypeObject(inGrainType))
> +    ThrowError(JSMSG_TYPEDOBJECT_HANDLE_BAD_ARGS, 1, "type object");
> +  var inGrainTypeIsScalar = TypeObjectIsScalarType(inGrainType);

s/scalar/simple

@@ +1131,5 @@
> +    callFunction(std_Array_push, args, inArray, outHandle);
> +    var r = callFunction(std_Function_apply, func, null, args);
> +
> +    if (r !== undefined)
> +      HandleSet(outHandle, r); // *handle = r

This is awkward because we can't assume that the callee didn't move outHandle. So we probably have to repeat the AttachHandle call.

@@ +1143,5 @@
> +  return result;
> +}
> +
> +function ReduceTypedSeqImpl(array, outputType, func, initial) {
> +  assert(IsObject(outputType) && ObjectIsTypeObject(outputType), "Reduce called on non-type-object outputType");

Add assertion about array

@@ +1147,5 @@
> +  assert(IsObject(outputType) && ObjectIsTypeObject(outputType), "Reduce called on non-type-object outputType");
> +
> +  var start, value;
> +
> +  if (initial === undefined && array.length < 2)

This doesn't seem to match JS behavior, as we said. JS only throws if array.length == 0.

@@ +1151,5 @@
> +  if (initial === undefined && array.length < 2)
> +    // RangeError("reduce requires array of length >= 2")
> +    ThrowError(JSMSG_TYPEDOBJECT_ARRAYTYPE_BAD_ARGS);
> +
> +  if (TypeObjectIsScalarType(outputType)) {

Based on the strawman and my memory, I think that this if is unnecessary, because we should take this approach even for compound types. Maybe move the FIXME up here.

@@ +1190,5 @@
> +  return value;
> +}
> +
> +function ScatterTypedSeqImpl(array, outputType, indices, defaultValue, conflictFunc) {
> +  assert(IsObject(outputType) && ObjectIsTypeObject(outputType), "Scatter called on non-type-object outputType");

Add assertion for array.

Validate type of other arguments where necessary.

@@ +1216,5 @@
> +  }
> +  return result;
> +}
> +
> +function FilterTypedSeqImpl(array, func) {

Add assertions for array.

Comment 17

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 8347341 [details] [diff] [review]
patch A v3: typedobject-polyfill.patch

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

::: js/src/builtin/TypedObject.js
@@ +33,5 @@
>  #define HAS_PROPERTY(obj, prop) \
>      callFunction(std_Object_hasOwnProperty, obj, prop)
>  
> +#define ASSERT_EQ(x, y) \
> +    assert(x === y, ""+x+" != "+y)

eh, its not used frequently enough to be worth defining.  (Especially since the version I wrote repeated the subexpressions x and y.)  I'll just rewrite the two calls in terms of assert.

@@ +839,5 @@
> +    ThrowError(JSMSG_TYPEDOBJECT_HANDLE_BAD_ARGS, "this", "type object");
> +  var kind = REPR_KIND(TYPE_TYPE_REPR(this));
> +  switch (kind) {
> +  case JS_TYPEREPR_SIZED_ARRAY_KIND:
> +    len = false;

good catch, that was left over from a prior iteration where I didn't pass |this.length| here (and instead passed a len of `false` and then extracted the |.length| from the passed |this|).

@@ +883,5 @@
> +}
> +
> +function TypedArrayMap(a, b) {
> +  // Arguments: [depth], func
> +  var intype = TypeOfTypedDatum(this);

Around the merry-go-round (see comment 6 and comment 8).

But okay, lets see how it looks to be more explicit about this.  (Though those checks alone won't immediately prevent non-array types from flowing in as the |intype| fed to |MapTypedSeqImpl|, e.g. if someone can attach this method to the prototype for a struct type.)

Comment 18

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 8347341 [details] [diff] [review]
patch A v3: typedobject-polyfill.patch

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

::: js/src/builtin/TypedObject.js
@@ +903,5 @@
> +}
> +
> +function TypedArrayScatter(a, b, c, d) {
> +  // Arguments: outputArrayType, indices, defaultValue, conflictFunction
> +  return ScatterTypedSeqImpl(this, a, b, c, d);

good catch.

@@ +1025,5 @@
> +    // Position handle to point at &result[...indices]
> +    AttachHandle(handle, result, offset);
> +
> +    // Invoke func(...indices, out)
> +    callFunction(std_Array_push, indices, handle);

I'm not going to worry about this level of performance tuning right now.

@@ +1030,5 @@
> +    var r = callFunction(std_Function_apply, func, null, indices);
> +    callFunction(std_Array_pop, indices);
> +
> +    if (r !== undefined)
> +      HandleSet(handle, r); // *handle = r

Ugh.  Okay.  (Should we consider making it an error to move outHandle in the callee?  I'm assuming we could detect that here.)

@@ +1085,5 @@
> +  }
> +}
> +
> +function MapTypedSeqImpl(inArray, depth, outputType, func) {
> +  assert(IsObject(outputType) && ObjectIsTypeObject(outputType), "Map called on non-type-object outputType");

Hmm.  That is probably a mistake since I am using this function to implement the |from| method, which only requires |inArray| be an array-like, I think.  Will attempt to fix.

Comment 19

[Niko Matsakis [:nmatsakis]]

Oh, sorry, I see your point about TypeOfTypedDatum and -- yes I forgot a second time. I personally try not to invoke "user-exposed" functions except in the outer level, and rather to have a lower-level variation that has no guards and just assertions. This way the assertions bubble up as "high as possible". I think this style is also preferred in spidermonkey, where it is considered bad style to invoke JS_Foo() and preferred to invoke js::foo().

Comment 20

[Niko Matsakis [:nmatsakis]]

(In reply to Felix S. Klock II [:pnkfelix, :fklock] from comment #18)
> Ugh.  Okay.  (Should we consider making it an error to move outHandle in the
> callee?  I'm assuming we could detect that here.)

I'm inclined more to see if we can amend the API somehow to make it impossible for users to move the handle. Dave and I were discussing this and he said that it's probably not going to work long term to have a "handle per thread" anyhow since it exposes the number of worker threads to the user. I'd say this is an area of "active uncertainty" right now.

Comment 21

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 8347341 [details] [diff] [review]
patch A v3: typedobject-polyfill.patch

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

::: js/src/builtin/TypedObject.js
@@ +1151,5 @@
> +  if (initial === undefined && array.length < 2)
> +    // RangeError("reduce requires array of length >= 2")
> +    ThrowError(JSMSG_TYPEDOBJECT_ARRAYTYPE_BAD_ARGS);
> +
> +  if (TypeObjectIsScalarType(outputType)) {

For the code as written, the |if (TypeObjectIsScalarType(outputType) ...| is necessary because in the then-block that it guards, it invokes |outputType| explicitly as a casting function, which currently throws an error on non-scalar types.

I do not know whether this is a sign of a bug in the Binary Data implementation (i.e. if all types, not just scalar ones, are meant to be invokable as coercion operators) or if it is a sign of a weakness in this implementation?  (But I think I do need to explicitly call the casting operator...)

Comment 22

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 8347341 [details] [diff] [review]
patch A v3: typedobject-polyfill.patch

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

::: js/src/builtin/TypedObject.js
@@ +1151,5 @@
> +  if (initial === undefined && array.length < 2)
> +    // RangeError("reduce requires array of length >= 2")
> +    ThrowError(JSMSG_TYPEDOBJECT_ARRAYTYPE_BAD_ARGS);
> +
> +  if (TypeObjectIsScalarType(outputType)) {

Also, if we do take the same approach for compound types, then code like this won't work as is (the imperative vectorAdd needs to return the accumulator |l| explicitly).

  var VectorType = uint32.array(3);
  var VectorsType = VectorType.array(3);
  var array = new VectorsType([[1, 2, 3],
                               [4, 5, 6],
                               [7, 8, 9]]);

  var sum = array.reduce(vectorAdd);

  function vectorAdd(l, r) {
    for (var i = 0; i < l.length; i++)
      l[i] += r[i];
  }

I'm not saying I'm opposed (I'm probably supportive of that shift); I just want to make sure we're on the same page about the implications of what we're doing.

Comment 23

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: patch A v4: typedobject-polyfill.patch

Okay, fourth time is the charm?

I am pretty sure this addresses all of your previous comments, but some of the changes I made caused somewhat large deviations from the previous patches (I'm thinking in particular of the revisions to the family of Map*SeqImpl....)

I also have some changes to the tests that were necessitated by this revision, will post that shortly.

Comment 24

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: patch T v3: typedobject-polyfill-tests

updates to tests necessitated by patch A v4.

Comment 25

[Niko Matsakis [:nmatsakis]]

Comment on attachment 8356433 [details] [diff] [review]
patch A v4: typedobject-polyfill.patch

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

OK, this looks good to me. :)

::: js/src/builtin/TypedObject.js
@@ +976,5 @@
> +  return (data[word] & mask) != 0;
> +}
> +
> +function TypeObjectIsArrayType(t) {
> +  // return (t instanceof ArrayType);

Remove these stray comments

@@ +995,5 @@
> +  }
> +}
> +
> +function TypeObjectIsSizedArrayType(t) {
> +  // return (t instanceof ArrayType);

Remove these stray comments

@@ +1014,5 @@
> +  }
> +}
> +
> +function TypeObjectIsSimpleType(t) {
> +  // return !(t instanceof ArrayType) && !(t instanceof StructType);

Remove these stray comments

Comment 26

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 8356434 [details] [diff] [review]
patch T v3: typedobject-polyfill-tests

Inherits r+ from Niko from attachment #8347339 [details] [diff] [review].

Comment 27

[Felix S. Klock II [:pnkfelix, :fklock]]

Okay, I had to make a few small modifications to the patches posted here to get things working.

Try build:
  https://tbpl.mozilla.org/?tree=Try&rev=c5cf7778697b

Pushed to inbound:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/4b29977351c4
  https://hg.mozilla.org/integration/mozilla-inbound/rev/7ede01d9d418

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4b29977351c4
https://hg.mozilla.org/mozilla-central/rev/7ede01d9d418