Shu-yu Guo [:shu] Assignee
	Description • 12 years ago

Stephan did the preliminary work in bug 711304.

It is rebased on top of m-i instead of ion, as we need to get this enabled in Nightly.

This patch updates several things:
 - Implements multidimensional array
 - Reorganization to be more "modern"
 - Implements the updated API found at http://wiki.ecmascript.org/doku.php?id=strawman:data_parallelism

Implementation details:
 - The shape (dimensions) array and the underlying buffer are kept as dense arrays for now for ease of GC.
 - Multidimensional arrays are backed as flat dense arrays.
 - Indexing a multidimensional array with less than |shape.length| indices will return you a freshly minted ParallelArray.

	Shu-yu Guo [:shu] Assignee
	Comment 2 • 12 years ago

Comment on attachment 646984 [details] [diff] [review]
Milestone 1

Hold off on the review. Recent discussion with Intel shows some more API changes, possibly deep-ish, patch will be updated eventually.

	Shu-yu Guo [:shu] Assignee
	Comment 3 • 12 years ago

API is still in flux, but per contract we need to get some API into Nightly by Aug. 31st. We (nmatsakis, dherman and I) have decided to go with the current API per http://wiki.ecmascript.org/doku.php?id=strawman:data_parallelism and revise later as needed.

	Shu-yu Guo [:shu] Assignee
	Comment 4 • 12 years ago

Comment on attachment 650460 [details] [diff] [review]
Milestone 1

jorendorff is way too busy to meet the deadline

	Luke Wagner [:luke]
	Comment 5 • 12 years ago

Comment on attachment 650460 [details] [diff] [review]
Milestone 1

I will also be a poor reviewer as I will be taking paternity leave soon and I already have other sizable reviews enqueued.

	Shu-yu Guo [:shu] Assignee
	Comment 6 • 12 years ago

Update to bring more in line with development version, but with the parallel code ripped out:

- Add a debug options object argument
- Return ExecutionStatus instead of bool to distinguish bail/failed compilation, though the distinction is not used in the sequential version

	David Mandelin [:dmandelin]
	Comment 7 • 12 years ago

Comment on attachment 650959 [details] [diff] [review]
Milestone 1

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

Generally clean code, mostly pretty easy to read. General comments:

- I'd like more comments that lay out key concepts, function behaviors, and parameter meanings (and fewer that just say what the next line of code is doing). I specifically commented on all the important ones I noticed.

- One challenge with this work is that you don't have a spec yet, so you can't just refer to a spec by section number for key behaviors. But you should try to find a way to mention what things are documented where, or at least briefly explain what the draft spec is for things that aren't obvious.

- Do you have performance constraints? It looks like everything is implemented in the easy-to-understand, not very optimized, but avoiding terrible blowups style. Is that going to be good enough? Do you have to compete with Array.map and the like?

- What about testing? It looks like you've given a basic test for each feature, but the included tests are not nearly enough to give any confidence that it's really solid. Is that something for later? I know this is milestone 1 of a future-oriented projects, so I certainly won't insist on an extensive test suite, but I'd like to know your plans.

Most of the detailed comments are just instances of the above, but I also picked a few nits and asked a few questions around things that looked like possible bugs to me.

::: js/src/builtin/ParallelArray.cpp
@@ +58,5 @@
> +}
> +
> +template <typename T, bool ConvertToT(JSContext *, const Value &, T *)>
> +static bool
> +ArrayLikeToVector(JSContext *cx, HandleObject obj, Vector<T> &vec)

It's not obvious what this is supposed to do in the non-simple cases, so please add a comment to document that. More questions about those non-simple cases below.

@@ +69,5 @@
> +            if (!vec.resize(length))
> +                return false;
> +
> +            for (uint32_t i = 0; i < length; i++) {
> +                RootedValue elem(cx);

We generally try to avoid putting Rooteds inside a loop like this, because they will link and unlink on each iteration once we are doing exact GC. Putting it just before the loop should be fine. (Although you also need to be careful about doing it in a function that's called from loops.) I think I saw at least one other example elsewhere.

@@ +76,5 @@
> +            }
> +
> +            return true;
> +        }
> +

This part surprised me. For an array a of dimensions (n1, n2, ... nk), this seems to be returning the array [ a[n1][0]...[0], ..., a[nk][0]...[0] ]. Does that come from the draft spec or something? Assuming it's right, it at least needs an explanation.

