[Webkit-unassigned] [Bug 37025] Web Inspector: GC Run event should be added to Timeline Panel

Sun Apr 4 04:17:11 PDT 2010

https://bugs.webkit.org/show_bug.cgi?id=37025

--- Comment #10 from Ilya Tikhonovsky <loislo at chromium.org>  2010-04-04 04:17:11 PST ---
(In reply to comment #9)

>> +        static void pushCollectedGCEvents(InspectorTimelineAgent*) {}
>> +        static void getHeapSize(size_t&, size_t&) {}

> - The fact that same tests are passing for JSC and V8 with no instrumentation
> for JSC is confusing. What would WebKit user see in the UI?

The users will see standard popups without Heap Size info. Also GC Run event
will not appear until somebody write an implementation of these methods for
JSC. The usedHeapSize/totalHeapSize properties at frontend side are defined but
zero in JSC case.

> - Bi-directional dependency between InspectorTimelineAgent and
> ScriptGCEventHelper bothers me. In theory, it should exist: you query for heap
> size and push gc events (i.e. pass control both ways). But I can see that your
> push is not really a push - it is accumulating events and timeline is actually
> queryign for them. So there really is a TimelineAgent -> ScriptGCEventHelper
> dependency only.

It is not possible to call TimelineAgent from GC directly but I can push GCRun
events to the agent 
each time when agent ask GCEventHelper about HeapSize info.
In case of pull design scheme, the agent will have to know about internal
structure of GCEventHelper.

>> +        v8::V8::AddGCPrologueCallback(gcPrologueCallback);
>> +        v8::V8::AddGCEpilogueCallback(gcEpilogueCallback);

> How expensive is being a callback?

These callbacks will be called once per garbage collector run if TimelineAgent
is in operation.
Helper for V8 is using vector for events data. Usually it will be small
constant time. 

> So you are polling for gc runs, not pushing them actually. So your GC events
> are not in line with other records time-wise. Is that a problem? Another thing
> is that you are polling for GC records in random places to my taste: on
> willSendResourceRequest, didFinishLoadingResource, addRecordToTimeline,
> pushCurrentRecord. I realize that it might be related to the helper method
> structure in the timeline agent, but I'd like to see it fixed since it really
> start looking random to me.

As far as GC run is happening more or less randomly I'm trying to push gc
events into to timeline each time when TimelineAgent is called via will or did
methods.
It is enough to do that at pushCurrentRecord and addRecordToTimeline but some
events like SendRequest are propagating event record to the frontend directly.

> FYI: some time ago we agreed that we should create jsc and v8 folders under
> WebCore/inspector where we would put things like ScriptGCEventHelper. Unlike
> bindings, in our case, they would share same interface header located under
> WebCore/inspector. You can be the first guy to do that. Or we will move these
> guys at once, so not a strict requirement to do it now.

I think it is not so many chances to use JSC and V8 at the some time and there
is no special reason to have an abstract class and implementation with virtual
functions.

> (From update of attachment 52498 [details])
> > +        static void pushCollectedGCEvents(InspectorTimelineAgent*) {}
> > +        static void getHeapSize(size_t&, size_t&) {}
> 
> - The fact that same tests are passing for JSC and V8 with no instrumentation
> for JSC is confusing. What would WebKit user see in the UI?
> - If we anyways create an instance of this ScriptGCEventHelper object, then
> these methods should probably be instance methods. In fact, I can see
> pushCollectedGCEvents being invoked on the instance already.
> - Bi-directional dependency between InspectorTimelineAgent and
> ScriptGCEventHelper bothers me. In theory, it should exist: you query for heap
> size and push gc events (i.e. pass control both ways). But I can see that your
> push is not really a push - it is accumulating events and timeline is actually
> queryign for them. So there really is a TimelineAgent -> ScriptGCEventHelper
> dependency only.
> 
> > +        v8::V8::AddGCPrologueCallback(gcPrologueCallback);
> > +        v8::V8::AddGCEpilogueCallback(gcEpilogueCallback);
> 
> How expensive is being a callback?
> 
> > +
> > +void ScriptGCEventHelper::pushCollectedGCEvents(InspectorTimelineAgent* agent)
> > +{
> > +    if (s_gcRunEvents.size()) {
> > +        for (GCRunEvents::iterator i = s_gcRunEvents.begin(); i != s_gcRunEvents.end(); ++i)
> > +            agent->didGCRun(i->startTime, i->endTime, i->collectedBytes);
> > +        s_gcRunEvents.clear();
> > +    }
> 
> So you are polling for gc runs, not pushing them actually. So your GC events
> are not in line with other records time-wise. Is that a problem? Another thing
> is that you are polling for GC records in random places to my taste: on
> willSendResourceRequest, didFinishLoadingResource, addRecordToTimeline,
> pushCurrentRecord. I realize that it might be related to the helper method
> structure in the timeline agent, but I'd like to see it fixed since it really
> start looking random to me.
> > +
> > +namespace WebCore {
> > +    class InspectorTimelineAgent;
> > +
> 
> We should not indent class declarations according to new style.
> 
> FYI: some time ago we agreed that we should create jsc and v8 folders under
> WebCore/inspector where we would put things like ScriptGCEventHelper. Unlike
> bindings, in our case, they would share same interface header located under
> WebCore/inspector. You can be the first guy to do that. Or we will move these
> guys at once, so not a strict requirement to do it now.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.