Bug 10930 – WebKit could benefit from a JavaScript live object profiler

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Rowe (bdash)]

With AJAX applications becoming more common, users are spending considerably longer times on the same page.  If their JavaScript code is poorly written, they may end up creating a large number of live but unused JavaScript objects that remain in memory.  It could be useful to be able to view the number and types of live objects to determine if objects are hanging around longer than anticipated, so that increases in memory footprint can be limited.

[David Kilzer (:ddkilzer)]

<rdar://problem/5762057>

[Kevin McCullough]

Committed Revision 34065.

[Kevin McCullough]

Hmmm I may have closed this prematurely.  Does the current WebInspector satisfy this?

[Mark Rowe (bdash)]

Nope.  This is about an object profiler, rather than the current execution time profiler.

[Mark Rowe (bdash)]

*** Bug 19485 has been marked as a duplicate of this bug. ***

[Kevin McCullough]

*** Bug 17241 has been marked as a duplicate of this bug. ***

[Horia Olaru]

Created attachment 29915 [details]
Adds live object profiling capabilities

The patch provides a way to access the size of a specific JavaScript object, where it was created and a way to expose this information in WebInspector/JavaScript.

The patch follows the general guidelines in this related discussion on webkit-dev (https://lists.webkit.org/pipermail/webkit-dev/2009-February/006554.html).

The size of an object

This issue is comprised of two parts: the way to access the size of a JS object per se, and the way to access the size of DOM objects in JS.

The idea was to add a method to JS objects that returns the size of the object it belongs to. To do this, a virtual method named ‘instanceSize’ was added to the implementation of JS objects. Since not all implementations inherit JSObject, but all inherit JSCell (e.g. JSString), instanceSize was added on JSCell. It returns size 0 and is inherited by all descendants of JSCell. This way, it can be implemented and updated only for specific types. Such implementations are included for a few basic types (Array, String, Number).

To access DOM object sizes, CodeGeneratorJS.pm was modified. This script generates JS wrappers for DOM objects. The script checks for the ‘HasInstanceSize’ attribute in the .idl file to see if it is to add an ‘instanceSize’ implementation to the JS wrapper. This maintains the size function naming convention across pure JS objects and DOM objects, while allowing the size function to reside within the implementation file of the DOM object. This DOM object size function was named ‘elementSize’ and it is added on HTMLImageElement, to prove functionality. 

Object creation information

The main purpose is to keep track of the location where an object is created in JavaScript. This includes the url/file name and line number, as well as a stack trace of the function in which the object is created. In order to access this information, I found it necessary to record it at object creation time, when url, line number, and creator function are known. To do this a structure (LiveObjects) was added to hold the information in the existing profiler infrastructure. Profiler now exposes the objectCreated and objectCollected methods. The objectCreated method is called from the JSCell operator new, when profiling is enabled. Similarly, objectCollected is called from the JSCell destructor. The (LiveObjects) structure holds a map of JSCell* and ProfilerNode*, registering objects allocated between the start and end of a profiling session. The existing ProfilerNode structure was used in order to access stack traces, as the current ProfilerNode at object creation time is the currently executing JS function.

Object information access from JavaScript/WebInspector.

To access the collected live object information, the objectData() function was added to the JS Profile object. This function returns an array of objects (of type ObjectInformation). The JS interface for the ObjectInfromation type is located in inspector/JavaScriptObjectInformation and exposes these properties: id, type name, size and createdFrom. The createdFrom object is a reference to a JS ProfileNode, which actually holds function information. Stack traces can be obtained from the createFrom object by recursively going up the .parent property.

The below console code will allow you to visualize the live object information. Note that profiling must be enabled from the WebInspector in order for the code to work.

console.profile(“MyProfile”);
console.profileEnd();
// find the ‘i’ for your profile here;
console.profiles[i].objectData();

[Oliver Hunt]

Comment on attachment 29915 [details]
Adds live object profiling capabilities

The most obvious issue with this patch is that the change to JSCell::operator new is likely to butcher performance whether the profiler is enabled or not -- have you tested perf in sunspider with the profiler disabled?

Additionally i think it would be best if the api added a different method to profiler object creation as 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.

[Horia Olaru]

Here are the sunspider results:
** TOTAL **:           *1.007x as slow*  852.3ms +/- 0.1%   858.3ms +/- 0.1%     significant
http://pastie.org/489146

Adding UNLIKELY macros in the checks inside the operator new statements, shaved off about 2ms:
** TOTAL **:           *1.005x as slow*  852.3ms +/- 0.1%   856.1ms +/- 0.1%     significant
http://pastie.org/489148

Is this acceptable?

I'm not sure I understand the second part of your comment. Do you mean to say there should be a way to enable performance profiling without enabling memory profiling?
If that is the case, I will include this in the next version of the patch.

[Horia Olaru]

Created attachment 30834 [details]
Adds live object profiling capabilities

This version of the patch no longer introduces significant performance regressions.
Sunspider results are:
http://pastie.org/496608

** TOTAL **:           -                 731.9ms +/- 0.1%   731.2ms +/- 0.1% 

The live object profiling is disabled by default when profiling is enabled. An explicit action must be performed by the user to enabled it. I have added a temporary function on the console object that enables or disables live memory profiling, until this functionality is hooked into the UI. Note that profiling still needs to be enabled before enabling live object profiling.

[Horia Olaru]

Created attachment 31295 [details]
Adds live object profiling capabilities

This new attachment is just an up to date version of the previous.

[Geoffrey Garen]

> m_doCollectAllocationTraces

"m_" is the prefix we use for data members. "s_" is for statics.

Can you test sunspider --v8 as well? It tends to allocate more objects.

I think Kevin is reviewing this more in-depth. I don't see any obvious problems here.

[Kevin McCullough]

Comment on attachment 31295 [details]
Adds live object profiling capabilities

> Index: JavaScriptCore/profiler/LiveObjects.cpp
> ===================================================================
> --- JavaScriptCore/profiler/LiveObjects.cpp	(revision 0)
> +++ JavaScriptCore/profiler/LiveObjects.cpp	(revision 0)
> +
> +LiveObjects::~LiveObjects()
> +{
> +    //FIXME: Should implement this
> +    //do we need to manually empty the HashMap? Find out.

This seems like a big deal. You should know the answer to this before we land.

> +}
> +
> +//returns true if new value was added (not existing)
> +bool LiveObjects::addNode(const JSCell* cell, PassRefPtr<ProfileNode> stackNode)
> +{
> +    ASSERT(cell);
> +    //FIXME: should this increase/adopt the reference to the stackNode?
> +    //if so, then it should dereference it in the removeNode methods

Ditto.
Also are we sure the JSCell won't be garbage collected?

> +    return (m_stackMap.add(cell, stackNode)).second;
> +}
> +
> +
> +void LiveObjects::toVector(Vector<ObjectIdentifier* >& vector) const
> +{
> +    ASSERT(vector.isEmpty());
> +    for (const_iterator it = begin(); it != end(); ++it) {
> +        if (!isFree(it->first)) {
> +            const JSCell* cell = it->first;
> +            const ClassInfo* info = cell->classInfo();
> +            ObjectIdentifier* obj = new ObjectIdentifier((unsigned)cell, info ? info->className : "Object", cell->instanceSize(), it->second);

Why don't you just make the ObjectIdentifier constructor accept the JSCell pointer and handle this logic in the constructor?  I could go either way on this one.

> +                vector.append(obj);
> +        }
> +        //FIXME: there should be an else branch here to remove the
> +        //free cells, which are not needed. They should be removed from the map
> +        //Can't remove items in a HashMap while iterating over it. Should
> +        //store the keys in an array and remove them afterwards.

Again, if this is important then we should do it.

> +    }
> +}
> +
> +} // namespace JSC

