673451 - Refactoring to ease the way for write barriers

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: patch

This patch makes a few small changes:
- It makes JSObject::slots private.
- It adds some const annotations on js::Value, mostly for the result of JSObject::getSlots().
- It removes some gratuitous uses of getSlotRef and replaces them with getSlot/setSlot.

This patch is a prerequisite for write barriers. I'm filing it as a separate bug since it might be nice to land it earlier.

Just to be safe, I checked for performance regressions and I saw none.

Comment 1

[Chris Leary [:cdleary] (not checking bugmail)]

Comment on attachment 547731 [details] [diff] [review]
patch

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

A few nits as revenge for not getting the cool patch in the stack. ;-)

::: js/src/jsobj.cpp
@@ +4341,2 @@
>              last = vref.toPrivateUint32();
> +            setSlot(*slotp, UndefinedValue());

Nit: can we spring for a temporary here to be a little more explicit?

::: js/src/jsobj.h
@@ +395,3 @@
>      js::Value   *slots;                     /* dynamically allocated slots,
>                                                 or pointer to fixedSlots() */
> +public:

Label style is half indent, here and above.

@@ +650,5 @@
> +    js::Value *getSlotsPtr() { return slots; }
> +    void setSlotsPtr(js::Value *vp) { slots = vp; }
> +
> +    void copySlots(uint32 dstStart, const js::Value *src, uint32 count) {
> +        memcpy(slots + dstStart, src, count * sizeof(js::Value));

Can we assert this range is within capacity?

@@ +654,5 @@
> +        memcpy(slots + dstStart, src, count * sizeof(js::Value));
> +    }
> +
> +    void setSlotsUndefined(uint32 start, uint32 count) {
> +        js::SetValueRangeToUndefined(slots + start, count);

Ditto.

::: js/src/jsobjinlines.h
@@ +359,5 @@
>      JS_ASSERT(isDenseArray());
>      return numSlots();
>  }
>  
> +inline const js::Value*

Space after typename before star while you're in there.

@@ +391,5 @@
> +inline void
> +JSObject::moveDenseArrayElements(uintN dstStart, uintN srcStart, uintN count)
> +{
> +    JS_ASSERT(isDenseArray());
> +    memmove(slots + dstStart, getSlots() + srcStart, count * sizeof(js::Value));

Again, can we assert that we're in capacity?

@@ +423,5 @@
>  JSObject::setCallObjCallee(JSObject *callee)
>  {
>      JS_ASSERT(isCall());
>      JS_ASSERT_IF(callee, callee->isFunction());
> +    setSlot(JSSLOT_CALL_CALLEE, js::ObjectOrNullValue(callee));

Whoa, there used to be a return in this inline void function?

::: js/src/jstracer.cpp
@@ +3836,5 @@
>  {
>      JS_ASSERT(slot == uint16(slot));
>      JS_ASSERT(globalObj->numSlots() <= MAX_GLOBAL_SLOTS);
>  
> +    const Value* vp = &globalObj->getSlot(slot);

It look like this whole file changed style away from SpiderMonkey's space-before-star. Lame.

@@ +14324,1 @@
>  	JS_ASSERT(sizeof(Value) == 8); // The |3| in the following statement requires this.

Can you kill the hard tab while you're here?

Comment 2

[(no longer active)]

Backed out of inbound because of build bustage.

Comment 3

[Nicholas Nethercote [inactive]]

(In reply to comment #0)
>
> - It makes JSObject::slots private.

Woo!  That means I can close bug 555429, bug 558262, bug 564094 and bug 565845.  Thanks :)

Comment 4

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/ed434f4c233e