898356 - [binary data] integrate binary data objects and typed arrays

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Niko Matsakis [:nmatsakis]]

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.

Comment 1

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

link to the spec please.

Comment 2

[Niko Matsakis [:nmatsakis]]

Attachment: Bug898356-Part1.diff

Comment 3

[Niko Matsakis [:nmatsakis]]

Attachment: Bug898356-Part2.diff

Comment 4

[Niko Matsakis [:nmatsakis]]

Attachment: Bug898356-Part3.diff

Comment 5

[Niko Matsakis [:nmatsakis]]

Comment on attachment 8374937 [details] [diff] [review]
Bug898356-Part1.diff

Actually not sure if this is ready yet!

Comment 6

[Niko Matsakis [:nmatsakis]]

Attachment: Bug898356-Part1.diff

Comment 7

[Niko Matsakis [:nmatsakis]]

Attachment: Bug898356-Part2.diff

Comment 8

[Niko Matsakis [:nmatsakis]]

Attachment: Bug898356-Part3.diff

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.

Comment 9

[Niko Matsakis [:nmatsakis]]

Attachment: Bug898356-Part4.diff

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.

Comment 10

[Niko Matsakis [:nmatsakis]]

Attachment: Bug898356-Part5.diff

This is the big patch. It stops using a custom void* allocation and instead allocates an array buffer.

Comment 11

[Niko Matsakis [:nmatsakis]]

Attachment: Bug898356-Part6.diff

Tests for the neutering checks. These are separated into a separate patch because they require both parts 4 and 5 to work.

Comment 12

[Niko Matsakis [:nmatsakis]]

Attachment: Bug898356-Part7.diff

Rename handles / typed object to opaque/transparent typed objects, since that is now the only difference.

Comment 13

[Niko Matsakis [:nmatsakis]]

Attachment: Bug898356-Part8.diff

Rename datum to typed object, since everything is a typed object now.

Comment 14

[Niko Matsakis [: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 :)

Comment 15

[Jan de Mooij [:jandem]]

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.

Comment 16

[Steve Fink [:sfink] [:s:]]

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?

Comment 17

[Steve Fink [:sfink] [:s:]]

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.

Comment 18

[Steve Fink [:sfink] [:s:]]

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. ;-)

Comment 19

[Steve Fink [:sfink] [:s:]]

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.)

Comment 20

[Steve Fink [:sfink] [:s:]]

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. ;-)

Comment 21

[Niko Matsakis [:nmatsakis]]

(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.

Comment 22

[Niko Matsakis [:nmatsakis]]

(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.

Comment 23

[Steve Fink [:sfink] [:s:]]

(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.

Comment 24

[Niko Matsakis [:nmatsakis]]

Attachment: Bug898356-Part1.diff

rebased, nits addressed

Comment 25

[Niko Matsakis [:nmatsakis]]

Attachment: Bug898356-Part2.diff

Comment 26

[Niko Matsakis [:nmatsakis]]

Attachment: Bug898356-Part3.diff

Comment 27

[Niko Matsakis [:nmatsakis]]

Attachment: Bug898356-Part4.diff

Comment 28

[Niko Matsakis [:nmatsakis]]

Attachment: Bug898356-Part5.diff

Comment 29

[Niko Matsakis [:nmatsakis]]

Attachment: Bug898356-Part5b.diff

s/argc/args.length()/ per sfink's request

Comment 30

[Niko Matsakis [:nmatsakis]]

Attachment: Bug898356-Part6.diff

Comment 31

[Niko Matsakis [:nmatsakis]]

Attachment: Bug898356-Part7.diff

Comment 32

[Niko Matsakis [:nmatsakis]]

Attachment: Bug898356-Part8.diff

Comment 33

[Niko Matsakis [:nmatsakis]]

Attachment: Bug898356-Part9.diff

Refactor into ArrayBufferObject.h per sfink's request

Comment 34

[Niko Matsakis [:nmatsakis]]

Rebased, updated diffs, carried over r+ in all cases

Comment 35

[Niko Matsakis [:nmatsakis]]

Attachment: Bug898356-Part9.diff

Rebase, move methods of ArrayBufferViewObject

Comment 36

[Niko Matsakis [:nmatsakis]]

Attachment: Bug898356-Part10.diff

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.

Comment 37

[Niko Matsakis [:nmatsakis]]

Try run: https://tbpl.mozilla.org/?tree=Try&rev=751c5b186a05 (seems green)

Comment 38

[Terrence Cole [:terrence]]

Comment on attachment 8378799 [details] [diff] [review]
Bug898356-Part10.diff

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

r=me

Comment 39

[Niko Matsakis [:nmatsakis]]

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=cc73b1f7a47d

Comment 40

[Thomas Wisniewski]

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

Comment 41

[Niko Matsakis [:nmatsakis]]

In fact it is not this patch that caused the regression, but rather Bug 930477 (at least in my own testing).

Comment 42

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/94fda27a798e
https://hg.mozilla.org/mozilla-central/rev/f163330fdc38
https://hg.mozilla.org/mozilla-central/rev/4285c0b53c33
https://hg.mozilla.org/mozilla-central/rev/b2ed6214694f
https://hg.mozilla.org/mozilla-central/rev/f3ad4ab36594
https://hg.mozilla.org/mozilla-central/rev/ae9593dcca2a
https://hg.mozilla.org/mozilla-central/rev/54fc8d0a12e5
https://hg.mozilla.org/mozilla-central/rev/e27f18b4e818
https://hg.mozilla.org/mozilla-central/rev/1fd48bfe7f70
https://hg.mozilla.org/mozilla-central/rev/5c4cec0ab08a
https://hg.mozilla.org/mozilla-central/rev/cc73b1f7a47d