@@ +97,5 @@
> +        return false;
> +
> +    for (uint32_t i = 0; i < length; i++) {
> +        if (obj->isDenseArray() && (i < obj->getDenseArrayInitializedLength())) {
> +            if (!ConvertToT(cx, obj->getDenseArrayElement(i), &vec[i]))

What does this do for holes?

@@ +101,5 @@
> +            if (!ConvertToT(cx, obj->getDenseArrayElement(i), &vec[i]))
> +                return false;
> +        } else {
> +            RootedValue elem(cx);
> +            if (!obj->getElement(cx, i, &elem) || !ConvertToT(cx, elem, &vec[i]))

This seems to return false if there is a missing element. Is that from the description of an array-like object in the spec?

@@ +187,5 @@
> +
> +template <>
> +ParallelArrayObject::ExecutionStatus
> +ParallelArrayObject::SequentialImpl::build(JSContext *cx, IndexInfo &iv,
> +                                           HandleObject elementalFun, HandleObject buffer)

Hmmm. I tend to think this would be better with an iterative implementation, partly for performance and partly because I'm not convinced this is simpler, given the need to imperatively update the iv argument.

If you want it to be recursive, it should at least use JS_CHECK_RECURSION and document the meaning of the iv parameter to help the reader.

@@ +247,5 @@
> +    JS_ASSERT(source->outermostDimension() == buffer->getDenseArrayInitializedLength());
> +    JS_ASSERT(buffer->isDenseArray());
> +
> +    uint32_t length = source->outermostDimension();
> +    bool flat = source->isOneDimensional();

This variable seems unnecessary.

@@ +303,5 @@
> +
> +    uint32_t length = source->outermostDimension();
> +    bool flat = source->isOneDimensional();
> +
> +    // The accumulator: the objet petit a.

The 'little object a'?

@@ +376,5 @@
> +
> +    // Convert the scatter vector to a Vector for easier access.
> +    IndexVector targets(cx);
> +    if (!ArrayLikeToIndexVector(cx, targetsObj, targets))
> +        return ExecutionFailed;

As with 'filter' (I typed that comment first), copying the input array seems inefficient.

@@ +457,5 @@
> +
> +    // Convert the filter vector to a Vector of bools.
> +    Vector<bool> filters(cx);
> +    if (!ArrayLikeToTruthyVector(cx, filtersObj, filters))
> +        return ExecutionFailed;

I guess I would need to test to be sure, but it seems kind of inefficient to copy the truth value array just to use it for a filter.

@@ +460,5 @@
> +    if (!ArrayLikeToTruthyVector(cx, filtersObj, filters))
> +        return ExecutionFailed;
> +
> +    uint32_t pos = 0;
> +    for (uint32_t i = 0; i < filters.length(); i++) {

Can put i++, pos++ here.

@@ +561,5 @@
> +} // namespace js
> +
> +#ifdef DEBUG
> +
> +// XXX: I don't like this, but how do I do it without a macro?

Construction debris. :-)

@@ +865,5 @@
> +ParallelArrayObject::getParallelArrayElement(JSContext *cx, uint32_t index, MutableHandleValue vp)
> +{
> +    JS_ASSERT(isOneDimensional());
> +
> +    uint32_t length = outermostDimension();

For this usage, I think it's nicer to define a method 'getOnlyDimension' (strawman name) that returns the dimension of a one-dimensional array, and asserts otherwise.

@@ +882,5 @@
> +ParallelArrayObject::getParallelArrayElement(JSContext *cx, IndexInfo &iv, MutableHandleValue vp)
> +{
> +    JS_ASSERT(iv.isInitialized());
> +
> +    // The dimension we're indexing.

I find that comment cryptic.

@@ +898,5 @@
> +            vp.setUndefined();
> +        else
> +            vp.set(buffer()->getDenseArrayElement(index));
> +        return true;
> +    }

This is somewhat of a duplicate of the 1-D form of getParallelArrayElement, but that doesn't seem like too big a deal.

@@ +900,5 @@
> +            vp.set(buffer()->getDenseArrayElement(index));
> +        return true;
> +    }
> +
> +    // If we aren't indexing a value, we're getting a row.

This isn't said in the strawman on the ES Wiki. I gather it's from some other draft spec doc? Our new ES code tends to refer to spec sections and steps, which is really nice. You don't have that yet, but it would be good to have some consistent way of saying roughly what comes from where when the behavior is non-obvious.

@@ +1029,5 @@
> +    if (!elementalFun)
> +        return false;
> +
> +    // How long the flattened array will be.
> +    uint32_t length = iv.dimensions[0] * iv.partialProducts[0];

This could be a method on IndexInfo.

@@ +1043,5 @@
> +        DebugOptions options;
> +        if (ToDebugOptions(cx, args[1], options)) {
> +            if (!CheckedImpl::build(cx, iv, elementalFun, buffer, options))
> +                return false;
> +            goto out;

That's kind of unfortunate. I'm sure you can find a way to make this do what you want without goto.

@@ +1066,5 @@
> +        return false;
> +    }
> +
> +    // Get the parallel array.
> +    RootedParallelArrayObject obj(cx, as(&args.thisv().toObject()));

Don't you need to check that it is a parallel array? Some of our code uses 'maybeX' to do that in one step (and also thus avoid potentially unsafe non-checked conversion ops).

@@ +1129,5 @@
> +        IndexInfo iv(cx);
> +        if (!iv.initialize(cx, obj, 1))
> +            return false;
> +        iv.indices[0] = 0;
> +        return obj->getParallelArrayElement(cx, iv, args.rval());

It seems like it would be nice to abstract the above into a single call, if it's at all convenient.

@@ +1169,5 @@
> +        JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_PAR_ARRAY_REDUCE_EMPTY);
> +        return false;
> +    }
> +
> +    // Else do the fold. Create a destination buffer.

Possible comment typo. Although the comment itself is mostly redundant.

@@ +1228,5 @@
> +    uint32_t targetsLength;
> +    if (!js_GetLengthProperty(cx, targets, &targetsLength))
> +        return false;
> +
> +    // Throw if we have different number of targets than indices.

This comment is confusing. It can probably just be deleted.

But why is this an error?

