Closed Bug 727306 Opened 11 years ago Closed 11 years ago

GC: specialize post write barrier for object slots


(Core :: JavaScript Engine, defect)

Not set





(Reporter: terrence, Assigned: terrence)




(1 file, 2 obsolete files)

Because we need to re-alloc the object slots pointers for slots growth and actively move it between objects in TradeGuts, we cannot safely store pointers to slots in our write buffer.  Instead we want to store a pair of JSObject* + offset in the write buffer.  This effectively means that we need to specialize the HeapValue::postWriteBarrier used when storing to slots.  The simplest way to do this is to sub-class HeapValue and use this sub-class for object slots.
Attached patch v0 (obsolete) — Splinter Review
This ended up being way less trouble than I thought it would be: there were only two operator= uses to replace.
Attachment #597506 - Flags: review?(wmccloskey)
Comment on attachment 597506 [details] [diff] [review]

Talked to Terrence about this. Looks good with a few revisions.
Attachment #597506 - Flags: review?(wmccloskey)
This is still quite nice.  Making HeapSlot inconvertible basically just expands the quantity of stuff that we need to rename.

The only large refactoring I did was removing InitValueRange.  I would have had to copy and add InitSlotRange and make it take 3 new parameters.  This left one user of InitValueRange and 3 users of the new, uglified InitSlotRange.  Since it's a 2 liner, I just inlined it everywhere and deleted Init*Range.

There also didn't appear to be users of ClearValueRange and DestroyValueRange, so I deleted them as well - did they already get inlined into jsobj in some other patch?  Let me know if I'm missing something, and I'll add them back.
Attachment #597506 - Attachment is obsolete: true
Attachment #599372 - Flags: review?(wmccloskey)
Comment on attachment 599372 [details] [diff] [review]
v1: Made HeapSlot inconvertable with HeapValue.

Review of attachment 599372 [details] [diff] [review]:

This basically looks good, although I'm a little worried that more changes will be necessary after making operator= private.

::: js/src/gc/Barrier-inl.h
@@ +176,5 @@
>  }
> +inline void
> +HeapSlot::init(JSCompartment *comp, const Value &v, const JSObject *obj, uint32_t slotno)
> +{

Please change slotno here as well.

Also, here and in ::set, I think we should be able to assert the following:
  JS_ASSERT(&obj->getSlotRef(slot) == this);

::: js/src/gc/Barrier.h
@@ +369,5 @@
> +/*
> + * Provides specialized heap barriers for object slots.
> + */
> +class HeapSlot

How about having a class EncapsulatedValue that HeapValue and HeapSlot can derive from? It would have the value, and it would also have all the isX methods. This would cut down on the code duplication and it doesn't require templates.

@@ +375,5 @@
> +    Value value;
> +
> +  public:
> +    inline void init(JSCompartment *comp, const Value &v, const JSObject *owner, uint32_t slotno);
> +    inline void set(JSCompartment *comp, const Value &v, const JSObject *owner, uint32_t slotno);

I don't think we need to pass the compartment here, since it's easy to get it from the object. And I think that a better order for the arguments would be (owner, slot, v). Also, jsobj.h seems to use slot as the name, rather than slotno, so I'd rather use that. (Although sometimes it seems to use index.)

::: js/src/jsgcmark.cpp
@@ +1028,5 @@
>       * The function uses explicit goto and implements the scanning of the
>       * object directly. It allows to eliminate the tail recursion and
>       * significantly improve the marking performance, see bug 641025.
>       */
> +    Value *vp, *end;

This is my fault, but I just noticed that we only ever deal with HeapSlots for this function. So can you make these HeapSlot* and then remove the casts?

::: js/src/jsinterp.h
@@ +352,5 @@
>  #endif
>  }
> +static JS_ALWAYS_INLINE void
> +Debug_SetValueRangeToCrashOnTouch(HeapSlot *vec, size_t len)

Probably Debug_SetSlotRangeToCrashOnTouch would be a better name.

::: js/src/jsobj.h
@@ +427,5 @@
>      ObjectElements(uint32_t capacity, uint32_t length)
>          : capacity(capacity), initializedLength(0), length(length)
>      {}
> +    HeapSlot* elements() { return (HeapSlot *)(uintptr_t(this) + sizeof(ObjectElements)); }

The style isn't very consistent in this file, but we try to use |HeapSlot *elements() ...|. Please change this one and fromElements.

::: js/src/jsobjinlines.h
@@ +633,2 @@
>              for (unsigned i = 0; i < count; i++, dst++, src++)
>                  *dst = *src;

Uh oh. I think you probably want to make operator= private for HeapSlot. And these assignments should be converted to sets. Otherwise these assignments won't end up with any barriers at all.

@@ +638,2 @@
>              for (unsigned i = 0; i < count; i++, dst--, src--)
>                  *dst = *src;

Same here.

