Implement parallelized version of mapPar for typed object arrays

RESOLVED FIXED in mozilla30

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: nmatsakis, Assigned: nmatsakis)

Tracking

unspecified
mozilla30
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

Assignee

Description

5 years ago
As a first step towards parallelizing the full set of operations over typed object arrays, implement a parallelized version of mapPar for 1, 2, and 3 dimensions.
Assignee

Updated

5 years ago
Blocks: PJS
Assignee

Updated

5 years ago
Assignee: nobody → nmatsakis
Assignee

Updated

5 years ago
Depends on: 898356
Assignee

Comment 1

5 years ago
Posted patch Bug972581-Part1.diff (obsolete) — Splinter Review
Just a little cleanup.
Attachment #8375958 - Flags: review?(till)
Assignee

Comment 2

5 years ago
Posted patch Bug972581-Part2.diff (obsolete) — Splinter Review
More cleanup.
Attachment #8375959 - Flags: review?(till)
Comment on attachment 8375958 [details] [diff] [review]
Bug972581-Part1.diff

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

sure
Attachment #8375958 - Flags: review?(till) → review+
Assignee

Comment 4

5 years ago
Posted patch Bug972581-Part3.diff (obsolete) — Splinter Review
This patch implements parallel map for a depth of 1. It needs some cleanup and some optimization -- for example, it relies on a number of functions for which I defined parallel equivalents in C, but which ought to be inlined. But it seems to work, at least for simple tests. Anyway, I doubt I will push just yet, but Shu I'd appreciate a quick look.
Attachment #8375961 - Flags: feedback?(shu)
Comment on attachment 8375959 [details] [diff] [review]
Bug972581-Part2.diff

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

r=me, after checking with nmatsakis that the change to using opaque instead of derived pointers in some places is correct.
Attachment #8375959 - Flags: review?(till) → review+
Assignee

Comment 6

5 years ago
Note to self: regarding till's point about the change to opaque, we need a test that checks that out pointers are opaque!

Comment 7

5 years ago
Comment on attachment 8375961 [details] [diff] [review]
Bug972581-Part3.diff

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

The general approach in mapThread looks good to me, though I just a did once-through. I noted the part I was most confused about (redirect).

Didn't really pay attention to the small helper functions and if they'll JIT well; that won't be too hard to fix.

::: js/src/builtin/TypedObject.js
@@ +962,5 @@
> +    return MapTypedParImpl(this, 1, thisType, a);
> +  else if (typeof a === "number")
> +    return ThrowError(JSMSG_TYPEDOBJECT_BAD_ARGS);
> +  else
> +    return ThrowError(JSMSG_TYPEDOBJECT_BAD_ARGS);

Nit: return after else

