Last Comment Bug 673451 - Refactoring to ease the way for write barriers
: Refactoring to ease the way for write barriers
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal with 1 vote (vote)
: mozilla8
Assigned To: Bill McCloskey (:billm)
:
Mentors:
Depends on:
Blocks: 641027
  Show dependency treegraph
 
Reported: 2011-07-22 10:11 PDT by Bill McCloskey (:billm)
Modified: 2011-07-28 08:14 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (89.10 KB, patch)
2011-07-22 10:11 PDT, Bill McCloskey (:billm)
cdleary: review+
Details | Diff | Splinter Review

Description Bill McCloskey (:billm) 2011-07-22 10:11:12 PDT
Created attachment 547731 [details] [diff] [review]
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) 2011-07-22 15:33:44 PDT
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 :Ehsan Akhgari 2011-07-26 11:43:05 PDT
Backed out of inbound because of build bustage.
Comment 3 Nicholas Nethercote [:njn] 2011-07-27 22:02:32 PDT
(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 :)

Note You need to log in before you can comment on or make changes to this bug.