[webkit-reviews] review denied: [Bug 10930] WebKit could benefit from a JavaScript live object profiler : [Attachment 31800] Adds live object profiling capabilities

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Jun 25 10:53:03 PDT 2009


Kevin McCullough <kmccullough at apple.com> has denied Horia Olaru
<olaru at adobe.com>'s request for review:
Bug 10930: WebKit could benefit from a JavaScript live object profiler
https://bugs.webkit.org/show_bug.cgi?id=10930

Attachment 31800: Adds live object profiling capabilities
https://bugs.webkit.org/attachment.cgi?id=31800&action=review

------- Additional Comments from Kevin McCullough <kmccullough at apple.com>

> Index: JavaScriptCore/runtime/JSArray.cpp
> ===================================================================
> --- JavaScriptCore/runtime/JSArray.cpp	(revision 45079)
> +++ JavaScriptCore/runtime/JSArray.cpp	(working copy)
> @@ -1048,6 +1048,35 @@ void JSArray::checkConsistency(Consisten
>  
>  #endif
>  
> +size_t JSArray::instanceSize() const
> +{
> +    size_t computedSize = 0;
> +
> +    // JS class overhead
> +    computedSize += sizeof(*this);
> +
> +    // ArrayStorage overhead - this takes into account the size of the
> +    // storage vector
> +    computedSize += storageSize(m_storage->m_vectorLength);
> +
> +    //  size of Sparse value map
> +    //  FIXME: A generic class/method could probably be implemented
> +    //  to measure the size of any HashMap containing JSObjects
> +    SparseArrayValueMap* map = m_storage->m_sparseValueMap;
> +    if (map) {
> +	   computedSize += sizeof(*map) + map->capacity() *
sizeof(SparseArrayValueMap::ValueType);
> +    }

We don't put curly brackets around a single line of code.

> +
> +    //  FIXME: Measure size of the JSObject propertyMap
> +    //  JSArray will store objects with non-numeric identifiers, and some
other types
> +    //  (see the comments at the beginning of JSArray.cpp) in its property
map
> +    //  (eg: arr["object"]), so it is important to measure this.
> +    //  This should probably be done in JSObject and a 'super' method should
be
> +    //  called.

Again if this is important we should do this.  And seeing how the only tests we
don't have
are for object size, this is a natural place for bugs to hide.

> +
> +    return computedSize;
> +}
> +
>  JSArray* constructEmptyArray(ExecState* exec)
>  {
>      return new (exec)
JSArray(exec->lexicalGlobalObject()->arrayStructure());
> Index: JavaScriptCore/runtime/JSString.cpp
> ===================================================================
> --- JavaScriptCore/runtime/JSString.cpp	(revision 45079)
> +++ JavaScriptCore/runtime/JSString.cpp	(working copy)
> @@ -112,6 +112,16 @@ bool JSString::getOwnPropertySlot(ExecSt
>      return JSString::getOwnPropertySlot(exec, Identifier::from(exec,
propertyName), slot);
>  }
>  
> +size_t JSString::instanceSize() const
> +{
> +    size_t computedSize = sizeof(*this);
> +
> +    computedSize += value().size() * sizeof(UChar) + value().cost();
> +//  FIXME: Add structure size

See comment above.

> +
> +    return computedSize;
> +}
> +
>  JSString* jsString(JSGlobalData* globalData, const UString& s)
>  {
>      int size = s.size();
> Index: JavaScriptCore/runtime/NumberObject.cpp
> ===================================================================
> --- JavaScriptCore/runtime/NumberObject.cpp	(revision 45079)
> +++ JavaScriptCore/runtime/NumberObject.cpp	(working copy)
> @@ -41,6 +41,13 @@ JSValue NumberObject::getJSNumber()
>      return internalValue();
>  }
>  
> +size_t NumberObject::instanceSize() const
> +{
> +//  FIXME: Aside from this, we should probably measure the size of the
> +//  structure/property map of an object - should probably be done higer up

Ditto.

> +    return sizeof(*this);
> +}
> +
>  NumberObject* constructNumber(ExecState* exec, JSValue number)
>  {
>      NumberObject* object = new (exec)
NumberObject(exec->lexicalGlobalObject()->numberObjectStructure());
> Index: JavaScriptCore/runtime/StringObject.cpp
> ===================================================================
> --- JavaScriptCore/runtime/StringObject.cpp	(revision 45079)
> +++ JavaScriptCore/runtime/StringObject.cpp	(working copy)
> @@ -98,4 +98,18 @@ JSString* StringObject::toThisJSString(E
>      return internalValue();
>  }
>  
> +size_t StringObject::instanceSize() const
> +{
> +    size_t computedSize = sizeof(*this);
> +
> +//  Add size of the contained JSString object

Ditto

> +    CollectorCell* cell =
reinterpret_cast<CollectorCell*>(asCell(JSWrapperObject::internalValue()));
> +    if (cell->u.freeCell.zeroIfFree)
> +	   computedSize += internalValue()->instanceSize();
> +
> +//  FIXME: Add structure size (if this object has a Structure member)

Ditto

> +
> +    return computedSize;
> +}
> +
>  } // namespace JSC
> Index: WebCore/html/HTMLImageElement.cpp
> ===================================================================
> --- WebCore/html/HTMLImageElement.cpp (revision 45079)
> +++ WebCore/html/HTMLImageElement.cpp (working copy)
> @@ -40,6 +40,17 @@ namespace WebCore {
>  
>  using namespace HTMLNames;
>  
> +size_t HTMLImageElement::elementSize() const
> +{
> +    size_t computedSize = sizeof(*this);
> +
> +    //should we compute and add the render area size?

You should try to find an answer to this question, but my suspicion is the
answer is no.  Geoff or Sam would know better than I.

> +    if(cachedImage() && cachedImage()->image()->data())
> +	   computedSize += cachedImage()->image()->data()->size();
> +
> +    return computedSize;
> +}
> +
>  HTMLImageElement::HTMLImageElement(const QualifiedName& tagName, Document*
doc, HTMLFormElement* form)
>      : HTMLElement(tagName, doc)
>      , m_imageLoader(this)
> Index: WebCore/inspector/JavaScriptObjectIdentifier.cpp
> ===================================================================
> --- WebCore/inspector/JavaScriptObjectIdentifier.cpp	(revision 0)
> +++ WebCore/inspector/JavaScriptObjectIdentifier.cpp	(revision 0)
> +JSValue toJS(ExecState* exec, ObjectIdentifier* objectInfo)
> +{
> +    if (!objectInfo)
> +	   return jsNull();
> +	   
> +    JSObject* objectInfoWrapper = objectIdentifierCache().get(objectInfo);
> +    if (objectInfoWrapper)
> +	   return objectInfoWrapper;
> +
> +    objectInfo->ref();
> +
> +    //FIXME: may need to cache the transformed JS objects.

Same comment.

> +    objectInfoWrapper = toJS(JSObjectMake(toRef(exec),
ObjectIdentifierClass(), static_cast<void*>(objectInfo)));
> +    objectIdentifierCache().set(objectInfo, objectInfoWrapper);
> +    return objectInfoWrapper;
> +}
> +
> +}
> Index: WebCore/inspector/JavaScriptProfile.cpp
> ===================================================================
> --- WebCore/inspector/JavaScriptProfile.cpp	(revision 45079)
> +++ WebCore/inspector/JavaScriptProfile.cpp	(working copy)
> @@ -131,6 +136,53 @@ static JSValueRef restoreAll(JSContextRe
>      return JSValueMakeUndefined(ctx);
>  }
>  
> +static JSValueRef getLiveObjects(JSContextRef ctx, JSObjectRef thisObject,
JSStringRef, JSValueRef* exception)
> +{
> +    Profile* profile =
static_cast<Profile*>(JSObjectGetPrivate(thisObject));
> +    if (!profile)
> +	   return JSValueMakeUndefined(ctx);
> +
> +    LiveObjects::ObjectVector objects =
profile->liveObjects()->cachedObjects();
> +
> +    JSObjectRef global = JSContextGetGlobalObject(ctx);
> +
> +

Extra whitespace.  You could actually probably move several of these lines of
code together.

> +    JSRetainPtr<JSStringRef> arrayString(Adopt,
JSStringCreateWithUTF8CString("Array"));
> +
> +    JSValueRef arrayProperty = JSObjectGetProperty(ctx, global,
arrayString.get(), exception);
> +    if (exception && *exception)
> +	   return JSValueMakeUndefined(ctx);
> +
> +    JSObjectRef arrayConstructor = JSValueToObject(ctx, arrayProperty,
exception);
> +    if (exception && *exception)
> +	   return JSValueMakeUndefined(ctx);
> +
> +    JSObjectRef result = JSObjectCallAsConstructor(ctx, arrayConstructor, 0,
0, exception);
> +    if (exception && *exception)
> +	   return JSValueMakeUndefined(ctx);
> +
> +    JSRetainPtr<JSStringRef> pushString(Adopt,
JSStringCreateWithUTF8CString("push"));
> +
> +    JSValueRef pushProperty = JSObjectGetProperty(ctx, result,
pushString.get(), exception);
> +    if (exception && *exception)
> +	   return JSValueMakeUndefined(ctx);
> +
> +    JSObjectRef pushFunction = JSValueToObject(ctx, pushProperty,
exception);
> +    if (exception && *exception)
> +	   return JSValueMakeUndefined(ctx);
> +
> +    for (size_t i = 0; i < objects.size(); ++i) {
> +	   ExecState* exec = toJS(ctx);
> +	   JSValueRef obj = toRef(exec, toJS(exec, objects[i].get()));
> +
> +	   JSObjectCallAsFunction(ctx, pushFunction, result, 1, &obj,
exception);
> +	   if (exception && *exception)
> +	       return JSValueMakeUndefined(ctx);
> +    }

I think someone added a function to do this somewhere.	All you have to do is
pass in your objects and it will return the array with the objects in the
array.
Again ask Sam he may know where this function is.

> +
> +    return result;
> +}
> +
>  static void finalize(JSObjectRef object)
>  {
>      Profile* profile = static_cast<Profile*>(JSObjectGetPrivate(object));
> Index: WebCore/page/Console.cpp
> ===================================================================
> --- WebCore/page/Console.cpp	(revision 45079)
> +++ WebCore/page/Console.cpp	(working copy)
> @@ -313,6 +313,19 @@ void Console::profileEnd(const JSC::UStr
>      }
>  }
>  
> +void Console::setMemoryProfilingEnabled(bool isEnabled)

We should definitely discuss this as a team. Is this the API we want to expose
for this feature?
Is this the right name for exposing this functionality?
What if we always profiled objects when profiling at all?
I would like to see some more discussion on this before we make a decision that
affect so many developers.


Overall I think you are almost there with this patch.  As you can see I'm very
resistant to checking in new features where I think
core functionality is substituted with FIXMEs.	You should try to answer all
the open questions you have before landing this.

It's good that we have some tests now since there is no UI yet, and so these
tests are the only way we have to investigate if this
is working correctly, and helps keep other developers from breaking a new
feature.

Once this lands you should coordinate with Tim for a way to get some UI for
this into the Profile's panel.


More information about the webkit-reviews mailing list