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

[bugzilla-daemon at webkit.org]

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


Pavel Feldman <pfeldman at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #52498|review?                     |review-
               Flag|                            |




--- Comment #9 from Pavel Feldman <pfeldman at chromium.org>  2010-04-03 22:49:59 PST ---
(From update of attachment 52498)
> +        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.