Closed Bug 989276 Opened 6 years ago Closed 5 years ago

Refactor slots on array buffer view objects

Categories

(Core :: JavaScript Engine, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: nmatsakis, Assigned: nmatsakis)

References

Details

Attachments

(2 files, 2 obsolete files)

Currently we store both BYTELENGTH and LENGTH. This is unnecessary, requiring an extra slot.
Attached patch Bug989276.diff (obsolete) — Splinter Review
Attachment #8398433 - Flags: review?(sphink)
Blocks: 973238
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.
Attachment #8398433 - Attachment is obsolete: true
Attachment #8398433 - Flags: review?(sphink)
Attached patch Bug989276.diffSplinter Review
OK, better.
Attachment #8398435 - Flags: review?(sphink)
Assignee: nobody → nmatsakis
Summary: Remove BYTELENGTH from typed arrays, data views, and typed objects → Refactor slots on array buffer view objects
Attached patch Bug989276-Part2.diff (obsolete) — Splinter Review
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.
Attachment #8398769 - Flags: review?(sphink)
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
Attachment #8398435 - Flags: review?(sphink) → review+
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.
Attachment #8398769 - Flags: review?(sphink) → review-
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).
Attachment #8398769 - Attachment is obsolete: true
Attachment #8400359 - Flags: review?(sphink)
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.
Attachment #8400359 - Flags: review?(sphink) → review+
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.
The latest bug to join the bug 1019955 fan club. Pushed a CLOBBER touch.
https://hg.mozilla.org/integration/mozilla-inbound/rev/7084e7ad7951
https://hg.mozilla.org/mozilla-central/rev/b1e0a1050d9b
https://hg.mozilla.org/mozilla-central/rev/039b9e7b3616
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.