[Webkit-unassigned] [Bug 10930] WebKit could benefit from a JavaScript live object profiler
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Jul 17 12:35:16 PDT 2009
https://bugs.webkit.org/show_bug.cgi?id=10930
Kevin McCullough <kmccullough at apple.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #32237|review? |review-
Flag| |
--- Comment #17 from Kevin McCullough <kmccullough at apple.com> 2009-07-17 12:35:13 PDT ---
(From update of attachment 32237)
> 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 @@
> +2009-07-03 Horia Olaru <olaru at adobe.com>
> +
> + 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.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list