[binary data] integrate binary data objects and typed arrays

RESOLVED FIXED in mozilla30

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: nmatsakis, Assigned: nmatsakis)

Tracking

(Blocks 3 bugs)

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

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(11 attachments, 12 obsolete attachments)

30.98 KB, patch
nmatsakis
: review+
Details | Diff | Splinter Review
19.07 KB, patch
nmatsakis
: review+
Details | Diff | Splinter Review
11.50 KB, patch
nmatsakis
: review+
Details | Diff | Splinter Review
29.10 KB, patch
nmatsakis
: review+
Details | Diff | Splinter Review
67.96 KB, patch
nmatsakis
: review+
Details | Diff | Splinter Review
8.49 KB, patch
nmatsakis
: review+
Details | Diff | Splinter Review
3.09 KB, patch
nmatsakis
: review+
Details | Diff | Splinter Review
20.50 KB, patch
nmatsakis
: review+
Details | Diff | Splinter Review
146.93 KB, patch
nmatsakis
: review+
Details | Diff | Splinter Review
155.15 KB, patch
nmatsakis
: review+
Details | Diff | Splinter Review
1.01 KB, patch
terrence
: review+
Details | Diff | Splinter Review
In some cases, binary data objects are supposed to be able to expose their raw contents as typed arrays using a `buffer` property. The `buffer` property can also be masked and reinstated. Precise details are to be found in the spec.
Posted patch Bug898356-Part1.diff (obsolete) — Splinter Review
Attachment #8374937 - Flags: review?(sphink)
Posted patch Bug898356-Part2.diff (obsolete) — Splinter Review
Attachment #8374938 - Flags: review?(sphink)
Posted patch Bug898356-Part3.diff (obsolete) — Splinter Review
Attachment #8374939 - Flags: review?(sphink)
Comment on attachment 8374937 [details] [diff] [review]
Bug898356-Part1.diff

