[webkit-reviews] review requested: [Bug 10930] WebKit could benefit from a JavaScript live object profiler : [Attachment 32237] adds live object profiling capabilities
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Jul 3 07:09:40 PDT 2009
Horia Olaru <olaru at adobe.com> has asked for review:
Bug 10930: WebKit could benefit from a JavaScript live object profiler
https://bugs.webkit.org/show_bug.cgi?id=10930
Attachment 32237: adds live object profiling capabilities
https://bugs.webkit.org/attachment.cgi?id=32237&action=edit
------- Additional Comments from Horia Olaru <olaru at adobe.com>
(In reply to comment #15)
> (From update of attachment 31800 [review])
> > + 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.
Corrected.
> > +
> > + // 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.
I've added an inheritance based mechanism to measure the cell size and the
object property map size, which includes this case as well.
Please see the modified implementation of JSCell::instanceSize and
JSObject::instanceSize.
> > --- 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.
Fixed. See above.
> > --- 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.
I've decided to rely on JSWrapperObject for the size calculation and removed
this implementation. NumberObject adds nothing more to size.
> > --- 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
Removed. Relying on JSWrapperObject.
> > --- 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.
Removed comment. Render area size is not a relevant mesurement.
> > --- 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.
Was already done, but forgot to remove comment.
> > --- 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.
I've changed the implementation of this function, as per the comment below.
> > + 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.
Thanks for pointing that out.
I've looked around and I think you're referring to
JSObjectRef::JSObjectMakeArray. It would do the job better, but the data
parameter is a JSValueRef array. I would have to iterate over the vector data
to create a new array to pass in. This is inefficient as JSObjectMakeArray does
this again inside (twice).
In light of the above, I've taken the JSObjectMakeArray code and replaced what
I did before.
> > 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?
I've added this API as a temporary solution until the functionality is hooked
into the UI. The current console API does not allow enabling and disabling
profiling from the console. An API only for enabling memory profiling would be
out of place. A function to do this should be added to the layoutTestController
object though.
> What if we always profiled objects when profiling at all?
Oliver Hunt points out in a comment above that:
> most frequently people wish to profile execution
> time rather than object overhead, yet this is likely to add significant time
> and memory pressure that will skew those results in that case.
Is there some statistic on this?
This is the reason I've made a switch to enable or disable memory profiling.
However, the memory profiler at this stage relies on performance profiling for
collecting object allocation traces.
> 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.
>
Additional changes:
- the m_liveObjects member of Profile dies along with the Profile, so I've made
it into an OwnPtr. The LiveObject class no longer needs to extend RefCounted.
- removed the typedefs for iterator and const_iterator and the functions
mapping the HashMap begin and end to LiveObject begin and end. They weren't
really necessary.
- renamed flushLiveObjects to finalizeCollectedData
More information about the webkit-reviews
mailing list