[Webkit-unassigned] [Bug 10930] WebKit could benefit from a JavaScript live object profiler
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Jun 24 12:03:32 PDT 2009
https://bugs.webkit.org/show_bug.cgi?id=10930
olaru at adobe.com changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #31295|0 |1
is obsolete| |
Attachment #31800| |review?
Flag| |
------- Comment #14 from olaru at adobe.com 2009-06-24 12:03 PDT -------
Created an attachment (id=31800)
--> (https://bugs.webkit.org/attachment.cgi?id=31800&action=view)
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.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
More information about the webkit-unassigned
mailing list