Closed Bug 1073842 Opened 10 years ago Closed 10 years ago

Manage memory differently for native vs. non-native objects

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: bhackett1024, Assigned: bhackett1024)

References

Details

Attachments

(9 files, 1 obsolete file)

Currently, native and non-native objects have the same layout in memory.  After the shape and type, objects have slots and elements pointers, and storage for fixed slots.  Non-native objects only use the shape and type; they can't use their slots or elements for storage, and while they have fixed slots these are not traced by the GC.

This is pretty inefficient, and will need to be fixed in order to have performant typed objects for bug 1058340.  What I think we need to do is break up the object class hierarchy.  Instead of:

JSObject : ObjectImpl : Cell

           shape
           type
           slots
           elements
           fixed slots

We would have:

NativeObject : JSObject : Cell

slots          shape
elements       type
fixed slots

This would remove ObjectImpl.  I guess this doesn't have to happen but I don't think there's any value in distinguishing between JSObject and ObjectImpl, since the two can be freely cast into one another and having two classes doesn't really address the bloat.  Distinguishing native objects from other objects in the type system would be pretty nice though since there are a lot of methods (both members of JSObject and other functions) that only work on native objects, which we currently manage using assertions.

I don't know if it would be helpful to have a NonNativeObject class inheriting from JSObject as well.