@@ +1613,5 @@
> +bool
> +ParallelArrayObject::toSource(JSContext *cx, CallArgs args)
> +{
> +    // TODO: NYI
> +    return toString(cx, args);

Debris.

::: js/src/builtin/ParallelArray.h
@@ +17,5 @@
> +class ParallelArrayObject;
> +typedef Rooted<ParallelArrayObject *> RootedParallelArrayObject;
> +typedef Handle<ParallelArrayObject *> HandleParallelArrayObject;
> +
> +class ParallelArrayObject : public JSObject {

Please add a header comment with important general information about parallel arrays. One thing I would like to see clearly laid out is how the multidimensional parallel array gets mapped to a one-dimensional representation.

@@ +21,5 @@
> +class ParallelArrayObject : public JSObject {
> +  public:
> +    typedef Vector<uint32_t> IndexVector;
> +
> +    struct IndexInfo {

Please add a comment explaining what this is (seems to be a tuple of indexes for an n-dimensional array of fixed dimensions) and how it's generally intended to be used.

I note that the implementation feels a bit 'heavy' for a relatively simple object, using 3 separate vectors. I'm wondering if you at least want to make sure there's a bit of inline storage in the vectors, enough to cover typical use cases.

@@ +24,5 @@
> +
> +    struct IndexInfo {
> +        IndexVector indices;
> +        IndexVector dimensions;
> +        IndexVector partialProducts;

It would be good to have a 1-line comment on each saying what it's for, especially partialProducts.

@@ +35,5 @@
> +        inline bool initialize(uint32_t space);
> +
> +        // Load dimensions from a source, then initialize.
> +        inline bool initialize(JSContext *cx, HandleParallelArrayObject source,
> +                               uint32_t space);

The comments on the initialize methods need to be clearer about when these are to be called and what the parameters mean. E.g., what is 'space'?

@@ +48,5 @@
> +    static Class class_;
> +
> +    static inline bool is(const Value &v);
> +    static inline bool is(JSObject *obj);
> +    static inline ParallelArrayObject *as(JSObject *obj);

Interesting choice of names, but I kind of like the brevity.

@@ +51,5 @@
> +    static inline bool is(JSObject *obj);
> +    static inline ParallelArrayObject *as(JSObject *obj);
> +
> +    inline JSObject *dimensionArray();
> +    inline JSObject *buffer();

I notice that in comment 0 you said "The shape (dimensions) array and the underlying buffer are kept as dense arrays _for now_ for ease of GC" (emphasis mine). If that helps you get the job done now, that's OK, but I would urge caution: a dense array is a far more complicated thing than what you need, and if code ends up depending on that extra complexity, it will be a lot of work to reverse that. I strongly recommend hiding that fact from the public interface. You might also want to talk to the GC people and see if they can give you an easy way to use vectors and stay GC-safe.

@@ +53,5 @@
> +
> +    inline JSObject *dimensionArray();
> +    inline JSObject *buffer();
> +    inline uint32_t bufferOffset();
> +    inline uint32_t outermostDimension();

'outermostDimension' is an example of something that's confusing if you don't document the way dimensions work. From reading other code, I gather it's based on the idea of viewing the linearized representation as containing nested lower-dimensional subarrays. It still unclear as yet as to why it's a particularly significant concept.

@@ +70,5 @@
> +  private:
> +    enum {
> +        // The ParallelArray API refers to dimensions as "shape", but to avoid
> +        // confusion with the internal engine notion of a shape we call it
> +        // "dimensions" here.

Good idea.

@@ +78,5 @@
> +        SLOT_BUFFER,
> +
> +        // First index of the underlying buffer to be considered in
> +        // bounds.
> +        SLOT_BUFFER_OFFSET,

Good comments here. Nit: join the above comment to one line.

@@ +86,5 @@
> +
> +    // Execution modes for the implementations of various options.
> +
> +    enum ExecutionMode {
> +        FallbackMode = 0,

This needs a better name or an explanation. At first, I thought it was the mode to fall back on if nothing else was available (which of course confused me because I also thought SequentialMode would play that role). I'm also not sure it's necessary. Will there someday be more modes, and will it become more important? Or is it just to make the entry point methods cleaner?

@@ +100,5 @@
> +
> +    template <ExecutionMode em>
> +    struct Impl {
> +        static ExecutionStatus build(JSContext *cx, IndexInfo &iv, HandleObject elementalFun,
> +                                     HandleObject buffer);

'build' needs documentation because it's not explicitly named in the spec.

@@ +105,5 @@
> +        static ExecutionStatus map(JSContext *cx, HandleParallelArrayObject source,
> +                                   HandleObject elementalFun, HandleObject buffer);
> +        static ExecutionStatus fold(JSContext *cx, HandleParallelArrayObject source,
> +                                    HandleObject elementalFun, HandleObject buffer,
> +                                    MutableHandleValue vp);

'fold' needs documentation because it's not really the same thing as the spec method 'fold'. In fact, it should probably be called something else.

@@ +133,5 @@
> +    static const char *ExecutionModeToString(ExecutionMode em);
> +    static const char *ExecutionStatusToString(ExecutionStatus ss);
> +    static bool ToDebugOptions(JSContext *cx, const Value &v, DebugOptions &options);
> +
> +    struct CheckedImpl {

Please document what CheckedImpl does.

	Shu-yu Guo [:shu] Assignee
	Comment 8 • 12 years ago

Wow, thanks for the fast review! Fixes incoming.

	Shu-yu Guo [:shu] Assignee
	Comment 9 • 12 years ago

Applied the review's suggestions. The biggest change is getting rid of the simulate dynamic dispatch CALL_IMPL and use actual inheritance to manage execution modes.

Specific points are inlined below:

> Generally clean code, mostly pretty easy to read. General comments:
>
> - I'd like more comments that lay out key concepts, function behaviors, and
> parameter meanings (and fewer that just say what the next line of code is
> doing). I specifically commented on all the important ones I noticed.
>
> - One challenge with this work is that you don't have a spec yet, so you
> can't just refer to a spec by section number for key behaviors. But you
> should try to find a way to mention what things are documented where, or at
> least briefly explain what the draft spec is for things that aren't obvious.
>

I've tried to document things better.

> - Do you have performance constraints? It looks like everything is
> implemented in the easy-to-understand, not very optimized, but avoiding
> terrible blowups style. Is that going to be good enough? Do you have to
> compete with Array.map and the like?
>

For the sequential implementation, which this is, I don't think we have any
performance constraints. We don't want to be stupid, but we're striving for
clarity here. The performance gains from using a ParallelArray is supposed to
come from the scalability and the parallelization, not the sequential fallback
execution.

> - What about testing? It looks like you've given a basic test for each
> feature, but the included tests are not nearly enough to give any confidence
> that it's really solid. Is that something for later? I know this is
> milestone 1 of a future-oriented projects, so I certainly won't insist on an
> extensive test suite, but I'd like to know your plans.
>

Yeah, we do need a comprehensive test suite, for some definition of
comprehensive. I will add more tests.

> Most of the detailed comments are just instances of the above, but I also
> picked a few nits and asked a few questions around things that looked like
> possible bugs to me.
>
> ::: js/src/builtin/ParallelArray.cpp
> @@ +58,5 @@
> > +}
> > +
> > +template <typename T, bool ConvertToT(JSContext *, const Value &, T *)>
> > +static bool
> > +ArrayLikeToVector(JSContext *cx, HandleObject obj, Vector<T> &vec)
>
> It's not obvious what this is supposed to do in the non-simple cases, so
> please add a comment to document that. More questions about those non-simple
> cases below.
>
> @@ +69,5 @@
> > +            if (!vec.resize(length))
> > +                return false;
> > +
> > +            for (uint32_t i = 0; i < length; i++) {
> > +                RootedValue elem(cx);
>
> We generally try to avoid putting Rooteds inside a loop like this, because
> they will link and unlink on each iteration once we are doing exact GC.
> Putting it just before the loop should be fine. (Although you also need to
> be careful about doing it in a function that's called from loops.) I think I
> saw at least one other example elsewhere.
>

Fixed.

> @@ +76,5 @@
> > +            }
> > +
> > +            return true;
> > +        }
> > +
>
> This part surprised me. For an array a of dimensions (n1, n2, ... nk), this
> seems to be returning the array [ a[n1][0]...[0], ..., a[nk][0]...[0] ].
> Does that come from the draft spec or something? Assuming it's right, it at
> least needs an explanation.
>

This is protected by |if (source->isOneDimensional())|. We do not need to
initialized an |IndexInfo| if the array is not multidimensional, thus the
different code path.

> @@ +97,5 @@
> > +        return false;
> > +
> > +    for (uint32_t i = 0; i < length; i++) {
> > +        if (obj->isDenseArray() && (i < obj->getDenseArrayInitializedLength())) {
> > +            if (!ConvertToT(cx, obj->getDenseArrayElement(i), &vec[i]))
>
> What does this do for holes?
>

Fixed. Default to |UndefinedValue()| when reading a hole.

> @@ +101,5 @@
> > +            if (!ConvertToT(cx, obj->getDenseArrayElement(i), &vec[i]))
> > +                return false;
> > +        } else {
> > +            RootedValue elem(cx);
> > +            if (!obj->getElement(cx, i, &elem) || !ConvertToT(cx, elem, &vec[i]))
>
> This seems to return false if there is a missing element. Is that from the
> description of an array-like object in the spec?
>

There is no description of array-like in the spec, they're simply described as
"array-like object". Does |JSObject::getElement| return false if [i] is
missing? I thought that just set |elem| to undefined and return true?

> @@ +187,5 @@
> > +
> > +template <>
> > +ParallelArrayObject::ExecutionStatus
> > +ParallelArrayObject::SequentialImpl::build(JSContext *cx, IndexInfo &iv,
> > +                                           HandleObject elementalFun, HandleObject buffer)
>
> Hmmm. I tend to think this would be better with an iterative implementation,
> partly for performance and partly because I'm not convinced this is simpler,
> given the need to imperatively update the iv argument.
>
> If you want it to be recursive, it should at least use JS_CHECK_RECURSION
> and document the meaning of the iv parameter to help the reader.
>

Good suggestion. Changed to iterative, now much easier to understand.

> @@ +247,5 @@
> > +    JS_ASSERT(source->outermostDimension() == buffer->getDenseArrayInitializedLength());
> > +    JS_ASSERT(buffer->isDenseArray());
> > +
> > +    uint32_t length = source->outermostDimension();
> > +    bool flat = source->isOneDimensional();
>
> This variable seems unnecessary.
>

Fixed.

> @@ +303,5 @@
> > +
> > +    uint32_t length = source->outermostDimension();
> > +    bool flat = source->isOneDimensional();
> > +
> > +    // The accumulator: the objet petit a.
>
> The 'little object a'?
>

Hah, sorry, this is a joke I couldn't resist:
https://de.twitter.com/PLT_Zizek/status/166599698893897730

The 'objet petit a(utre)' is a term from Lacanian psychoanalytic theory,
meaning 'the little other', where 'other' is used in a technical sense to mean
the thing that is perceived and opposed to the subject, i.e., the object.

> @@ +376,5 @@
> > +
> > +    // Convert the scatter vector to a Vector for easier access.
> > +    IndexVector targets(cx);
> > +    if (!ArrayLikeToIndexVector(cx, targetsObj, targets))
> > +        return ExecutionFailed;
>
> As with 'filter' (I typed that comment first), copying the input array seems
> inefficient.
>

Fixed. I've refactored the code to not do this anymore.

> @@ +457,5 @@
> > +
> > +    // Convert the filter vector to a Vector of bools.
> > +    Vector<bool> filters(cx);
> > +    if (!ArrayLikeToTruthyVector(cx, filtersObj, filters))
> > +        return ExecutionFailed;
>
> I guess I would need to test to be sure, but it seems kind of inefficient to
> copy the truth value array just to use it for a filter.
>

Fixed. Same as above.

> @@ +460,5 @@
> > +    if (!ArrayLikeToTruthyVector(cx, filtersObj, filters))
> > +        return ExecutionFailed;
> > +
> > +    uint32_t pos = 0;
> > +    for (uint32_t i = 0; i < filters.length(); i++) {
>
> Can put i++, pos++ here.
>

Can't, because pos isn't incremented every iteration, only when the filter
array element is truthy.

> @@ +561,5 @@
> > +} // namespace js
> > +
> > +#ifdef DEBUG
> > +
> > +// XXX: I don't like this, but how do I do it without a macro?
>
> Construction debris. :-)
>

What does debris mean in this case? Execution modes got all reworked in any
case and this is now gone.

> @@ +865,5 @@
> > +ParallelArrayObject::getParallelArrayElement(JSContext *cx, uint32_t index, MutableHandleValue vp)
> > +{
> > +    JS_ASSERT(isOneDimensional());
> > +
> > +    uint32_t length = outermostDimension();
>
> For this usage, I think it's nicer to define a method 'getOnlyDimension'
> (strawman name) that returns the dimension of a one-dimensional array, and
> asserts otherwise.
>

Logically it's the |getParallelArrayElement| method that should only work on
1D arrays. I don't see an independent use case for getting the outermost
dimension and asserting 1D-ness at the same time.

> @@ +882,5 @@
> > +ParallelArrayObject::getParallelArrayElement(JSContext *cx, IndexInfo &iv, MutableHandleValue vp)
> > +{
> > +    JS_ASSERT(iv.isInitialized());
> > +
> > +    // The dimension we're indexing.
>
> I find that comment cryptic.
>

Fixed.

> @@ +898,5 @@
> > +            vp.setUndefined();
> > +        else
> > +            vp.set(buffer()->getDenseArrayElement(index));
> > +        return true;
> > +    }
>
> This is somewhat of a duplicate of the 1-D form of getParallelArrayElement,
> but that doesn't seem like too big a deal.
>

Here I wanted to avoid making a |RootedValue|.

> @@ +900,5 @@
> > +            vp.set(buffer()->getDenseArrayElement(index));
> > +        return true;
> > +    }
> > +
> > +    // If we aren't indexing a value, we're getting a row.
>
> This isn't said in the strawman on the ES Wiki. I gather it's from some
> other draft spec doc? Our new ES code tends to refer to spec sections and
> steps, which is really nice. You don't have that yet, but it would be good
> to have some consistent way of saying roughly what comes from where when the
> behavior is non-obvious.
>

The state of the spec is not fully spelled out in many cases still,
unfortunately. I will, however, provide more detailed comments.

> @@ +1029,5 @@
> > +    if (!elementalFun)
> > +        return false;
> > +
> > +    // How long the flattened array will be.
> > +    uint32_t length = iv.dimensions[0] * iv.partialProducts[0];
>
> This could be a method on IndexInfo.
>

Added |IndexInfo::scalarLengthOfDimensions|.

> @@ +1043,5 @@
> > +        DebugOptions options;
> > +        if (ToDebugOptions(cx, args[1], options)) {
> > +            if (!CheckedImpl::build(cx, iv, elementalFun, buffer, options))
> > +                return false;
> > +            goto out;
>
> That's kind of unfortunate. I'm sure you can find a way to make this do what
> you want without goto.
>

I will duplicate the code (it's pretty short) instead of a goto. I still like
uses of goto to do some simple non-local control, though.

> @@ +1066,5 @@
> > +        return false;
> > +    }
> > +
> > +    // Get the parallel array.
> > +    RootedParallelArrayObject obj(cx, as(&args.thisv().toObject()));
>
> Don't you need to check that it is a parallel array? Some of our code uses
> 'maybeX' to do that in one step (and also thus avoid potentially unsafe
> non-checked conversion ops).
>

No, because these are all wrapped with |CallNonGenericMethod|, so the receiver
should already be a parallel array.

> @@ +1129,5 @@
> > +        IndexInfo iv(cx);
> > +        if (!iv.initialize(cx, obj, 1))
> > +            return false;
> > +        iv.indices[0] = 0;
> > +        return obj->getParallelArrayElement(cx, iv, args.rval());
>
> It seems like it would be nice to abstract the above into a single call, if
> it's at all convenient.
>

Fixed. |getParallelArrayElement| with an integer index now creates a temporary
|IndexInfo| and is a convenience wrapper. The old |getParallelArrayElement|
that asserts on one dimensionality is renamed to |getMethodFromOnlyDimension|.

> @@ +1169,5 @@
> > +        JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_PAR_ARRAY_REDUCE_EMPTY);
> > +        return false;
> > +    }
> > +
> > +    // Else do the fold. Create a destination buffer.
>
> Possible comment typo. Although the comment itself is mostly redundant.
>

Not a typo. The scan operation is like the prefix sum of reduce. Both are
expressible (well, I guess everything is) in terms of fold.

> @@ +1228,5 @@
> > +    uint32_t targetsLength;
> > +    if (!js_GetLengthProperty(cx, targets, &targetsLength))
> > +        return false;
> > +
> > +    // Throw if we have different number of targets than indices.
>
> This comment is confusing. It can probably just be deleted.
>
> But why is this an error?
>

Reworded. It is the error because the spec says so, probably to get early
reporting instead of weird bugs.

> @@ +1613,5 @@
> > +bool
> > +ParallelArrayObject::toSource(JSContext *cx, CallArgs args)
> > +{
> > +    // TODO: NYI
> > +    return toString(cx, args);
>
> Debris.
>

What does debris mean in this case?

> ::: js/src/builtin/ParallelArray.h
> @@ +17,5 @@
> > +class ParallelArrayObject;
> > +typedef Rooted<ParallelArrayObject *> RootedParallelArrayObject;
> > +typedef Handle<ParallelArrayObject *> HandleParallelArrayObject;
> > +
> > +class ParallelArrayObject : public JSObject {
>
> Please add a header comment with important general information about
> parallel arrays. One thing I would like to see clearly laid out is how the
> multidimensional parallel array gets mapped to a one-dimensional
> representation.
>

Fixed.

> @@ +21,5 @@
> > +class ParallelArrayObject : public JSObject {
> > +  public:
> > +    typedef Vector<uint32_t> IndexVector;
> > +
> > +    struct IndexInfo {
>
> Please add a comment explaining what this is (seems to be a tuple of indexes
> for an n-dimensional array of fixed dimensions) and how it's generally
> intended to be used.
>
> I note that the implementation feels a bit 'heavy' for a relatively simple
> object, using 3 separate vectors. I'm wondering if you at least want to make
> sure there's a bit of inline storage in the vectors, enough to cover typical
> use cases.
>

Made sure there are some inline storage.

> @@ +24,5 @@
> > +
> > +    struct IndexInfo {
> > +        IndexVector indices;
> > +        IndexVector dimensions;
> > +        IndexVector partialProducts;
>
> It would be good to have a 1-line comment on each saying what it's for,
> especially partialProducts.
>

Documented.

> @@ +35,5 @@
> > +        inline bool initialize(uint32_t space);
> > +
> > +        // Load dimensions from a source, then initialize.
> > +        inline bool initialize(JSContext *cx, HandleParallelArrayObject source,
> > +                               uint32_t space);
>
> The comments on the initialize methods need to be clearer about when these
> are to be called and what the parameters mean. E.g., what is 'space'?
>

Documented.

> @@ +48,5 @@
> > +    static Class class_;
> > +
> > +    static inline bool is(const Value &v);
> > +    static inline bool is(JSObject *obj);
> > +    static inline ParallelArrayObject *as(JSObject *obj);
>
> Interesting choice of names, but I kind of like the brevity.
>
> @@ +51,5 @@
> > +    static inline bool is(JSObject *obj);
> > +    static inline ParallelArrayObject *as(JSObject *obj);
> > +
> > +    inline JSObject *dimensionArray();
> > +    inline JSObject *buffer();
>
> I notice that in comment 0 you said "The shape (dimensions) array and the
> underlying buffer are kept as dense arrays _for now_ for ease of GC"
> (emphasis mine). If that helps you get the job done now, that's OK, but I
> would urge caution: a dense array is a far more complicated thing than what
> you need, and if code ends up depending on that extra complexity, it will be
> a lot of work to reverse that. I strongly recommend hiding that fact from
> the public interface. You might also want to talk to the GC people and see
> if they can give you an easy way to use vectors and stay GC-safe.
>

We've talked with Luke who has suggested to just use dense arrays. Plus, the
|shape| property _is_ user visible and we need to return an array anyways.

> @@ +53,5 @@
> > +
> > +    inline JSObject *dimensionArray();
> > +    inline JSObject *buffer();
> > +    inline uint32_t bufferOffset();
> > +    inline uint32_t outermostDimension();
>
> 'outermostDimension' is an example of something that's confusing if you
> don't document the way dimensions work. From reading other code, I gather
> it's based on the idea of viewing the linearized representation as
> containing nested lower-dimensional subarrays. It still unclear as yet as to
> why it's a particularly significant concept.
>

The outermost dimension is really just the outermost dimension of your
array. If the array is >1D, then the outermost dimension how many "rows" your
array has (even though each row could be higher dimension, etc).

It's a significant concept because all operations except the comprehension
form of the constructor operate on the outermost dimension only. For example,
if you have a 2x4 array, calling |map| on it will only iterate 2 times, over
the outermost dimension.

> @@ +70,5 @@
> > +  private:
> > +    enum {
> > +        // The ParallelArray API refers to dimensions as "shape", but to avoid
> > +        // confusion with the internal engine notion of a shape we call it
> > +        // "dimensions" here.
>
> Good idea.
>
> @@ +78,5 @@
> > +        SLOT_BUFFER,
> > +
> > +        // First index of the underlying buffer to be considered in
> > +        // bounds.
> > +        SLOT_BUFFER_OFFSET,
>
> Good comments here. Nit: join the above comment to one line.
>

Fixed.

> @@ +86,5 @@
> > +
> > +    // Execution modes for the implementations of various options.
> > +
> > +    enum ExecutionMode {
> > +        FallbackMode = 0,
>
> This needs a better name or an explanation. At first, I thought it was the
> mode to fall back on if nothing else was available (which of course confused
> me because I also thought SequentialMode would play that role). I'm also not
> sure it's necessary. Will there someday be more modes, and will it become
> more important? Or is it just to make the entry point methods cleaner?
>

Niko and I have refactored execution modes. See comments in patch.

> @@ +100,5 @@
> > +
> > +    template <ExecutionMode em>
> > +    struct Impl {
> > +        static ExecutionStatus build(JSContext *cx, IndexInfo &iv, HandleObject elementalFun,
> > +                                     HandleObject buffer);
>
> 'build' needs documentation because it's not explicitly named in the spec.
>

Documented.

> @@ +105,5 @@
> > +        static ExecutionStatus map(JSContext *cx, HandleParallelArrayObject source,
> > +                                   HandleObject elementalFun, HandleObject buffer);
> > +        static ExecutionStatus fold(JSContext *cx, HandleParallelArrayObject source,
> > +                                    HandleObject elementalFun, HandleObject buffer,
> > +                                    MutableHandleValue vp);
>
> 'fold' needs documentation because it's not really the same thing as the
> spec method 'fold'. In fact, it should probably be called something else.
>

The spec has no |fold|. It has a |reduce| and a |scan|, which is like a prefix
sum |reduce|. I've renamed it to reduceOrScan.

> @@ +133,5 @@
> > +    static const char *ExecutionModeToString(ExecutionMode em);
> > +    static const char *ExecutionStatusToString(ExecutionStatus ss);
> > +    static bool ToDebugOptions(JSContext *cx, const Value &v, DebugOptions &options);
> > +
> > +    struct CheckedImpl {
>
> Please document what CheckedImpl does.

This is a casualty of the execution modes refactor.

	David Mandelin [:dmandelin]
	Comment 10 • 12 years ago

(In reply to Shu-yu Guo [:shu] from comment #9)
> > - Do you have performance constraints? It looks like everything is
> > implemented in the easy-to-understand, not very optimized, but avoiding
> > terrible blowups style. Is that going to be good enough? Do you have to
> > compete with Array.map and the like?
> 
> For the sequential implementation, which this is, I don't think we have any
> performance constraints. We don't want to be stupid, but we're striving for
> clarity here. The performance gains from using a ParallelArray is supposed to
> come from the scalability and the parallelization, not the sequential
> fallback execution.

OK.

> There is no description of array-like in the spec, they're simply described
> as "array-like object". Does |JSObject::getElement| return false if [i] is
> missing? I thought that just set |elem| to undefined and return true?

I think you're right about that. In the doc for the constructor, the spec says that array like means it has a length property and enumerable properties from 0 to (length-1). It also says "if elements are missing, the undefined value is used". That surprised me, because in the earlier discussions they talked about ParallelArrays as being typed, although it looks like they are now not typed.

> > @@ +303,5 @@
> > > +
> > > +    uint32_t length = source->outermostDimension();
> > > +    bool flat = source->isOneDimensional();
> > > +
> > > +    // The accumulator: the objet petit a.
> >
> > The 'little object a'?
> >
> 
> Hah, sorry, this is a joke I couldn't resist:
> https://de.twitter.com/PLT_Zizek/status/166599698893897730
> 
> The 'objet petit a(utre)' is a term from Lacanian psychoanalytic theory,
> meaning 'the little other', where 'other' is used in a technical sense to
> mean the thing that is perceived and opposed to the subject, i.e., the object.

Just include the link or quotation then. :-D

> > @@ +460,5 @@
> > > +    if (!ArrayLikeToTruthyVector(cx, filtersObj, filters))
> > > +        return ExecutionFailed;
> > > +
> > > +    uint32_t pos = 0;
> > > +    for (uint32_t i = 0; i < filters.length(); i++) {
> >
> > Can put i++, pos++ here.
> 
> Can't, because pos isn't incremented every iteration, only when the filter
> array element is truthy.

Ah, I missed that.

> > @@ +561,5 @@
> > > +} // namespace js
> > > +
> > > +#ifdef DEBUG
> > > +
> > > +// XXX: I don't like this, but how do I do it without a macro?
> >
> > Construction debris. :-)
> 
> What does debris mean in this case? Execution modes got all reworked in any
> case and this is now gone.

I just meant the "XXX" was something you probably meant to cut out before posting the patch.

> > @@ +865,5 @@
> > > +ParallelArrayObject::getParallelArrayElement(JSContext *cx, uint32_t index, MutableHandleValue vp)
> > > +{
> > > +    JS_ASSERT(isOneDimensional());
> > > +
> > > +    uint32_t length = outermostDimension();
> >
> > For this usage, I think it's nicer to define a method 'getOnlyDimension'
> > (strawman name) that returns the dimension of a one-dimensional array, and
> > asserts otherwise.
> 
> Logically it's the |getParallelArrayElement| method that should only work on
> 1D arrays. I don't see an independent use case for getting the outermost
> dimension and asserting 1D-ness at the same time.

Hmm, maybe there wasn't one. 

> > @@ +898,5 @@
> > > +            vp.setUndefined();
> > > +        else
> > > +            vp.set(buffer()->getDenseArrayElement(index));
> > > +        return true;
> > > +    }
> >
> > This is somewhat of a duplicate of the 1-D form of getParallelArrayElement,
> > but that doesn't seem like too big a deal.
> 
> Here I wanted to avoid making a |RootedValue|.

That's a plenty good reason for a bit of duplication, I think.

> > @@ +1043,5 @@
> > > +        DebugOptions options;
> > > +        if (ToDebugOptions(cx, args[1], options)) {
> > > +            if (!CheckedImpl::build(cx, iv, elementalFun, buffer, options))
> > > +                return false;
> > > +            goto out;
> >
> > That's kind of unfortunate. I'm sure you can find a way to make this do what
> > you want without goto.
> 
> I will duplicate the code (it's pretty short) instead of a goto. I still like
> uses of goto to do some simple non-local control, though.

Yeah, that stuff's always a judgment call (or just taste (or just habit)). I've used that idiom before myself, but ISTR that in SpiderMonkey it was sometimes inconvenient for reasons I can't entirely remember (probably something about it being harder to refactor around or something).

> > @@ +1066,5 @@
> > > +        return false;
> > > +    }
> > > +
> > > +    // Get the parallel array.
> > > +    RootedParallelArrayObject obj(cx, as(&args.thisv().toObject()));
> >
> > Don't you need to check that it is a parallel array? Some of our code uses
> > 'maybeX' to do that in one step (and also thus avoid potentially unsafe
> > non-checked conversion ops).
> 
> No, because these are all wrapped with |CallNonGenericMethod|, so the
> receiver should already be a parallel array.

Ah, right.

> > @@ +1228,5 @@
> > > +    uint32_t targetsLength;
> > > +    if (!js_GetLengthProperty(cx, targets, &targetsLength))
> > > +        return false;
> > > +
> > > +    // Throw if we have different number of targets than indices.
> >
> > This comment is confusing. It can probably just be deleted.
> >
> > But why is this an error?
> 
> Reworded. It is the error because the spec says so, probably to get early
> reporting instead of weird bugs.

OK, I read that part of the spec again, and now I see that it throws if "the length of indices does not match the length of the ParallelArray". "match" is ambiguous; I notice that you are requiring the indices <= length, which I guess works because nothing blows up if the indices array is shorter.

> > @@ +1613,5 @@
> > > +bool
> > > +ParallelArrayObject::toSource(JSContext *cx, CallArgs args)
> > > +{
> > > +    // TODO: NYI
> > > +    return toString(cx, args);
> >
> > Debris.
> 
> What does debris mean in this case?

We usually don't check in code that has TODO or XXX left in it, although you will find some in the tree. They tend to indicate something that should have gotten fixed before landing, or else they become confusing or get forgotten about over time. If you need to land with something missing, it's better to file a bug for the missing piece. You can leave a brief explanatory comment, which can refer to that bug.

> > @@ +21,5 @@
> > > +class ParallelArrayObject : public JSObject {
> > > +  public:
> > > +    typedef Vector<uint32_t> IndexVector;
> > > +
> > > +    struct IndexInfo {
> >
> > Please add a comment explaining what this is (seems to be a tuple of indexes
> > for an n-dimensional array of fixed dimensions) and how it's generally
> > intended to be used.
> >
> > I note that the implementation feels a bit 'heavy' for a relatively simple
> > object, using 3 separate vectors. I'm wondering if you at least want to make
> > sure there's a bit of inline storage in the vectors, enough to cover typical
> > use cases.
> 
> Made sure there are some inline storage.

Cool. And of course, that may or may not be that significant given that you said you only need non-blowup perf for the sequential version.

> We've talked with Luke who has suggested to just use dense arrays. Plus, the
> |shape| property _is_ user visible and we need to return an array anyways.

OK.

> > @@ +53,5 @@
> > > +
> > > +    inline JSObject *dimensionArray();
> > > +    inline JSObject *buffer();
> > > +    inline uint32_t bufferOffset();
> > > +    inline uint32_t outermostDimension();
> >
> > 'outermostDimension' is an example of something that's confusing if you
> > don't document the way dimensions work. From reading other code, I gather
> > it's based on the idea of viewing the linearized representation as
> > containing nested lower-dimensional subarrays. It still unclear as yet as to
> > why it's a particularly significant concept.
> 
> The outermost dimension is really just the outermost dimension of your
> array. If the array is >1D, then the outermost dimension how many "rows" your
> array has (even though each row could be higher dimension, etc).
> 
> It's a significant concept because all operations except the comprehension
> form of the constructor operate on the outermost dimension only. For example,
> if you have a 2x4 array, calling |map| on it will only iterate 2 times, over
> the outermost dimension.

Thanks. And I saw you added that to the explanatory comment. I notice that the spec uses the word "outermost" several times, but never defines it, which is weird. I guess it means about what you'd expect it means.

> The spec has no |fold|. It has a |reduce| and a |scan|, which is like a
> prefix sum |reduce|. I've renamed it to reduceOrScan.

I kind of like to avoid XOrY names, but if you think that |fold| is too confusing in this context, fair enough.

I'll try to finish the review of the new version today, although I have some other stuff to do, so I might need until tomorrow.

	David Mandelin [:dmandelin]
	Comment 11 • 12 years ago

Comment on attachment 651606 [details] [diff] [review]
Milestone 1 v2

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

I have a few more remarks and suggestions; the docs could be improved further by taking another pass over the writing and considering the suggestions below. But it's basically ready to land and there's no need to fuss over details, so just make whatever improvements you think are worthwhile (if any) and carry over the r+.

::: js/src/builtin/ParallelArray.cpp
@@ +88,5 @@
> +//
> +// Otherwise we do what is done in GetElement in jsarray.cpp.
> +static bool
> +GetElement(JSContext *cx, HandleObject obj, HandleParallelArrayObject pa,
> +           IndexInfo &iv, uint32_t i, MutableHandleValue vp)

It would be good to name this GetElementHelper or something, to emphasize the fact that it's not a standard accessor function but rather something kind of special you need in order to make stuff work over multiple kinds of arrays.

@@ +169,5 @@
> +//
> +// The comprehension form. Build a parallel array from a dimension vector and
> +// using elementalFun, writing the results into buffer. The dimension vector
> +// and its partial products are kept in iv. The function elementalFun is passed
> +// indices as multiple arguments.

I think it would be a bit more idiomatic to put the comment over the "declaration".

@@ +179,5 @@
> +//
> +// Map elementalFun over the elements of the outermost dimension of source,
> +// writing the results into buffer. The buffer must be as long as the
> +// outermost dimension of the source. The elementalFun is passed
> +// (element, index, collection) as arguments, in that order.

Nice.

@@ +191,5 @@
> +// Reduce source in the outermost dimension using elementalFun. If vp is not
> +// null, then the final value of the reduction is stored into vp. If buffer is
> +// not null, then buffer[i] is the result of calling reduce on the subarray
> +// from [0,i]. The elementalFun is passed 2 values to be reduced and should be
> +// commutative and associative.

Probably don't want to say "should be commutative and associative" flat out, since the spec apparently says it's fine not to be, as long as you don't care about getting weird results. ;-)

@@ +240,5 @@
> +        args.setCallee(ObjectValue(*elementalFun));
> +        args.thisv().setUndefined();
> +
> +        // Compute and set indices.
> +        iv.fromScalar(i);

Hey, nice! I didn't even realize it would be *this* simple with the index machinery you had built.

@@ +1586,5 @@
> +
> +bool
> +ParallelArrayObject::toSource(JSContext *cx, CallArgs args)
> +{
> +    // TODO: NYI

As mentioned earlier, for this, just file a followup bug and replace the TODO with a 1-line explanatory comment.

::: js/src/builtin/ParallelArray.h
@@ +36,5 @@
> +//
> +// Except for the comprehension form, all operations (e.g. map, filter,
> +// reduce) operate on the outermost dimension only. That is, those operations
> +// only operate on the "rows" of the array.
> +//

Nice. Thanks for adding this. it strikes me that there is still some terminological trickiness around the meanings of "outermost" and especially "element". In the context of multidimensional arrays I expect "element" to mean a scalar, but I think some of your comments (and perhaps) also the spec use it to refer to any subarray you can get by indexing k outer dimensions, so it would be good to make that explicit.

@@ +44,5 @@
> +
> +    //
> +    // Helper structure to help index higher-dimensional arrays to convert
> +    // between scalar offsets in the flat dense array backing and mathematical
> +    // notation of indices.

Reads a little rough. It seems surprisingly difficult to describe concisely, though. I think one of the notable features is that an IndexInfo doesn't have to be an index to an element; that it can be an index of k<n integers that names an (n-k)-dimensional subarrays.

@@ +47,5 @@
> +    // between scalar offsets in the flat dense array backing and mathematical
> +    // notation of indices.
> +    //
> +    // Computation to and from scalar offsets require partial products of the
> +    // dimensions, which we cache here.

You can strike the above: you made it very clear in the comment below, so it's not needed here.

@@ +92,5 @@
> +
> +        // Prepares indices and partial products once dimensions have been
> +        // set. Resizes the indices vector to be of length |space| and
> +        // computes the partial product vector.
> +        inline bool initialize(uint32_t space);

This is kind of a tricky API, because of the preconditions. I see how you're using it and that there isn't an easy replacement, so not a huge deal, but I wanted to point that out. I think these methods would be easier to understand if you put a header over both of them saying that in order to use an IndexInfo, you have to (a) set the dimensions and (b) initialize; and that the 3-arg method does both in one step.

|space| is kind of implementation-y, but I think I get it. Please do note whether it's just a hint or is mandatory; and if so, what the constraint is.

@@ +215,5 @@
> +    HandleObject filters,              \
> +    HandleObject buffer
> +
> +#define DECLARE_OP(NAME) \
> +    ExecutionStatus NAME(NAME ## _ARGS)

Should probably put a JS_ prefix on these to help avoid name collisions.

@@ +286,5 @@
> +    // Debug options can be passed in as an extra argument to the
> +    // operations. The grammar is:
> +    //
> +    //   options ::= { mode: "par" | "seq",
> +    //                 expect: "fail" | "bail" | "success" }

Thanks for adding this. Btw, what's the long-term plan for these? I assume they can't be part of the final product, can they?

	Shu-yu Guo [:shu] Assignee
	Comment 12 • 12 years ago

Applied dmandelin's suggestions & small bug fixes as surfaced by the test suite. Note previous version was already r+

	Shu-yu Guo [:shu] Assignee
	Comment 14 • 12 years ago

http://hg.mozilla.org/integration/mozilla-inbound/rev/06dae3ea0f89

	Ryan VanderMeulen [:RyanVM]
	Comment 15 • 12 years ago

http://mozillamemes.tumblr.com/post/21637966463/yes-i-have-a-condition-that-forces-me-to-insert
Backed out for build failures on Windows. Links to Try pushes (assuming they were done) are also strongly advised. Also, please set the in-testsuite flag when landing.

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

	Ryan VanderMeulen [:RyanVM]
	Comment 16 • 12 years ago

Example failure:
https://tbpl.mozilla.org/php/getParsedLog.php?id=14446821&tree=Mozilla-Inbound

e:\builds\moz2_slave\m-in-w32\build\js\src\builtin/ParallelArray.h(235) : error C2010: '.' : unexpected in macro formal parameter list

	Shu-yu Guo [:shu] Assignee
	Comment 17 • 12 years ago

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

	Shu-yu Guo [:shu] Assignee
	Comment 18 • 12 years ago

http://hg.mozilla.org/integration/mozilla-inbound/rev/ea2ad8970f3e

	Ryan VanderMeulen [:RyanVM]
	Comment 19 • 12 years ago

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

	Daniel Veditz [:dveditz]
	Comment 20 • 12 years ago

Shu: what concerns do you have that you would like the security team to review? Just make sure the feature is covered adequately in testing/fuzzing, or do you think this warrants a low-level code review?

	Shu-yu Guo [:shu] Assignee
	Comment 21 • 12 years ago

(In reply to Daniel Veditz [:dveditz] from comment #20)
> Shu: what concerns do you have that you would like the security team to
> review? Just make sure the feature is covered adequately in testing/fuzzing,
> or do you think this warrants a low-level code review?

Well, this has already landed due to contractual reasons. I was made to understand later that new features that land need a security review, and this never received one. I've been fixing a steady stream of fuzzbugs in the meantime already; not sure if this needs low-level code review.

	Florian Scholz (Open Web Docs)
	Comment 22 • 10 years ago

I'm removing the documentation request here, as this API has been removed with bug 944074 entirely. 
There is still this page mentioning that it existed if people should come across ParallelArray in older Nightlies:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/ParallelArray

Please mark bugs introducing the new PJS APIs as dev-doc-needed for MDN!