@@ +1436,5 @@
> +  ForkJoin(mapThread, ShrinkLeftmost(slicesInfo), ForkJoinMode(mode));
> +  return outArray;
> +
> +  function redirect(typedObj, offset) {
> +    if (false && (!InParallelSection() || !outGrainTypeIsTransparent)) {

if (false) ?

@@ +1450,5 @@
> +      // if it does. But those don't exist yet.
> +      //
> +      // Moreover, checking if the type is transparent is an
> +      // overapproximation: users can manually declare opaque types
> +      // that nonetheless only contain scalar data.

This comment confuses me. Let me see if I understand:

The property we're trying to guarantee is to not let the in/out fat pointers we pass to the kernel functions to escape. This could happen in sequential mode or when the output grain type is opaque, meaning that output type may contain an opaque reference type Object or Any. In those cases, we make a copy of the fat pointer.

I'm not exactly sure *why* this comment confuses me. I think I'm overloading the use of "object" between "typed object" and "JS object" in my mind, and logically it kinda makes sense for a typed object to "contain" other typed objects, in that you have nested StructTypes, but here you really mean JS objects.

@@ +1480,5 @@
> +      // the different iterations cannot communicate typed object
> +      // pointers to one another during parallel exec, we need only
> +      // fear escaped typed objects from *other* slices, so we can
> +      // just set the target region once.
> +      var endOffset = std_Math_imul(outGrainTypeSize, indexEnd + 1);

Is the + 1 necessary, since indexEnd is already 1 beyond the last writable index?
Attachment #8375961 - Flags: feedback?(shu) → feedback+
Assignee

Comment 8

5 years ago
(In reply to Shu-yu Guo [:shu] from comment #7)
> > +  function redirect(typedObj, offset) {
> > +    if (false && (!InParallelSection() || !outGrainTypeIsTransparent)) {
> 
> if (false) ?

Sorry, that's a FIXME -- `InParallelSection()` OUGHT to be replaced with a constant in ion, but for some reason it wasn't, and it was causing parallel execution to bailout. Or something. I haven't figured out the full story yet.

> I'm not exactly sure *why* this comment confuses me. I think I'm overloading
> the use of "object" between "typed object" and "JS object" in my mind, and
> logically it kinda makes sense for a typed object to "contain" other typed
> objects, in that you have nested StructTypes, but here you really mean JS
> objects.

I'll see if I can make it clearer, but your explanation was correct.

> 
> @@ +1480,5 @@
> > +      var endOffset = std_Math_imul(outGrainTypeSize, indexEnd + 1);
> 
> Is the + 1 necessary, since indexEnd is already 1 beyond the last writable
> index?

Ah, the classic off by one error. Good catch.
Assignee

Updated

5 years ago
Blocks: 974981
Assignee

Comment 9

5 years ago
Attachment #8375958 - Attachment is obsolete: true
Attachment #8375959 - Attachment is obsolete: true
Attachment #8379138 - Flags: review+
Assignee

Comment 10

5 years ago
Posted patch Bug972581-Part2.diff (obsolete) — Splinter Review
Basically same as before but added a test and fixed the if (false) business
Attachment #8375961 - Attachment is obsolete: true
Attachment #8379139 - Flags: review?(shu)
Assignee

Comment 11

5 years ago
Comment on attachment 8379139 [details] [diff] [review]
Bug972581-Part2.diff

Oops, clearing r? because I forgot to address the feedback comments. New version coming soon!
Attachment #8379139 - Flags: review?(shu)
Assignee

Comment 12

5 years ago
Attachment #8379139 - Attachment is obsolete: true
Attachment #8379151 - Flags: review?(shu)
Comment on attachment 8379151 [details] [diff] [review]
Bug972581-Part2.diff

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

LGTM. I didn't look very closely at the tests.

Most comments are small; main thing I'd like to see addressed is moving the PJS macros into Utilities.js.

::: js/src/builtin/TypedObject.cpp
@@ +2673,5 @@
> +
> +    JS_ASSERT(typedObj.typedMem() != nullptr); // must be attached already
> +
> +    typedObj.setPrivate(typedObj.owner().dataPointer() + offset);
> +    typedObj.setReservedSlot(JS_TYPEDOBJ_SLOT_BYTEOFFSET, Int32Value(offset));

I'm somewhat surprised there's no existing setOffset that encapsulates the slot.

::: js/src/builtin/TypedObject.js
@@ +953,5 @@
> +  if (!TypeDescrIsArrayType(thisType))
> +    return ThrowError(JSMSG_TYPEDOBJECT_BAD_ARGS);
> +
> +  // Arguments: [depth], func
> +  if (typeof a === "number" && typeof b === "function")

I imagine we want a IsCallable check instead of a typeof == function check.

@@ +958,5 @@
> +    return MapTypedParImpl(this, a, thisType, b);
> +  else if (typeof a === "function")
> +    return MapTypedParImpl(this, 1, thisType, a);
> +  else if (typeof a === "number")
> +    return ThrowError(JSMSG_TYPEDOBJECT_BAD_ARGS);

What is this branch for? It seems subsumed by the else below.

@@ +960,5 @@
> +    return MapTypedParImpl(this, 1, thisType, a);
> +  else if (typeof a === "number")
> +    return ThrowError(JSMSG_TYPEDOBJECT_BAD_ARGS);
> +  else
> +    return ThrowError(JSMSG_TYPEDOBJECT_BAD_ARGS);

Nit: return after else here and above. Blame Brendan.

@@ +1410,5 @@
> +    // that assume the value won't escape and copy it if it
> +    // does. But those don't exist yet.  Moreover, checking if the
> +    // type is transparent is an overapproximation: users can
> +    // manually declare opaque types that nonetheless only contain
> +    // scalar data.

I find this comment block easier to understand than before.

@@ +1455,5 @@
> +    var inTypedObject = inPointer.getDerivedIf(inGrainTypeIsComplex);
> +    var outPointer = new TypedObjectPointer(outGrainType, outArray, 0);
> +    var outTypedObject = outPointer.getOpaqueIf(outGrainTypeIsComplex);
> +    callFunction(std_Array_push, pointers, { inTypedObject: inTypedObject,
> +                                             outTypedObject: outTypedObject });

There're macros like ARRAY_PUSH #defined in Array.js.

We should move those and the PJS-specific macros from Array.js into Utilities.js. I imagine the PJS macros like GET_SLICE work right now only by virtue of TypedObject.js being #included after Array.js
Attachment #8379151 - Flags: review?(shu) → review+
Assignee

Updated

5 years ago
Depends on: 975456
https://hg.mozilla.org/mozilla-central/rev/a7c61b562512
https://hg.mozilla.org/mozilla-central/rev/108209641936
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.