We would still use the existing object allocation kinds for both native and non-native objects, checking whether the object is native during tracing and finalization (which we do already).
I guess, reading the comments on ObjectImpl a bit, I think distinguishing NativeObject from JSObject accomplishes much the same thing that distinguishing ObjectImpl from JSObject is trying to do, since ObjectImpl's intent is to separate out the slot storage and internal organization of the object from its external interface.  Only NativeObject can have slot/element manipulating methods, and now we'll have better enforcement of this property since JSObject* can only be converted to NativeObject* with an explicit downcast.
Are proxies native or non-native for purposes of this discussion?  Because proxies certainly used reserved (and fixed) slots.
(In reply to Boris Zbarsky [:bz] from comment #2)
> Are proxies native or non-native for purposes of this discussion?  Because
> proxies certainly used reserved (and fixed) slots.

Proxies are non-native.  They use reserved slots right now but this is a limitation of the API and a waste of memory.  It would be better if they extended JSObject with more natural C++ style fields, and I'd like to do that as part of this bug.
So for the DOM we currently have proxies with a single reserved slot (for our purposes; we only use the proxy extra slot) which points to the C++ DOM object.  This part is not a waste of memory, and makes the memory layout of all DOM objects, proxy or not, identical, which significantly simplifies dealing with them.
(In reply to Boris Zbarsky [:bz] from comment #4)
> So for the DOM we currently have proxies with a single reserved slot (for
> our purposes; we only use the proxy extra slot) which points to the C++ DOM
> object.  This part is not a waste of memory, and makes the memory layout of
> all DOM objects, proxy or not, identical, which significantly simplifies
> dealing with them.

OK, we should still be able to ensure that DOM proxies have a HeapValue in the same place as it would be in a DOM object.  The layout of proxy objects is mainly contingent on the tracing/finalization code that runs for those objects, which should have more flexibility after bug 966518 lands (which looks like it will be soon, but in any case before that gets fixed up we can just pad and assign slots for all proxies to match their current layout.)
OK.

That will preserve the current behavior.  It still won't allow doing things like storing cached values in slots, which we support on non-proxy DOM objects and have been wanting to support on proxy ones.

How feasible is it to still allow fixed slots on proxies based on JSClass flags?  That shouldn't involve needing to store a pointer to them, since they're right in the object, right?  I guess that would involve conditionals in the slot reads, though, since the offset in the object would be different...
(In reply to Boris Zbarsky [:bz] from comment #6)
> How feasible is it to still allow fixed slots on proxies based on JSClass
> flags?  That shouldn't involve needing to store a pointer to them, since
> they're right in the object, right?  I guess that would involve conditionals
> in the slot reads, though, since the offset in the object would be
> different...

I don't think this would be all that different from what this bug is proposing.  Proxies will have the flexibility to extend JSObject with whatever data they want (as long as it fits in one of the JSObject allocation kinds), and they (all of them, or just DOM proxies) could do this so that they have a fixed length array of HeapValues in the same place they would be in a normal native object.  These heap values would behave the same way as the fixed slots in a proxy do now --- they aren't traced automatically during GC but will have barriers etc. triggered on them --- and since the HeapValues are in the same place for both kinds of objects code can still access objects that might be either DOM proxies or DOM native objects without extra branches.

For sizing proxies and other non-native objects, I don't think tying this to the clasp is a good idea.  Using JSCLASS_RESERVED_SLOTS as we do now is inadequate, internally we have lots of places where we want to size objects differently even if they have the same clasp (plain objects, arrays, array buffers, typed arrays, typed objects).  Our internal allocation functions give control over sizing to whoever is doing the allocation, and we can do this (optionally) in the API as well.
Sounds good.  We can probably handle doing manual tracing of the values in the proxy case, since the tracing code can be per-concrete-object-kind instead of superclasses having to know to trace stuff.
Attached patch WIP (obsolete) — Splinter Review
WIP for the organizational changes required to add NativeObject.  This is a giant monster patch but I don't see a good way of breaking it up significantly that doesn't leave us in a weird state where slot stuff is split between JSObject and NativeObject.

This patch changes ObjectImpl to NativeObject, makes it a subclass of JSObject, moves the slots and elements members and all functions relating to slots/elements storage into NativeObject, and compiles.  This doesn't actually change semantics anywhere though (except to fix a few minor bugs I came across) and uses a fakeNative() function to do an artificial downcast from JSObject to NativeObject when an object might be non-native.  These calls are mostly restricted to non-native object manipulation code and GC stuff.

The next steps here are to move the slots and elements pointers back into JSObject, so that objects once again all have the same representation.  At that point there should be no change in behavior with this patch applied, except for a bunch of new checked downcasts to NativeObject and other types which were only implicit before.

Once this patch gets in, the layout of non-native objects can be changed and the fakeNative() calls removed.  Doing things in this order is better I think because without the organizational changes it's hard to pin down the places where we are accessing the contents of objects that might be non-native.
Assignee: nobody → bhackett1024
Fixed up organizational patch, per comment 9.
Attachment #8497072 - Attachment is obsolete: true
Attachment #8497505 - Flags: review?(luke)
Comment on attachment 8497505 [details] [diff] [review]
organizational patch

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

I really like the way this is all headed.  Also, this split seems much more natural than the JSObject/ObjectImpl split.

I don't know if you planned to do this already right before landing (to avoid rebase pain), but could you rename vm/ObjectImpl* to vm/NativeObject*?

This is half pre-existing, but since this is a monster-reorg patch anyway: do you think we could move all the JSObject/NativeObject/Shape methods back to their respective .cpp/.h's?  They seemed to be pretty interwingled and I've found it frustrating when trying to walk through the code.  (I think there are a few exceptional cases with inline methods that really want to go in the other header.)

::: js/src/builtin/Intl.cpp
@@ +998,5 @@
>      JS_ASSERT(args[0].isObject());
>      JS_ASSERT(args[1].isString());
>      JS_ASSERT(args[2].isString());
>  
> +    RootedNativeObject collator(cx, &args[0].toObject().as<NativeObject>());

How do you know this is a NativeObject?  I see a getClass() test below, so it seems like 'this' could be any object (e.g., via .call).  (Same question in a few cases below.)

::: js/src/jsobj.cpp
@@ +1137,5 @@
>          }
>          return Reject(cx, obj, JSMSG_OBJECT_NOT_EXTENSIBLE, throwError, rval);
>      }
>  
> +    return DefinePropertyOnObject(cx, obj.as<NativeObject>(), id, desc, throwError, rval);

Pre-existing and more of a future question:

I know we've had since forever the invariant that only non-native objects have lookup/define hooks.  I was wondering if there was a nice pinch point to assert the invariant:
  is<NativeObject>() iff (!class->define* && !class->lookup* && ...)

I was also wondering if we could even go farther by segregating the portion of js::Class that held non-native-only hooks and making this a js::NonNativeClass (which derives JS::Class and was engine-internal).  js::NativeObject::getClass() would return a js::Class and js::NonNativeObject (which you were considering if it had any uses) would return a js::NonNativeClass.  With this change, the pattern in all these ops would be: test if we are non-native, if so, obj->as<NonNativeObject>().getClass()->lookupOp(...).

::: js/src/vm/ObjectImpl.h
@@ +1600,5 @@
> +namespace js {
> +
> +// Alternate to JSObject::as<NativeObject>() that tolerates null pointers.
> +inline NativeObject *
> +AsNativeObject(JSObject *obj)

Could you name it MaybeNativeObject?
Attachment #8497505 - Flags: review?(luke) → review+
Could we not name this anything involving "native"?  The term is 15+-year-old nigh-meaningless detritus, from the time when ES(1-?)3 distinguished between "native" objects and host objects.  BasicObject and ProxyObject as a split, or something.  I dunno.  But, if we're touching everything here, *please* not native.
FWIW, I considered this (having also complained about "native" in the past).  But, atm, it seems like there are only two major uses of the word "native": native objects (obj->isNative()) and native functions (fun->isNative()) and I don't find myself confused by this; it's usually clear from the context.  Also, these are meant to be impl terms, not spec terms (they don't highlight a spec distinction), so "native" doesn't seem especially bad.
Keywords: leave-open
Here is a patch to move NativeObject manipulating methods in jsobj files into more appropriate files (including both NativeObject member functions and NativeObject-only baseops and other methods).  This still isn't complete as there are lots of NativeObject methods in Shape.cpp, but that is kind of a longstanding issue and is probably better fixed by factoring that stuff so it can be done as methods on Shape rather than NativeObject.

https://hg.mozilla.org/integration/mozilla-inbound/rev/e210c7768dfd
And a couple more patches (two, since hg patch messed up) to rename the ObjectImpl files to NativeObject:

https://hg.mozilla.org/integration/mozilla-inbound/rev/ebcbf94ef701
https://hg.mozilla.org/integration/mozilla-inbound/rev/3955cb15faf7
Attached patch patchSplinter Review
Patch for the rest of this bug.  This removes all the fakeNative methods and moves slots/elements into NativeObject.  Proxies and typed objects extend their base class with the additional members they want:

Proxies have a pointer to an array of values in the same place as the slots for a normal object, and use the second word in their header for the proxy handler.  So the difference vs. the current representation is that proxies always have a slot vector malloc'ed, rather than usually having fixed slots, though the DOM_OBJECT_SLOT mechanism is mostly unchanged.  There are some spots in codegen that assumed the DOM_OBJECT_SLOT will always be fixed, though I don't know why this is guaranteed to be the case given JSObject::swap.  Incidentally the main reason we can't just put all the data inline in the proxies is because of JSObject::swap, and if there was a better handle on what this method can be used for and the ability to vary layouts between proxies this situation can be improved.

Inline and outline typed objects just store their data following the shape/type.  For outline typed objects the data is stored unboxed, and for both inline and outline typed objects the layout now matches the goal laid out in bug 1058340.

This patch is green on try, and I'll try to split it up for review.

https://tbpl.mozilla.org/?tree=Try&rev=cbaaee84f336
This patch moves slots/elements into NativeObject and fixes most of the remaining uses of fakeNativeSlots etc. for these.  It also fixes some related things like making sure GC allocation kind sizes are correct and removing uses of getPrivate() for non-native objects.
Attachment #8502716 - Flags: review?(luke)
Attached patch new proxy layoutSplinter Review
This patch moves ProxyObject over into using the new layout for its data.
Attachment #8502717 - Flags: review?(wmccloskey)
This kind of goes in tandem with the previous patch, but this has fixes for DOM_OBJECT_SLOT handling with proxies, and also fixes up the JITs for accessing proxies and DOM_OBJECT_SLOT.
Attachment #8502718 - Flags: review?(efaustbmo)
Attachment #8502718 - Flags: review?(bzbarsky)
Currently we support proxy classes adding additional slots to the object.  This facility isn't used anywhere in the browser though and is difficult to support at the moment without individual classes having control of the proxy data layout.  This patch removes these extensible slots and also removes MAX_FIXED_SLOTS from the external API, which wasn't used meaningfully anywhere.
Attachment #8502720 - Flags: review?(wmccloskey)
This moves typed objects over into using a new unboxed layout for their data, and has some fixes for generic array buffer view manipulation code that is affected by this change.
Attachment #8502721 - Flags: review?(sphink)
Attached patch fix tradegutsSplinter Review
TradeGuts is broken by this change, and as is so often the case it was easier to just rewrite this function than try to fix it in place.  At some point ReserveForTradeGuts was made infallible, so this just removes that function and does allocation inline where needed, crashing on OOM.  This simplifies things a lot.
Attachment #8502723 - Flags: review?(wmccloskey)
Don't use the NewObjectCache for non-native objects.  This cache is now a mess and I don't know how much it really buys us anymore, might want to investigate removing it.
Attachment #8502725 - Flags: review?(luke)
Comment on attachment 8502718 [details] [diff] [review]
DOM and JIT proxy fixes

> +++ b/js/src/jit/CodeGenerator.cpp

This seems like we'll output a lot more (and more complicated) code for loading the DOM private.  That seems somewhat unfortunate.  :(

Is that a temporary thing until we add a way to add something like fixed slots to proxies?  Or a new permanent thing?  What is the plan for possibly supporting extra fixed-slot-like things on proxies (e.g. so we can do [StoreInSlot] on them), if any?

Have we tried measuring a microbenchmark with this change to see how much the cost of a simple DOM getter changes?

I didn't carefully read over the exact bit-munging going on in the JIT stuff (e.g. whether the args to branchTest32 make sense here), because I can't figure out what the arguments to some of these masm functions are supposed to be, as usual.  :(

>+++ b/js/src/jsfriendapi.h
>+    if (sobj->slotRef(slot).isMarkable() || value.isMarkable())

Why does the isMarkable() no longer need to be ifdef JSGC_GENERATIONAL?

>+++ b/js/src/jsproxy.h
>+    if (sobj->slotRef(slot).isMarkable() || value.isMarkable())

Likewise.

r=me modulo the above
Attachment #8502718 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] from comment #28)
> Comment on attachment 8502718 [details] [diff] [review]

> Is that a temporary thing until we add a way to add something like fixed
> slots to proxies?  Or a new permanent thing?  What is the plan for possibly
> supporting extra fixed-slot-like things on proxies (e.g. so we can do
> [StoreInSlot] on them), if any?

This is temporary, but won't be clean to fix until we start storing the proxy handler in the clasp.  At that point there won't be any data which the vm requires to be stored in the proxy, and classes/handlers can specify how much space they want inline after the object and can use that space however they want (though they'll have to manage dynamic allocations manually).

This needs to be coupled with some taming of JSObject::swap, so that we can be sure that slots we think are inline definitely are, regardless of any swaps that happen (i.e. We would know how much inline data an object needs and either crash or throw an error when swapping with another object that is too small).

> Have we tried measuring a microbenchmark with this change to see how much
> the cost of a simple DOM getter changes?

Can you post a microbenchmark I can test?

> >+++ b/js/src/jsfriendapi.h
> >+    if (sobj->slotRef(slot).isMarkable() || value.isMarkable())
> 
> Why does the isMarkable() no longer need to be ifdef JSGC_GENERATIONAL?

This is cleanup, removing this optimization that is only in place in builds without ggc.  I can restore this code and add an AssignmentMightNeedBarrier helper though if that would make more sense.
> but won't be clean to fix until we start storing the proxy handler in the clasp.

OK, that seems fine; we've been meaning to do that for a while anyway.  Especially if this lands right after this next uplift instead of right before, which I suspect is what will happen anyway.

> Can you post a microbenchmark I can test?

Sure.  The simplest way to test is probably to remove the [Constant] annotation from nodeType in dom/webidl/Node.webidl (so it doesn't get loop-hoisted), then rebuild in dom/bindings, then run this microbenchmark with and without this patch:

  <script>
    var elem = document.documentElement;
    var count = 10000000;
    var start = performance.now();
    for (var j = 0; j < count; ++j)
      elem.nodeType;
    var end = performance.now();
    document.write((end - start) / count * 1e6);
  </script>

to get average time per get in nanoseconds.

> This is cleanup, removing this optimization that is only in place in builds without ggc.  

Oh, I see, isMarkable() is defined unconditionally.  Yeah, this seems fine.
(In reply to Boris Zbarsky [:bz] from comment #30)
> Sure.  The simplest way to test is probably to remove the [Constant]
> annotation from nodeType in dom/webidl/Node.webidl (so it doesn't get
> loop-hoisted), then rebuild in dom/bindings, then run this microbenchmark
> with and without this patch:
> 
>   <script>
>     var elem = document.documentElement;
>     var count = 10000000;
>     var start = performance.now();
>     for (var j = 0; j < count; ++j)
>       elem.nodeType;
>     var end = performance.now();
>     document.write((end - start) / count * 1e6);
>   </script>
> 
> to get average time per get in nanoseconds.

I bumped the count for more consistent results and tried this benchmark with and without this patch.  Without this patch I get about 5.3 ns per iteration, with this patch I get about 5.8 ns per iteration.  So this has slowed down by about 10%, but we'll be able to get that back without too much work once bug 966518 lands.  Is this OK?
Yeah, that sounds just fine; this is about the worst possible case, so the relative hit would be smaller in other cases.  Thank you for checking that!
Attachment #8502718 - Flags: review?(efaustbmo) → review+
Comment on attachment 8502721 [details] [diff] [review]
typed object and typed array fixes

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

I read through the JIT portions but I don't know enough to review them. Jan, can you take a quick look at the JIT parts?

::: js/src/builtin/TypedObject.cpp
@@ +1590,5 @@
>  
> +    OutlineTypedObject *typedObj = &obj->as<OutlineTypedObject>();
> +
> +    typedObj->setOwnerNoBarrier(nullptr);
> +    typedObj->setData(nullptr);

These separate setters for owner and data seem a little too accident prone. I think I'd prefer

  setOwnerAndDataNoBarrier
  setOwnerAndData
  setData

This one would be setOwnerAndDataNoBarrier

@@ +1608,5 @@
>      if (!buffer.addView(cx, this))
>          CrashAtUnhandlableOOM("TypedObject::attach");
>  
> +    setOwnerNoBarrier(&buffer);
> +    setData(buffer.dataPointer() + offset);

setOwnerAndDataNoBarrier, but move the !isAttached() assert to immediately before to make the NoBarrier part more obvious.

@@ +1633,5 @@
> +
> +        // Trigger a post barrier when attaching an object outside the nursery
> +        // to one that is inside it.
> +        if (!IsInsideNursery(this) && IsInsideNursery(owner))
> +            cx->runtime()->gc.storeBuffer.putWholeCellFromMainThread(this);

setOwnerAndData

::: js/src/builtin/TypedObject.h
@@ +688,5 @@
> +    // barriers are managed directly.
> +    JSObject *owner_;
> +
> +    // Data pointer to some offset in the owner's contents.
> +    uint8_t *data_;

It's so nice to be able to do this, and not force everything to be a Value!

@@ +696,5 @@
> +    uint32_t unsizedLength_;
> +
> +    void setOwnerNoBarrier(JSObject *owner) {
> +        owner_ = owner;
> +    }

Yeah, I'd rather not be able to set the owner without touching data.

@@ +709,5 @@
> +        // Use a larger allocation kind for unsized arrays, to accommodate the
> +        // unsized length.
> +        if (descr->is<UnsizedArrayTypeDescr>())
> +            return gc::FINALIZE_OBJECT2;
> +        return gc::FINALIZE_OBJECT0;

It would be nice to have a better-looking mechanism for this, but never mind for now.

@@ +736,5 @@
> +        return unsizedLength_;
> +    }
> +
> +    void setData(uint8_t *data) {
> +        data_ = data;

Would it be possible to assert that if data is non-null, then it points within owner's memory region?

@@ +795,5 @@
>  // Class for an opaque typed object whose data is allocated inline.
>  class InlineOpaqueTypedObject : public TypedObject
>  {
> +    // Start of the inline data, which immediately follows the shape and type.
> +    uint8_t data[1];

Shouldn't this be data_?

::: js/src/vm/ArrayBufferObject.cpp
@@ +1016,5 @@
>      return as<TypedObject>().typedMem();
>  }
>  
> +void
> +ArrayBufferViewObject::setDataPointer(uint8_t *data)

This is going to collide with my patch to remove ArrayBufferViewObject. But the logic is exactly right. It'll just need to be a static function, I guess.
Attachment #8502721 - Flags: review?(sphink)
Attachment #8502721 - Flags: review?(jdemooij)
Attachment #8502721 - Flags: review+
(In reply to Steve Fink [:sfink] from comment #33)
> @@ +795,5 @@
> >  // Class for an opaque typed object whose data is allocated inline.
> >  class InlineOpaqueTypedObject : public TypedObject
> >  {
> > +    // Start of the inline data, which immediately follows the shape and type.
> > +    uint8_t data[1];
> 
> Shouldn't this be data_?

It's not valid to use a length-one array like this to represent an array of data after some other allocation in C++, I believe.  C++ requires that indexes into any array, don't exceed the array's constant-sized bounds.  This will trigger ubsan violations and similar if written/used this way.
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #34)
> It's not valid to use a length-one array like this to represent an array of
> data after some other allocation in C++, I believe.  C++ requires that
> indexes into any array, don't exceed the array's constant-sized bounds. 
> This will trigger ubsan violations and similar if written/used this way.

There are several places we use length-one arrays in a similar fashion to this without issues, such as in SharedScriptData and TypeScript.  In each of these cases --- including this bug --- the array[1] is a placeholder indicating the start of the array but isn't directly indexed anywhere.  Having the data[1] here just make it clearer what is going wrt how the typed object is laid out and how we compute the start of the object's data (i.e. &data rather than some junky pointer arithmetic.)  FWIW that C++ requirement is pretty out of touch with how heap arrays are used, not just in firefox but in all sorts of systems code.
Comment on attachment 8502716 [details] [diff] [review]
Remove slots/elements from non-native objects

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

::: js/src/jsobj.h
@@ +190,5 @@
> +    // valid for native objects, but during initialization are set for all
> +    // objects. For non-native objects, these must not be dynamically allocated
> +    // pointers which leak when the non-native object finishes initialization.
> +    inline void setInitialSlots(js::HeapSlot *slots);
> +    inline void setInitialElements(js::HeapSlot *elements);

For setInitialElements, there appears to be only one caller which is a JSObject method; so could you remove this method from the public interface and do the static_cast+initialize+comment at the one callsite?

For setInitialSlots, there seem to be a bunch of sites that assign slots that might be dynamically allocated, but I'm assuming that, for non-native objects, the nDynamicSlots = 0 so stored pointer is null?  Is that right?  Anyhow, this is a somewhat dangerous function for such an innocent name; how about giving it a more descriptive (scarier) name or instead making it a member of NativeObject and then having the callsites static_cast to a NativeObject with comments explaining why that's ok?

::: js/src/jsobjinlines.h
@@ +302,5 @@
> +    // objects can't have any fixed slots, for compatibility with
> +    // GetReservedOrProxyPrivateSlot.
> +    MOZ_ASSERT_IF(!type->clasp()->isNative(), JSCLASS_RESERVED_SLOTS(type->clasp()) == 0);
> +    MOZ_ASSERT_IF(!type->clasp()->isNative(), !type->clasp()->hasPrivate());
> +    MOZ_ASSERT_IF(!type->clasp()->isNative(), shape->numFixedSlots() == 0);

Could you also assert !native implies shape->slotSpan() == 0?

@@ +324,4 @@
>  
>      size_t span = shape->slotSpan();
>      if (span)
> +        obj->as<js::NativeObject>().initializeSlotRange(0, span);

Pre-existing, but could you change to "if (size_t span = shape->slotSpan()"?

::: js/src/vm/NativeObject.h
@@ +354,5 @@
> +    /* Slots for object properties. */
> +    js::HeapSlot *slots;
> +
> +    /* Slots for object dense elements. */
> +    js::HeapSlot *elements;

In this (or a follow-up rs=me mass-renaming patch) add _ suffixes to these for symmetry with shape_ and type_ in JSObject?
Attachment #8502716 - Flags: review?(luke) → review+
Comment on attachment 8502725 [details] [diff] [review]
don't use NewObjectCache for non-native objects

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

That does sound like a good idea to investigate if this can be removed in a follow-up.
Attachment #8502725 - Flags: review?(luke) → review+
(In reply to Brian Hackett (:bhackett) from comment #35)
> In each of
> these cases --- including this bug --- the array[1] is a placeholder
> indicating the start of the array but isn't directly indexed anywhere. 
> Having the data[1] here just make it clearer what is going wrt how the typed
> object is laid out and how we compute the start of the object's data (i.e.
> &data rather than some junky pointer arithmetic.)

Ah.  That's fine, then.

> FWIW that C++ requirement
> is pretty out of touch with how heap arrays are used, not just in firefox
> but in all sorts of systems code.

Might be, but we go to war with the language we have, not the language we wish we had.  And the language we have says you can't access stuff through these arrays past their first element.
Comment on attachment 8502721 [details] [diff] [review]
typed object and typed array fixes

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

The Ion changes look good to me.
Attachment #8502721 - Flags: review?(jdemooij) → review+
Attachment #8502720 - Flags: review?(wmccloskey) → review+
Comment on attachment 8502717 [details] [diff] [review]
new proxy layout

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

Cool!

::: js/src/jsgc.cpp
@@ +4375,5 @@
>      return obj->is<CrossCompartmentWrapperObject>() && !IsDeadProxyObject(obj);
>  }
>  
>  /* static */ unsigned
>  ProxyObject::grayLinkSlot(JSObject *obj)

Can you rename this to grayLinkExtraSlot?

::: js/src/jsproxy.h
@@ +347,5 @@
> +// type, the proxy has a ProxyDataLayout structure with a pointer to an array
> +// of values and the proxy's handler. This is designed both so that proxies can
> +// be easily swapped with other objects (via RemapWrapper) and to provide
> +// simple external storage for proxies that mimics the layout used for other
> +// objects.

Can you please add that |values| and |handler| take the same space that |slots| and |elements| take in a NativeObject, so that ProxyObject and NativeObject have the same size?

@@ +360,5 @@
> +GetProxyDataLayout(JSObject *obj)
> +{
> +    MOZ_ASSERT(IsProxy(obj));
> +    return reinterpret_cast<ProxyDataLayout *>(reinterpret_cast<uint8_t *>(obj) +
> +                                               2 * sizeof(void *));

I'd rather we declare this 2*sizeof(void*) number as a global const somewhere and then assert that it's equal to sizeof(JSObject) in some place where both are available.

In addition, I think we should have an assertion that sizeof(ProxyObject) == sizeof(JSObject_Slots0) == sizeof(NativeObject) somewhere.

::: js/src/vm/ProxyObject.cpp
@@ +50,3 @@
>          return nullptr;
>  
> +    RootedObject obj(cx, NewObjectWithGivenProto(cx, clasp, proto, parent, allocKind, newKind));

It seems a little weird that this function calls setInitialElements and setInitialSlots, which cast to a NativeObject*. But it doesn't seem like that would actually cause problems besides confusion. Eventually it would be nice to splits this path a bit more though. Can you please add a comment here explaining that this function will initialize proxy->data to some possibly strange values, but we'll just overwrite them immediately?

@@ +74,5 @@
>  {
> +    Value *vp = &GetProxyDataLayout(this)->values->privateSlot;
> +
> +    // Trigger a barrier before writing the slot.
> +    if (vp->isMarkable() || priv.isMarkable())

This check is supposed to be an optimization to avoid write barriers. It seems particularly useless here, since setCrossCompartmentPrivate will always be called with a markable value. So let's just take the slow path.

In addition, if you make the change I recommend below, we can just do |*slotOfPrivate() = priv|.

::: js/src/vm/ProxyObject.h
@@ +33,3 @@
>      void setSameCompartmentPrivate(const Value &priv);
>  
> +    Value *slotOfPrivate() {

This is kinda scary since it's a really easy way for someone to write to the private and skip write barriers. I don't see any reason why we can't return a HeapValue here though. You'll have to cast, but that seems fine to me. I think the only reason we can't make privateSlot a HeapValue is that it's in a public header.

This should also allow you to removed the uses of Unbarriered marking functions earlier in the patch.

It might be nicer to have a SpiderMonkey-internal version of ProxyDataLayout that does use HeapValue for these fields, but maybe that's too much trouble.

@@ +68,4 @@
>      }
>  
>    private:
> +    Value *slotOfExtra(size_t n) {

I think a HeapValue would be better here too.
Attachment #8502717 - Flags: review?(wmccloskey) → review+
Comment on attachment 8502723 [details] [diff] [review]
fix tradeguts

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

This is a really nice cleanup! Can you please remove the crashOnFailure parameter from SetClassAndProto?

::: js/src/jsobj.cpp
@@ +2352,5 @@
> +
> +    if (hasPrivate())
> +        setPrivate(priv);
> +    else
> +        MOZ_ASSERT(!priv);

This seems a little odd. I get that we want |a| and |b| to both have private pointers or neither. But are private pointers not allowed to be null? I'd rather this were an up-front assertion about |a/b->hasPrivate()|.

@@ +2370,5 @@
> +    initSlotRange(0, values.begin(), values.length());
> +}
> +
> +/* static */void
> +JSObject::TradeGuts(JSContext *cx, JSObject *a, JSObject *b)

Can we just fold this code into JSObject::swap? I don't see any real distinction between them.
Attachment #8502723 - Flags: review?(wmccloskey) → review+
(In reply to Bill McCloskey (:billm) from comment #41)
> This seems a little odd. I get that we want |a| and |b| to both have private
> pointers or neither. But are private pointers not allowed to be null? I'd
> rather this were an up-front assertion about |a/b->hasPrivate()|.

It's fine if one of the objects has a private pointer but the other one doesn't.  For native objects we just saved the original private pointer of the object, and reassign it to the appropriate slot in its new layout after it has been swapped.  The MOZ_ASSERT(!priv) is checking we didn't save some bogus value if the object didn't even have a private pointer.
Add _ suffixes for NativeObject::slots/elements:

https://hg.mozilla.org/integration/mozilla-inbound/rev/8c605fc7e0e1
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Keywords: leave-open
Target Milestone: --- → mozilla36
Depends on: 1089665
Depends on: 1358596
You need to log in before you can comment on or make changes to this bug.