@@ +1039,3 @@
>  JSObject::initializeSlotRange(size_t start, size_t length)
>  {
> +

Why the extra line?

::: js/src/jsxml.cpp
@@ +7535,5 @@
>  namespace js {
>  bool
> +GlobalObject::getFunctionNamespace(JSContext *cx, HeapSlot *sp)

I don't understand why this needs to change. Why can't it just be a Value*?
Attachment #599372 - Flags: review?(wmccloskey)
Thanks for all the help getting this whipped into shape!

I think this version is looking pretty close to good.  The only thing I'm not entirely sure about is what exact set of methods, overrides, and delete keywords I need and/or should have for the constructor, destructor, and operator= in the EncapsulatedValue class stack.

It appears that these work a bit differently from other methods, in that C++ seems to prefer to create a default instead of calling the base class implementation.  I've "solved" this by putting concrete instances of these methods in all the derived classes (even when identical) and making the base class implementations private and deleted.  One exception is the base constructor and destructor, which appear to require an implementation, even if never called directly. 

All shell tests are passing locally and I've started another try build to test the browser:
Attachment #599372 - Attachment is obsolete: true
Attachment #600534 - Flags: review?(wmccloskey)
Comment on attachment 600534 [details] [diff] [review]
v2: With review feedback.

Review of attachment 600534 [details] [diff] [review]:

Looks good. Just a few small fixes.

::: js/src/gc/Barrier-inl.h
@@ +220,5 @@
> +
> +inline void
> +HeapSlot::set(const JSObject *obj, uint32_t slot, const Value &v)
> +{
> +    JS_ASSERT_IF(!obj->isArray(), &const_cast<JSObject *>(obj)->getSlotRef(slot) == this);

Could you additionally assert that if it is an array, then &obj->getDenseArrayElement(slot) == (const Value *)this ?
And the same in the other ::set method.

@@ +239,5 @@
> +        js::gc::Cell *cell = (js::gc::Cell *)value.toGCThing();
> +        JS_ASSERT(cell->compartment() == comp ||
> +                  cell->compartment() == comp->rt->atomsCompartment);
> +    }
> +#endif

You can remove this #ifdef block and replace it with JS_ASSERT(obj->compartment() == comp).

::: js/src/gc/Barrier.h
@@ +399,5 @@
> +    explicit inline HeapSlot(const JSObject *obj, uint32_t slot, const HeapSlot &v);
> +    inline ~HeapSlot();
> +
> +    inline void init(const JSObject *owner, uint32_t slot, const Value &v);
> +    inline void init(JSCompartment *comp, const JSObject *owner, uint32_t slot, const Value &v);

I should have mentioned this earlier, but can you take out the const specified on the JSObject*? I'm not aware of any other place where we put const on a JSObject*. I don't think there's much benefit to having const here, and it will just cause additional casts and more consts down the road. Please fix this throughout the patch.

::: js/src/jsarrayinlines.h
@@ +66,5 @@
>      if (initlen < index + extra) {
> +        JSCompartment *comp = compartment();
> +        size_t offset = initlen;
> +        for (js::HeapSlot *sp = elements + initlen; sp != elements + (index + extra); sp++)
> +            sp->init(comp, this, offset++, js::MagicValue(JS_ARRAY_HOLE));

Could you increment offset in the for loop, rather than in the init statement? That might be a little clearer.

::: js/src/jsobj.cpp
@@ +3853,4 @@
>  }
>  inline void
>  JSObject::invalidateSlotRange(size_t start, size_t length)

We might as well use getSlotRange here if you feel like changing it.

::: js/src/jsobjinlines.h
@@ +595,5 @@
>  {
>      JS_ASSERT(dstStart + count <= getDenseArrayCapacity());
>      JSCompartment *comp = compartment();
>      for (unsigned i = 0; i < count; ++i)
> +        elements[dstStart + i].set(comp, this, i, src[i]);

Shouldn't this be dstStart+i as the argument to set?

@@ +604,5 @@
>  {
>      JS_ASSERT(dstStart + count <= getDenseArrayCapacity());
>      JSCompartment *comp = compartment();
>      for (unsigned i = 0; i < count; ++i)
> +        elements[dstStart + i].init(comp, this, i, src[i]);

Same here.

@@ +634,2 @@
>              for (unsigned i = 0; i < count; i++, dst++, src++)
> +                dst->set(comp, this, dstStart + i, *src);

It's probably safer to do this:
  dst->set(comp, this, dst - elements, *src);

@@ +639,2 @@
>              for (unsigned i = 0; i < count; i++, dst--, src--)
> +                dst->set(comp, this, dstStart + count - 1 - i, *src);

...and the same should work here.

::: js/src/jswrapper.cpp
@@ +875,1 @@
>                                "wrappedObject");

The spacing here is slightly off.
Attachment #600534 - Flags: review?(wmccloskey) → review+
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in before you can comment on or make changes to this bug.