[webkit-reviews] review denied: [Bug 37025] Web Inspector: GC Run event should be added to Timeline Panel : [Attachment 52498] [patch] second iteration.

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sat Apr 3 22:49:58 PDT 2010


Pavel Feldman <pfeldman at chromium.org> has denied Ilya Tikhonovsky
<loislo at chromium.org>'s request for review:
Bug 37025: Web Inspector: GC Run event should be added to Timeline Panel
https://bugs.webkit.org/show_bug.cgi?id=37025

Attachment 52498: [patch] second iteration.
https://bugs.webkit.org/attachment.cgi?id=52498&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
> +	   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.


More information about the webkit-reviews mailing list