[webkit-reviews] review requested: [Bug 84907] Memory wasted in JSString for non-rope strings : [Attachment 139228] Patch updated from review comments

[bugzilla-daemon at webkit.org]

Michael Saboff <msaboff at apple.com> has asked  for review:
Bug 84907: Memory wasted in JSString for non-rope strings
https://bugs.webkit.org/show_bug.cgi?id=84907

Attachment 139228: Patch updated from review comments
https://bugs.webkit.org/attachment.cgi?id=139228&action=review

------- Additional Comments from Michael Saboff <msaboff at apple.com>
(In reply to comment #13)
> (From update of attachment 139058 [details])
> View in context:
https://bugs.webkit.org/attachment.cgi?id=139058&action=review
> 
> Looks good, but some small tweaks required before landing.
> 
> > Source/JavaScriptCore/runtime/JSString.cpp:63
> > +	 if (thisObject->isJSRopeStringObject())
> > +	     JSRopeString::visitChildren(thisObject, visitor);
> > +}
> > +
> > +void JSRopeString::visitChildren(JSString* thisObject, SlotVisitor&
visitor)
> 
> It's slightly dirty pool to have two classes with distinct GC visit functions
but identical ClassInfos. ClassInfo is supposed to contain a pointer to an
object's visitChildren function. It's also against our convention to have a
subclass "visitChildren" function that never calls its base class's
"visitChildren" function.
> 
> I think a slightly better way to represent this is to fold the rope-style
marking code into JSString::visitChildren. That way, JSString's ClassInfo is
still "true", and JSRopeString is just an implementation detail for the fact
that a JSString may be variable-sized.

Modified static JSRopeString::visitChildren() to JSRopeString::visitFibers().

> > Source/JavaScriptCore/runtime/JSString.cpp:65
> > +	 JSRopeString* thisObjectAsRope =
reinterpret_cast<JSRopeString*>(thisObject);
> 
> static_cast is the C++ cast for casting a base class pointer to a subclass
pointer.

Fixed all cited reinterpret_casts to static_casts.

> > Source/JavaScriptCore/runtime/JSString.h:167
> > +	     bool isRope() const { return m_value.isNull(); }
> > +	     bool is8Bit() const { return m_is8Bit; }
> > +	     bool isJSRopeStringObject() const { return m_isJSRopeStringObject;
}
> 
> I'd rather not have two different ways to ask "Am I a rope?" -- It's hard to
tell which one to use when. Instead, please always use the existing isRope()
function, and remove the new one.

They actually serve two different purposes.  isRope() is really
isUnresolvedRope() so I changed it's name.  When a JSRopeString is created,
isRope() (now isUnresolvedRope()) is true, but when the rope is resolved, it
becomes false.	isJSRopeStringObject() will be true throughout the life of the
JSRopeObject and is used only in JSString::visitChildren() to make sure we
visit the fibers.