> Index: JavaScriptCore/profiler/Profiler.cpp
> ===================================================================
> --- JavaScriptCore/profiler/Profiler.cpp	(revision 44682)
> +++ JavaScriptCore/profiler/Profiler.cpp	(working copy)
> @@ -86,8 +98,11 @@ PassRefPtr<Profile> Profiler::stopProfil
>              RefPtr<Profile> returnProfile = profileGenerator->profile();
>  
>              m_currentProfiles.remove(i);
> -            if (!m_currentProfiles.size())
> +            if (!m_currentProfiles.size()) {
> +                //There is no need to check whether m_isMemoryProfiling is enabled

Why? Is the assumption that setDoCollectAllocationTraces is cheap and will always be so? 

> +                JSCell::setDoCollectAllocationTraces(false);
>                  s_sharedEnabledProfilerReference = 0;
> +            }
>              
>              return returnProfile;
>          }
> @@ -104,6 +119,14 @@ static inline void dispatchFunctionToPro
>      }
>  }
>  
> +static inline void dispatchObjectToProfiles(const Vector<RefPtr<ProfileGenerator> >& profiles, ProfileGenerator::ProfileObjectFunction function, const JSCell* cell, unsigned currentProfileTargetGroup)
> +{
> +    for (size_t i = 0; i < profiles.size(); ++i) {
> +        if (profiles[i]->profileGroup() == currentProfileTargetGroup || !profiles[i]->originatingGlobalExec())
> +            (profiles[i].get()->*function)(cell);
> +    }
> +}
> +
>  void Profiler::willExecute(ExecState* exec, JSValue function)
>  {
>      ASSERT(!m_currentProfiles.isEmpty());
> @@ -134,6 +157,24 @@ void Profiler::didExecute(ExecState* exe
>      dispatchFunctionToProfiles(m_currentProfiles, &ProfileGenerator::didExecute, createCallIdentifier(&exec->globalData(), JSValue(), sourceURL, startingLineNumber), exec->lexicalGlobalObject()->profileGroup());
>  }
>  
> +void Profiler::objectCreated(ExecState* exec, JSCell* createdObject)
> +{
> +    ASSERT(!m_currentProfiles.isEmpty());
> +
> +    dispatchObjectToProfiles(m_currentProfiles, &ProfileGenerator::objectCreated, createdObject, exec->lexicalGlobalObject()->profileGroup());

Since this is the only caller to dispatchObjectToProfiles you don't really need to pass the m_currentProfiles, but I guess it looks like you were intending this to have more callers with different functions passed to it?

> +}
> +


This is as far as I got with this today.  I'll try to look at the rest of it, or a new version of it, later.
One thing that I think would be good would be a way to test this.  I realize that there is a lot of functionality in the Web Inspector that does not have automated testing,
and that I am one of the culprits of the Inspector being in that state, but I don't want to continue this trend of expanding Web Inspector functionality without a way to
implement automated tests when bugs are fixed.

I'm not sure the automated tests are necessary for this patch, but it's something I'd love to hear feedback on from the rest of the community.

[Horia Olaru]

Created attachment 31800 [details]
Adds live object profiling capabilities

Thanks for the comments.

@Geoffrey

> "m_" is the prefix we use for data members. "s_" is for statics.

Changed it to s_doCollectAllocationTraces. Thanks.

> Can you test sunspider --v8 as well? It tends to allocate more objects.

** TOTAL **:      *1.005x as slow*  3150.0ms +/- 0.1%   3164.2ms +/- 0.1%     significant

http://pastie.org/523206

This is with this version of the patch. What are your thoughts on this?



@Kevin

> > --- JavaScriptCore/profiler/LiveObjects.cpp	(revision 0)
> > +++ JavaScriptCore/profiler/LiveObjects.cpp	(revision 0)
> > +
> > +LiveObjects::~LiveObjects()
> > +{
> > +    //FIXME: Should implement this
> > +    //do we need to manually empty the HashMap? Find out.
> 
> This seems like a big deal. You should know the answer to this before we land.

Emptying the HashMap and destroying its objects is done when the map is destroyed.
There is no need to do it manually. Comment removed.

> > +bool LiveObjects::addNode(const JSCell* cell, PassRefPtr<ProfileNode> stackNode)
> > +{
> > +    ASSERT(cell);
> > +    //FIXME: should this increase/adopt the reference to the stackNode?
> > +    //if so, then it should dereference it in the removeNode methods
> 
> Ditto.

The hashmap value, to which the stackNode is assigned, is a RefPtr and does not need
to be manually managed. Comment removed.

> Also are we sure the JSCell won't be garbage collected?

I'm going on the assumption that the main use case for a memory profiler is to find leaks. In this case,
the developer wants to see the live objects and collected objects are not so important since they don't leak.

The isFree(cell) method returns true for a cell that has been collected and such cells are skipped.

The addNode and removeNode methods of LiveObjects are called from the above objectCreated
and objectCollected entry points. The objectCreated method is called at the end of the new operator
and adds a cell to the map. The objectCollected method is called when the cell is collected,
becoming free, and it removes the object from the map. This mechanism ensures that during a profiling
session there are no collected cells inside the liveObects map.

However, when profiling is stopped, the hooks inside the heap are disconnected from this profile.
Cells that are live at this time may become free in the future. To avoid loosing this information,
I have added a method that flushes the cell map to a vector of ObjectIdentifiers when profiling is stopped.
This vector then becomes the data that will be displayed by the JavaScript Profile object.

> > +void LiveObjects::toVector(Vector<ObjectIdentifier* >& vector) const
> > +{
> > +    ASSERT(vector.isEmpty());
> > +    for (const_iterator it = begin(); it != end(); ++it) {
> > +        if (!isFree(it->first)) {
> > +            const JSCell* cell = it->first;
> > +            const ClassInfo* info = cell->classInfo();
> > +            ObjectIdentifier* obj = new ObjectIdentifier((unsigned)cell, info ? info->className : "Object", cell->instanceSize(), it->second);
> 
> Why don't you just make the ObjectIdentifier constructor accept the JSCell
> pointer and handle this logic in the constructor?  I could go either way on
> this one.

Initially I didn't want to add any logic to the ObjectIdentifier struct.
Now, in order to avoid memory issues, I've changed ObjectIdentifier to a RefCounted type and
moved some of the logic in the constructors.

> > +                vector.append(obj);
> > +        }
> > +        //FIXME: there should be an else branch here to remove the
> > +        //free cells, which are not needed. They should be removed from the map
> > +        //Can't remove items in a HashMap while iterating over it. Should
> > +        //store the keys in an array and remove them afterwards.
> 
> Again, if this is important then we should do it.

This shouldn't be an issue any longer.

> > +    }
> > +}
> > +
> > +} // namespace JSC
> 
> > Index: JavaScriptCore/profiler/Profiler.cpp
> > ===================================================================
> > --- JavaScriptCore/profiler/Profiler.cpp	(revision 44682)
> > +++ JavaScriptCore/profiler/Profiler.cpp	(working copy)
> > @@ -86,8 +98,11 @@ PassRefPtr<Profile> Profiler::stopProfil
> >              RefPtr<Profile> returnProfile = profileGenerator->profile();
> >  
> >              m_currentProfiles.remove(i);
> > -            if (!m_currentProfiles.size())
> > +            if (!m_currentProfiles.size()) {
> > +                //There is no need to check whether m_isMemoryProfiling is enabled
> 
> Why? Is the assumption that setDoCollectAllocationTraces is cheap and will
> always be so? 
> 
> > +                JSCell::setDoCollectAllocationTraces(false);
> >                  s_sharedEnabledProfilerReference = 0;
> > +            }

It should be cheap, but you're right, there no solid reason not to check.
Removed comment and added check.

> >  
> > +void Profiler::objectCreated(ExecState* exec, JSCell* createdObject)
> > +{
> > +    ASSERT(!m_currentProfiles.isEmpty());
> > +
> > +    dispatchObjectToProfiles(m_currentProfiles, &ProfileGenerator::objectCreated, createdObject, exec->lexicalGlobalObject()->profileGroup());
> 
> Since this is the only caller to dispatchObjectToProfiles you don't really need
> to pass the m_currentProfiles, but I guess it looks like you were intending
> this to have more callers with different functions passed to it?

Indeed, there were more callers intended for this function.
I've removed the dispatchObjectToProfiles function completely and moved the code inside
objectCreated, its only caller.

> This is as far as I got with this today.  I'll try to look at the rest of it,
> or a new version of it, later.
> One thing that I think would be good would be a way to test this.  I realize
> that there is a lot of functionality in the Web Inspector that does not have
> automated testing,
> and that I am one of the culprits of the Inspector being in that state, but I
> don't want to continue this trend of expanding Web Inspector functionality
> without a way to
> implement automated tests when bugs are fixed.
> 
> I'm not sure the automated tests are necessary for this patch, but it's
> something I'd love to hear feedback on from the rest of the community.
> 

I've added some (not very complex) tests to verify the following:
- the JavaScript objects and properties exposed on the Profile and ObjectIdentifier interface
- stack traces for object allocation
- types for allocated objects
- number of allocated / collected objects in the given period of time

What's left is testing the actual size of the objects, which is the tough part. I've given this some
thought and can't say that I've come to any conclusions. The issue is that there's no reliable way
to predict the expected results.

I'm not keen on 'hardcoding' the expected sizes in the tests, since the evolution of the returned size
is not predictable.  Some random implementation change may fail a lot of tests for no reason.

One idea would be to do a 'fuzzy' measurement of size, allowing some degree of variation.

Another would be to find a reproducible relationship between the sizes of objects (e.g. "an array of five
objects should have a smaller size than an array of ten") which can be used to check validity. This,
however is not as simple as it sounds (the former example depends on the implementation of Array,
which doesn't guarantee that relationship).

I'd like to some feedback on this issue as well.

[Kevin McCullough]

Comment on attachment 31800 [details]
Adds live object profiling capabilities


> 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.

[Horia Olaru]

Created attachment 32237 [details]
adds live object profiling capabilities

(In reply to comment #15)
> (From update of attachment 31800 [details] [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

[Kevin McCullough]

Comment on attachment 32237 [details]
adds live object profiling capabilities


> Index: JavaScriptCore/profiler/LiveObjects.cpp
> ===================================================================
> --- JavaScriptCore/profiler/LiveObjects.cpp	(revision 0)
> +++ JavaScriptCore/profiler/LiveObjects.cpp	(revision 0)
> +
> +void LiveObjects::removeNode(const JSCell* cell)
> +{
> +    ASSERT(cell);
> +    if (!m_storeCollectedObjects)
> +        m_stackMap.remove(cell);
> +    else {
> +        RefPtr<ProfileNode> node = m_stackMap.take(cell);
> +        if (node && !isFree(cell))
> +            m_cachedObjects.append(ObjectIdentifier::create(m_cachedObjects.size(), cell, node));

Why are we appending to m_cachedObjects only if cell was not freed?

> +    }
> +}
> +// This method is meant to be called from the stopProfiling method of ProfileGenerator
> +// After profiling stops, live objects are likely to be collected, so we store the
> +// information we have now.
> +void LiveObjects::finalizeCollectedData()
> +{
> +    toVector(m_cachedObjects);
> +

You can probably put toVector()'s code here since it's not called from anywhere else

> +    // We no longer need the stack map after this
> +    m_stackMap.clear();
> +}

> Index: JavaScriptCore/profiler/ObjectIdentifier.h
> ===================================================================
> --- JavaScriptCore/profiler/ObjectIdentifier.h	(revision 0)
> +++ JavaScriptCore/profiler/ObjectIdentifier.h	(revision 0)
> +namespace JSC {
> +
> +    class ProfileNode;
> +
> +    class ObjectIdentifier : public RefCounted<ObjectIdentifier> {
> +    public:
> +        unsigned uid() const { return m_uid; }
> +        UString type() const { return m_type; }
> +        unsigned size() const { return m_size; }
> +        ProfileNode* creatingFunction() const { return m_creatingFunction.get(); }
> +
> +        static PassRefPtr<ObjectIdentifier> create(unsigned, const JSCell*, PassRefPtr<ProfileNode>);

This is just my own nit pick but I think our style (although it may not be documented) is to put the create function before the accessors.

> +
> +    private:
> +        unsigned m_uid;
> +        UString m_type;
> +        unsigned m_size;
> +        RefPtr<ProfileNode> m_creatingFunction;
> +
> +        ObjectIdentifier(unsigned, const JSCell*, PassRefPtr<ProfileNode>);

Likewise the constructor before the members.

> +    };
> +
> Index: JavaScriptCore/profiler/Profiler.cpp
> ===================================================================
> --- JavaScriptCore/profiler/Profiler.cpp	(revision 45533)
> +++ JavaScriptCore/profiler/Profiler.cpp	(working copy)
> @@ -86,8 +98,12 @@ PassRefPtr<Profile> Profiler::stopProfil
>              RefPtr<Profile> returnProfile = profileGenerator->profile();
>  
>              m_currentProfiles.remove(i);
> -            if (!m_currentProfiles.size())
> +            if (!m_currentProfiles.size()) {
> +                if (m_isMemoryProfilingEnabled)
> +                    JSCell::setDoCollectAllocationTraces(false);
> +

The current Profiler can be running multiple Profiles at the same time.  If we start and stop JSCell from collecting allocation traces when a profile is started and stopped then if two profiles are started and one stops the second
profile will no longer get collection or allocation information.  The was we solve this problem currently is that the JS is re-compiled with the debugging hooks only when the profiler is enabled and in dispatchFunctionToProfiles() the
list of profiles is empty so no messages are sent.  This still causes a callIdentifier to be created which is wasteful and we could get some benefit here from doing less work, but the point is that we need to preserve the 
functionality of allowing multiple profiles to run at once.

>                  s_sharedEnabledProfilerReference = 0;
> +            }
>              
>              return returnProfile;
>          }
> Index: JavaScriptCore/runtime/JSArray.cpp
> ===================================================================
> --- JavaScriptCore/runtime/JSArray.cpp	(revision 45533)
> +++ JavaScriptCore/runtime/JSArray.cpp	(working copy)
> @@ -1048,6 +1048,22 @@ void JSArray::checkConsistency(Consisten
>  
>  #endif
>  
> +size_t JSArray::instanceSize() const
> +{
> +    // The Base::instanceSize includes the size of the property storage map,
> +    // which counts for cases when the array stores data as properties
> +    size_t computedSize = Base::instanceSize();
> +
> +    // ArrayStorage - storageSize return value includes the storage vector 
> +    computedSize += storageSize(m_storage->m_vectorLength);

Please put a space before and after "->" (e.g. "m_storage -> m_vectorLength")

> +
> +    //  size of Sparse value map
> +    if (SparseArrayValueMap* map = m_storage->m_sparseValueMap)
> +        computedSize += sizeof(*map) + map->capacity() * sizeof(SparseArrayValueMap::ValueType);

Ditto

> +
> +    return computedSize;
> +}
> +
>  JSArray* constructEmptyArray(ExecState* exec)
>  {
>      return new (exec) JSArray(exec->lexicalGlobalObject()->arrayStructure());

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 45533)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,61 @@
> [phone]  Horia Olaru  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Bug 10930: WebKit could benefit from a JavaScript live object profiler
> +        https://bugs.webkit.org/show_bug.cgi?id=10930
> +
> +        Added two new headers to forward JavaScriptCore headers.
> +
> +        Modified CodeGeneratorJS.pm to check for the "HasInstanceSize"
> +        attribute and insert an instanceSize method in the generated JSObjects.
> +        This method calls the elementSize method from the DOM object
> +        implementation.
> +
> +        Added an elementSize method to HTMLImageElement and updated the idl
> +        to reflect this for the generator script. This is not a complete
> +        implementation of a size function, but is added to prove functionality.

This concerns me.  What more work needs to be done?

> +
> +        Added the JavaScriptObjectIdentifier to the inspector. This is a class
> +        containing the JS representation of a profiler live object. The new
> +        liveObjects property added to the JavaScriptProfile class is an Array
> +        of such objects.
> +
> +        Added the setMemoryProfilingEnabled(boolean) function to the console
> +        object.
> Index: WebCore/html/HTMLImageElement.cpp
> ===================================================================
> --- WebCore/html/HTMLImageElement.cpp	(revision 45533)
> +++ WebCore/html/HTMLImageElement.cpp	(working copy)
> @@ -40,6 +40,16 @@ namespace WebCore {
>  
>  using namespace HTMLNames;
>  
> +size_t HTMLImageElement::elementSize() const
> +{
> +    size_t computedSize = sizeof(*this);
> +
> +    if(cachedImage() && cachedImage()->image()->data())
> +        computedSize += cachedImage()->image()->data()->size();
> +
> +    return computedSize;
> +}

Is this the only HTMLElement that needs an elementSize()?

> Index: WebCore/page/Console.cpp
> ===================================================================
> --- WebCore/page/Console.cpp	(revision 45533)
> +++ WebCore/page/Console.cpp	(working copy)
> @@ -315,6 +315,19 @@ void Console::profileEnd(const JSC::UStr
>      controller->addProfile(profile, lastCaller.lineNumber(), lastCaller.sourceURL());
>  }
>  
> +void Console::setMemoryProfilingEnabled(bool isEnabled)
> +{
> +    Page* page = this->page();
> +    if (!page)
> +        return;
> +
> +    // FIXME: see the FIXME in Console::profile
> +    if (!page->inspectorController()->profilerEnabled())
> +        return;
> +
> +    JSC::Profiler::profiler()->setIsMemoryProfiling(isEnabled);
> +}
> +

I think this patch may be best served by being broken up.  Maybe the JSC part landed first then the WebCore part.  I'm not comfortable exposing a temporary API just to make it work for now with plans to replace it later.  That is a
good way to end up with API to stay persistent much longer than intended.

For the purposes of the test cases I'd say just make memory profiling be enabled whenever normal profiling is, or maybe someone else has a better suggestion.

It looks like this is getting pretty good and almost ready to land.  I think it might work well to break the patch up, and we'll need to get some UI as soon as possible after this lands.

[Joseph Pecoraro]

Taking this, and using it as the root of the Web Inspector's Heap Instrumentation feature. I will be relating sub tasks, etc.

[Joseph Pecoraro]

I think we can mark this as closed now that we have the ability to take Heap Snapshots using the JavaScript Allocations Timeline in Web Inspector.

Please file enhancement requests or report any issues you find!

From the original webkit-dev thread and comments, it was mentioned that one of the desired features was getting a backtrace / callstack for individual allocations. That feature has not yet been implemented. It is tracked by:
<https://webkit.org/b/156764> Consider gathering backtraces at allocation sites to show in the web inspector