[webkit-reviews] review denied: [Bug 84907] Memory wasted in JSString for non-rope strings : [Attachment 139058] Another Windows build fix - added new symbol to exports
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Apr 26 18:24:23 PDT 2012
Geoffrey Garen <ggaren at apple.com> has denied Michael Saboff
<msaboff at apple.com>'s request for review:
Bug 84907: Memory wasted in JSString for non-rope strings
https://bugs.webkit.org/show_bug.cgi?id=84907
Attachment 139058: Another Windows build fix - added new symbol to exports
https://bugs.webkit.org/attachment.cgi?id=139058&action=review
------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
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.
> 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.
> Source/JavaScriptCore/runtime/JSString.cpp:157
> + JSRopeString* currentFiberAsRope =
reinterpret_cast<JSRopeString*>(currentFiber);
static_cast is the C++ cast for casting a base class pointer to a subclass
pointer.
> Source/JavaScriptCore/runtime/JSString.cpp:186
> + JSRopeString* currentFiberAsRope =
reinterpret_cast<JSRopeString*>(currentFiber);
static_cast is the C++ cast for casting a base class pointer to a subclass
pointer.
> 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.
> Source/JavaScriptCore/runtime/JSString.h:349
> + reinterpret_cast<const JSRopeString*>(this)->resolveRope(exec);
static_cast is the C++ cast for casting a base class pointer to a subclass
pointer.
> Source/JavaScriptCore/runtime/JSString.h:356
> + reinterpret_cast<const JSRopeString*>(this)->resolveRope(0);
static_cast is the C++ cast for casting a base class pointer to a subclass
pointer.
> Source/JavaScriptCore/runtime/JSString.h:364
> + return
reinterpret_cast<JSRopeString*>(this)->getIndexSlowCase(exec, i);
static_cast is the C++ cast for casting a base class pointer to a subclass
pointer.
More information about the webkit-reviews
mailing list