989276 - Refactor slots on array buffer view objects

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Niko Matsakis [:nmatsakis]]

Currently we store both BYTELENGTH and LENGTH. This is unnecessary, requiring an extra slot.

Comment 1

[Niko Matsakis [:nmatsakis]]

Attachment: Bug989276.diff

Comment 2

[Niko Matsakis [:nmatsakis]]

Comment on attachment 8398433 [details] [diff] [review]
Bug989276.diff

Oops, this version was poorly rebased out of my branch. I'll post an updated one.

Comment 3

[Niko Matsakis [:nmatsakis]]

Attachment: Bug989276.diff

OK, better.

Comment 4

[Niko Matsakis [:nmatsakis]]

Try run: https://tbpl.mozilla.org/?tree=Try&rev=4b7e158b9a30

Comment 5

[Niko Matsakis [:nmatsakis]]

Attachment: Bug989276-Part2.diff

Here is another patch that removes the use of the "private" slot and instead just reserves an extra slot for the data pointer. Since array buffer view objects define their own trace, we still don't use PointerValue (since it implies alignment restrictions).

One thing I'm still not sure about is whether I'm doing something wrong with respect to barriers here. I don't quite see how I could be, since any change to the pointer must involve a change to the offset field (at minimum), or be part of the adjustments in the trace hook.

Comment 6

[Niko Matsakis [:nmatsakis]]

Try run with both patches: https://tbpl.mozilla.org/?tree=Try&rev=946b8b880863

Comment 7

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

Comment on attachment 8398435 [details] [diff] [review]
Bug989276.diff

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

Good idea, especially with typed objects merged in.

::: js/src/vm/ArrayBufferObject.h
@@ +230,5 @@
>      /* Offset of view in underlying ArrayBufferObject */
>      static const size_t BYTEOFFSET_SLOT  = JS_TYPEDOBJ_SLOT_BYTEOFFSET;
>  
>      /* Byte length of view */
> +    static const size_t LENGTH_SLOT      = JS_TYPEDOBJ_SLOT_LENGTH;

Update the comment

Comment 8

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

Comment on attachment 8398769 [details] [diff] [review]
Bug989276-Part2.diff

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

I don't see how this will work. ArrayBuffers are native objects. Their slots will be marked during GC by interpreting them as Values (whether or not you define a custom trace hook). Random JSObject pointers stored into them aren't going to be valid.

You are correct that PrivateValue won't work due to alignment constraints.

You could use space after the slots. You'd probably want to land that after bug 987508, which collides with this patch quite a bit too. I guess that wouldn't be hugely different from using the private slot, though.

::: js/src/vm/ArrayBufferObject.h
@@ +227,5 @@
>  class ArrayBufferViewObject : public JSObject
>  {
>    protected:
>      /* Offset of view in underlying ArrayBufferObject */
> +    static const size_t DATA_SLOT        = JS_BUFVIEW_SLOT_DATA;

Needs a new comment.

@@ +235,3 @@
>  
>      /* Byte length of view */
> +    static const size_t LENGTH_SLOT      = JS_BUFVIEW_SLOT_LENGTH;

(from previous patch) needs updated comment.

Comment 9

[Niko Matsakis [:nmatsakis]]

Attachment: Bug989276-Part2.diff

A less ambitious form of the Part 2 patch. Retains private data for the view pointer, just makes it less ad-hoc. Structures the slot constants by type. One possible perf regression is that I changed TypedArrayObject::viewData() to just call getPrivate(), which then presumably has to go through the shape. I figured this is not the fast path (which is the JIT).

Comment 10

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

Comment on attachment 8400359 [details] [diff] [review]
Bug989276-Part2.diff

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

::: js/src/builtin/TypedObject.cpp
@@ +2219,5 @@
>  TypedObject::dataOffset()
>  {
> +    // Compute offset of private data based on TransparentTypedObject;
> +    // both OpaqueTypedObject and TransparentTypedObject have the same
> +    // number of slots, so no problem there.

Oh, yeah? Assert! :-)

@@ +2222,5 @@
> +    // both OpaqueTypedObject and TransparentTypedObject have the same
> +    // number of slots, so no problem there.
> +    gc::AllocKind allocKind = gc::GetGCObjectKind(&TransparentTypedObject::class_);
> +    size_t nfixed = gc::GetGCKindSlots(allocKind);
> +    return JSObject::getPrivateDataOffset(nfixed - 1);

This conflicts a bit with the usual usage of nfixed (eg getPrivate() ends up looking at inlineSlots[nfixed], not inlineSlots[nfixed-1]). Though in looking deeper, it seems like our terminology and usage are totally tangled up. Still, seeing "nfixed - 1" sets of alarm bells.

Option #1: just rename nfixed here to nslots. Or if you want to be precise, nInlineSlots or nFixedOrPrivateSlots. But nslots is fine.

Option #2: use this instead:

  Class *clasp = &TransparentTypedObject::class_;
  size_t privateSlot = gc::GetGCKindSlots(gc::GetGCObjectKind(clasp), clasp);
  return JSObject::getPrivateDataOffset(privateSlot);

thus proving that all this stuff is totally insane.

Comment 11

[Niko Matsakis [:nmatsakis]]

Waldo -- I saw your questions on IRC, I had long ago intended to remove BYTELENGTH actually, as you can see from the patches on this bug, but I forgot I never landed it. Thanks for reminding me.

Comment 12

[Niko Matsakis [:nmatsakis]]

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

Comment 13

[Niko Matsakis [:nmatsakis]]

Pushed to inbound:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b1e0a1050d9b
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/039b9e7b3616

Comment 14

[Ryan VanderMeulen [:RyanVM]]

The latest bug to join the bug 1019955 fan club. Pushed a CLOBBER touch.
https://hg.mozilla.org/integration/mozilla-inbound/rev/7084e7ad7951

Comment 15

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

https://hg.mozilla.org/mozilla-central/rev/b1e0a1050d9b
https://hg.mozilla.org/mozilla-central/rev/039b9e7b3616