Actually not sure if this is ready yet!
Attachment #8374937 - Attachment is obsolete: true
Attachment #8374937 - Flags: review?(sphink)
Attachment #8374938 - Attachment is obsolete: true
Attachment #8374938 - Flags: review?(sphink)
Attachment #8374939 - Attachment is obsolete: true
Attachment #8374939 - Flags: review?(sphink)
Posted patch Bug898356-Part1.diff (obsolete) — Splinter Review
Attachment #8375153 - Flags: review?(sphink)
Posted patch Bug898356-Part2.diff (obsolete) — Splinter Review
Attachment #8375154 - Flags: review?(sphink)
Posted patch Bug898356-Part3.diff (obsolete) — Splinter Review
This change updates the constructor to more closely match the intended spec. In particular, the old constructor for unsized arrays required both an exemplar  and a length (new Array(3, [1, 2, 3]) which isn't really how it's supposed to work. Unlike most of the patches, this one is intended to change the behavior in a non-backwards compatible way.
Attachment #8375156 - Flags: review?(sphink)
Posted patch Bug898356-Part4.diff (obsolete) — Splinter Review
Once typed objects are operating over array buffers, we must be wary of them being neutered. Sometimes this check can be folded into an array bounds check, but not always.
Attachment #8375157 - Flags: review?(jdemooij)
Posted patch Bug898356-Part5.diff (obsolete) — Splinter Review
This is the big patch. It stops using a custom void* allocation and instead allocates an array buffer.
Attachment #8375158 - Flags: review?(sphink)
Posted patch Bug898356-Part6.diff (obsolete) — Splinter Review
Tests for the neutering checks. These are separated into a separate patch because they require both parts 4 and 5 to work.
Attachment #8375160 - Flags: review?(jdemooij)
Posted patch Bug898356-Part7.diff (obsolete) — Splinter Review
Rename handles / typed object to opaque/transparent typed objects, since that is now the only difference.
Attachment #8375163 - Flags: review?(sphink)
Posted patch Bug898356-Part8.diff (obsolete) — Splinter Review
Rename datum to typed object, since everything is a typed object now.
Attachment #8375164 - Flags: review?(sphink)
Assignee: general → nmatsakis
(In reply to Niko Matsakis [:nmatsakis] from comment #10)
> This is the big patch. It stops using a custom void* allocation and instead
> allocates an array buffer.

sfink -- some additional notes:

1. The user visible behavior doesn't change much, but:

a. When creating a (transparent) typed object, you may specify a buffer/offset manually if you choose.
b. The patch also adds the `storage()` function which returns the storage of the typed object.

(Though in the meantime Dmitry and I agreed to change this function into distinct `buffer`, `byteOffset`, and `byteLength` functions to avoid the necessity of allocating an object. I'll either open a new bug or add a new patch for that change.)

2. When reviewing part 5, note the later patches that rename TypedHandle to OpaqueTypedObject and TypedObject to TransparentTypedObject. Probably I should have reordered these, but that's... not how I did it. Sorry :)
Blocks: 972398
Blocks: 972400
Blocks: 972403
Blocks: 972581
Comment on attachment 8375157 [details] [diff] [review]
Bug898356-Part4.diff

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

r=me with nits addressed.

::: js/src/jit/CodeGenerator.cpp
@@ +3122,5 @@
> +        // DO NOT COMMIT
> +        //char *foo = (char*) malloc(512);
> +        //sprintf(foo, "MIR instruction %d returned object with unexpected type",
> +        //mir->id());
> +        //masm.assumeUnreachable(foo);

Do not commit :)

@@ +4102,5 @@
> +    if (!bailoutIf(Assembler::Zero, lir->snapshot()))
> +        return false;
> +
> +    // FIXME There is prob a better way to do this passthrough
> +    masm.movePtr(obj, out);

Yeah, you probably want to do what LTypeBarrier does:

(1) In Lowering::visitNeuterCheck, do "redefine(ins, ins->input()) && add(...)" instead of define().
(2) Change LNeuterCheck to have 0 defs.
Attachment #8375157 - Flags: review?(jdemooij) → review+
Attachment #8375160 - Flags: review?(jdemooij) → review+
Comment on attachment 8375153 [details] [diff] [review]
Bug898356-Part1.diff

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

I'm going to mark this r+ because I don't want the naming to block landing, especially since you can do a further renaming on top of the rest of the patch series. But I'd at least like answers to the naming questions. Sorry for the trouble.

::: js/src/builtin/TypedObject.h
@@ +9,5 @@
>  
>  #include "jsobj.h"
>  
>  #include "builtin/TypedObjectConstants.h"
> +#include "builtin/TypedObjectSimple.h"

I'm not crazy about the naming. We don't use this pattern anywhere else. Is there a better description of what the difference is than "simple" vs "not simple"?

And a related question: what part of this should be exposed externally? It really feels like some part of this ought to live in js/public. Is that the same stuff that is now in TypedObjectSimple.h?
Attachment #8375153 - Flags: review?(sphink) → review+
Comment on attachment 8375154 [details] [diff] [review]
Bug898356-Part2.diff

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

It would be nice to have the additional detail, but given how much this simplifies, this seems better for now.
Attachment #8375154 - Flags: review?(sphink) → review+
Comment on attachment 8375156 [details] [diff] [review]
Bug898356-Part3.diff

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

::: js/src/builtin/TypedObject.cpp
@@ +2108,5 @@
> +    //   new TypeObj()
> +    //   new TypeObj(buffer, [offset]) [1]
> +    //   new TypeObj(data)
> +    //
> +    // [1] coming in a later patch in this series

I'm going to forget to check whether you removed this part of the comment. ;-)
Attachment #8375156 - Flags: review?(sphink) → review+
Comment on attachment 8375158 [details] [diff] [review]
Bug898356-Part5.diff

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

Very nicely done! This is great.

::: js/src/builtin/TypedObject.cpp
@@ +1996,5 @@
> +
> +void
> +TypedDatum::neuter(JSContext *cx)
> +{
> +    AutoLockForCompilation lock(cx);

What's AutoLockForCompilation? I don't see it in the tree. Do typed arrays need it?

@@ +2136,5 @@
> +        Rooted<ArrayBufferObject*> buffer(cx);
> +        buffer = &args[0].toObject().as<ArrayBufferObject>();
> +
> +        int32_t offset;
> +        if (argc >= 2 && !args[1].isUndefined()) {

I know it's more verbose, but I really think you should use args.length() instead of argc to make the transition to CallArgs easier (as in, sometime natives are going to switch from argc/vp to passing in args.)
Attachment #8375158 - Flags: review?(sphink) → review+
Attachment #8375163 - Flags: review?(sphink) → review+
Comment on attachment 8375164 [details] [diff] [review]
Bug898356-Part8.diff

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

I barely skimmed this patch. Hopefully there's nothing nasty hidden in it anywhere. ;-)
Attachment #8375164 - Flags: review?(sphink) → review+
(In reply to Steve Fink [:sfink] from comment #16)
> ::: js/src/builtin/TypedObject.h
> @@ +9,5 @@
> >  
> >  #include "jsobj.h"
> >  
> >  #include "builtin/TypedObjectConstants.h"
> > +#include "builtin/TypedObjectSimple.h"
> 
> I'm not crazy about the naming. We don't use this pattern anywhere else. Is
> there a better description of what the difference is than "simple" vs "not
> simple"?

I wasn't crazy about this division either. The reason for it was so that typed objects could extend ArrayBufferViewObject. Probably the best thing would be to separate out array buffers (and ArrayBufferViewObject) from the rest of typed arrays, so that it could be included by both typed objects and typed arrays. What do you think about that idea?

> And a related question: what part of this should be exposed externally? It
> really feels like some part of this ought to live in js/public. Is that the
> same stuff that is now in TypedObjectSimple.h?

Good question, I don't really know just what should be exposed publicly.
(In reply to Steve Fink [:sfink] from comment #19)
> What's AutoLockForCompilation? I don't see it in the tree. Do typed arrays
> need it?

I copied it from typed arrays, maybe it's been removed in the meantime.

> I know it's more verbose, but I really think you should use args.length()
> instead of argc to make the transition to CallArgs easier (as in, sometime
> natives are going to switch from argc/vp to passing in args.)

OK.
(In reply to Niko Matsakis [:nmatsakis] from comment #21)
> (In reply to Steve Fink [:sfink] from comment #16)
> > ::: js/src/builtin/TypedObject.h
> > @@ +9,5 @@
> > >  
> > >  #include "jsobj.h"
> > >  
> > >  #include "builtin/TypedObjectConstants.h"
> > > +#include "builtin/TypedObjectSimple.h"
> > 
> > I'm not crazy about the naming. We don't use this pattern anywhere else. Is
> > there a better description of what the difference is than "simple" vs "not
> > simple"?
> 
> I wasn't crazy about this division either. The reason for it was so that
> typed objects could extend ArrayBufferViewObject. Probably the best thing
> would be to separate out array buffers (and ArrayBufferViewObject) from the
> rest of typed arrays, so that it could be included by both typed objects and
> typed arrays. What do you think about that idea?

I like that much better! Especially given that now that typed objects use ArrayBuffers too, there's no reason to treat typed arrays specially (as in, group them with ArrayBuffer) as compared to other ArrayBufferViewObjects.

> > And a related question: what part of this should be exposed externally? It
> > really feels like some part of this ought to live in js/public. Is that the
> > same stuff that is now in TypedObjectSimple.h?
> 
> Good question, I don't really know just what should be exposed publicly.

Ok. May as well punt on that for now, then. I bet Gecko will find some reason to want more access before long, so we can deal with it then.
rebased, nits addressed
Attachment #8375153 - Attachment is obsolete: true
Attachment #8378502 - Flags: review+
Attachment #8375154 - Attachment is obsolete: true
Attachment #8378503 - Flags: review+
Attachment #8375156 - Attachment is obsolete: true
Attachment #8378504 - Flags: review+
Attachment #8375157 - Attachment is obsolete: true
Attachment #8375158 - Attachment is obsolete: true
Attachment #8378590 - Flags: review+
s/argc/args.length()/ per sfink's request
Attachment #8378592 - Flags: review+
Attachment #8375160 - Attachment is obsolete: true
Attachment #8378594 - Flags: review+
Attachment #8375163 - Attachment is obsolete: true
Attachment #8378596 - Flags: review+
Attachment #8375164 - Attachment is obsolete: true
Attachment #8378598 - Flags: review+
Posted patch Bug898356-Part9.diff (obsolete) — Splinter Review
Refactor into ArrayBufferObject.h per sfink's request
Attachment #8378599 - Flags: review+
Attachment #8378505 - Flags: review+
Rebased, updated diffs, carried over r+ in all cases
Rebase, move methods of ArrayBufferViewObject
Attachment #8378599 - Attachment is obsolete: true
Attachment #8378795 - Flags: review+
Barrier changes to the void* pointer associated with a TypedObject. This fixes a problem observed in SM(r) builds on try server, where the typed object would be tenured before being attached to the array buffer, and would then not be traced as part of a minor gc and hence not have its view updated when buffer is moved.
Attachment #8378799 - Flags: review?(terrence)
Comment on attachment 8378799 [details] [diff] [review]
Bug898356-Part10.diff

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

r=me
Attachment #8378799 - Flags: review?(terrence) → review+
Just FYI, the Octane 2 Mandreel benchmarks on AWFY appear to have regressed substantially with this patch (or perhaps the patch in 930477). Regression window is http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=25168278f39a&tochange=cc5ce0bdff58
In fact it is not this patch that caused the regression, but rather Bug 930477 (at least in my own testing).
You need to log in before you can comment on or make changes to this bug.