Summary: | Web Inspector: [Chromium] Add an ability to look up and explore an object from a heap profile | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mikhail Naganov <mnaganov> | ||||||||||||
Component: | Web Inspector (Deprecated) | Assignee: | Mikhail Naganov <mnaganov> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | apavlov, bweinstein, dglazkov, gustavo, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, webkit.review.bot, xan.lopez, yurys | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | All | ||||||||||||||
OS: | All | ||||||||||||||
Bug Depends on: | 69234, 70039 | ||||||||||||||
Bug Blocks: | |||||||||||||||
Attachments: |
|
Description
Mikhail Naganov
2011-05-20 02:09:29 PDT
Created attachment 104521 [details]
WIP: A prototype implementation
Comment on attachment 104521 [details] WIP: A prototype implementation View in context: https://bugs.webkit.org/attachment.cgi?id=104521&action=review Overall looks good. Could you please get rid of the copypaste and we can give it green light. > Source/WebCore/inspector/Inspector.json:1840 > + { "name": "objectId", "type": "integer" } You should probably start expanding your type definitions in this JSON. > Source/WebCore/inspector/front-end/DetailedHeapshotGridNodes.js:273 > + if (this._type === "string") { This looks like a nasty copypaste. You could either create a new class ObjectPopover.js (preferred) or place this code into the ObjectPropertiesSection. > Source/WebCore/inspector/front-end/DetailedHeapshotView.js:1048 > + var element = hover[0]; Not nice *** Bug 61176 has been marked as a duplicate of this bug. *** Created attachment 109160 [details]
Patch
Might not compile on the cr-linux bot, as the new V8 version hasn't been pushed yet.
Also, contains 3 style violations:
Source/WebCore/inspector/InspectorProfilerAgent.cpp:382: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5]
Source/WebCore/inspector/InspectorProfilerAgent.h:94: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5]
-- These two can't be fixed, as in Inspector RPC we pass results this way. --
Source/WebCore/bindings/v8/ScriptProfiler.cpp:92: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4]
-- Results of 'if' branches are different types, so '?:' can't be used. --
Attachment 109160 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/inspector/InspectorProfilerAgent.cpp:382: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5]
Source/WebCore/inspector/InspectorProfilerAgent.h:94: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5]
Source/WebCore/bindings/v8/ScriptProfiler.cpp:92: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4]
Total errors found: 3 in 15 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 109160 [details] Patch Attachment 109160 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/9900083 Comment on attachment 109160 [details] Patch Attachment 109160 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/9893332 Created attachment 109162 [details]
Add JSC stubs
Attachment 109162 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/inspector/InspectorProfilerAgent.cpp:382: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5]
Source/WebCore/inspector/InspectorProfilerAgent.h:94: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5]
Source/WebCore/bindings/v8/ScriptProfiler.cpp:92: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4]
Total errors found: 3 in 17 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 109162 [details] Add JSC stubs Attachment 109162 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9880917 Comment on attachment 109162 [details] Add JSC stubs Attachment 109162 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/9889387 Created attachment 109163 [details]
JSC fix again
Attachment 109163 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/inspector/InspectorProfilerAgent.cpp:382: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5]
Source/WebCore/inspector/InspectorProfilerAgent.h:94: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5]
Source/WebCore/bindings/v8/ScriptProfiler.cpp:92: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4]
Total errors found: 3 in 17 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 109163 [details] JSC fix again Attachment 109163 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9898134 Comment on attachment 109163 [details] JSC fix again View in context: https://bugs.webkit.org/attachment.cgi?id=109163&action=review Overall looks good. A number of nits to be fixed prior to landing. > Source/WebCore/bindings/js/ScriptProfiler.cpp:45 > +PassRefPtr<InspectorValue> ScriptProfiler::getObjectByHeapObjectId(unsigned, InjectedScriptManager*) While we use "get" prefixes in the protocol, regular WebCore classes should not have those. > Source/WebCore/bindings/v8/ScriptProfiler.cpp:83 > + v8::HandleScope scope; Blank line here would be great. > Source/WebCore/bindings/v8/ScriptProfiler.cpp:87 > + v8::Handle<v8::Object> object(value.As<v8::Object>()); ditto >> Source/WebCore/bindings/v8/ScriptProfiler.cpp:92 >> + if (!injectedScript.hasNoValue()) > > An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] Please follow the suggested style. > Source/WebCore/inspector/Inspector.json:1947 > + { "name": "objectId", "type": "integer" } You should define class for your ids in the long run. > Source/WebCore/inspector/InspectorProfilerAgent.cpp:384 > + if (m_frontend) { no need to check for front-end. > Source/WebCore/inspector/InspectorProfilerAgent.cpp:386 > + heapObject->asObject(result); Since this result is not optional (according to the protocol), you can't pass null here. You should assign errorString. > Source/WebCore/inspector/front-end/DetailedHeapshotGridNodes.js:189 > + } else if (node.flags & tree.snapshot.nodeFlags.canBeQueried) { no need for {} > Source/WebCore/inspector/front-end/DetailedHeapshotGridNodes.js:274 > + if (this._type === "string") { no need for {} > Source/WebCore/inspector/front-end/DetailedHeapshotGridNodes.js:275 > + callback({ type: "string", description: this._name }); Use WebInspector.RemoteObject.fromPrimitiveValue(this._name) > Source/WebCore/inspector/front-end/DetailedHeapshotGridNodes.js:279 > + if (object.type) You should check for the error here. > Source/WebCore/inspector/front-end/DetailedHeapshotGridNodes.js:282 > + callback({ type: "null", description: WebInspector.UIString("Not available") }); ditto > Source/WebCore/inspector/front-end/HeapSnapshot.js:995 > + var list = []; A test for this method would be great. > Source/WebCore/inspector/front-end/Popover.js:292 > +WebInspector.ObjectPopoverHelper = function(panelElement, getAnchor, queryObject, onHide, disableOnClick) Please extract this in a separate change. (In reply to comment #15) > (From update of attachment 109163 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=109163&action=review > > Overall looks good. A number of nits to be fixed prior to landing. > > > Source/WebCore/bindings/js/ScriptProfiler.cpp:45 > > +PassRefPtr<InspectorValue> ScriptProfiler::getObjectByHeapObjectId(unsigned, InjectedScriptManager*) > > While we use "get" prefixes in the protocol, regular WebCore classes should not have those. > Done. > > Source/WebCore/bindings/v8/ScriptProfiler.cpp:83 > > + v8::HandleScope scope; > > Blank line here would be great. > Done. > > Source/WebCore/bindings/v8/ScriptProfiler.cpp:87 > > + v8::Handle<v8::Object> object(value.As<v8::Object>()); > > ditto > Done. > >> Source/WebCore/bindings/v8/ScriptProfiler.cpp:92 > >> + if (!injectedScript.hasNoValue()) > > > > An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] > > Please follow the suggested style. > Done. > > Source/WebCore/inspector/Inspector.json:1947 > > + { "name": "objectId", "type": "integer" } > > You should define class for your ids in the long run. > Thanks, I'll consider that. > > Source/WebCore/inspector/InspectorProfilerAgent.cpp:384 > > + if (m_frontend) { > > no need to check for front-end. > Done. > > Source/WebCore/inspector/InspectorProfilerAgent.cpp:386 > > + heapObject->asObject(result); > > Since this result is not optional (according to the protocol), you can't pass null here. You should assign errorString. > Done. > > Source/WebCore/inspector/front-end/DetailedHeapshotGridNodes.js:189 > > + } else if (node.flags & tree.snapshot.nodeFlags.canBeQueried) { > > no need for {} > Done. > > Source/WebCore/inspector/front-end/DetailedHeapshotGridNodes.js:274 > > + if (this._type === "string") { > > no need for {} > Done. > > Source/WebCore/inspector/front-end/DetailedHeapshotGridNodes.js:275 > > + callback({ type: "string", description: this._name }); > > Use WebInspector.RemoteObject.fromPrimitiveValue(this._name) > Done. > > Source/WebCore/inspector/front-end/DetailedHeapshotGridNodes.js:279 > > + if (object.type) > > You should check for the error here. > Done. > > Source/WebCore/inspector/front-end/DetailedHeapshotGridNodes.js:282 > > + callback({ type: "null", description: WebInspector.UIString("Not available") }); > > ditto > Done. > > Source/WebCore/inspector/front-end/HeapSnapshot.js:995 > > + var list = []; > > A test for this method would be great. > Adding... > > Source/WebCore/inspector/front-end/Popover.js:292 > > +WebInspector.ObjectPopoverHelper = function(panelElement, getAnchor, queryObject, onHide, disableOnClick) > > Please extract this in a separate change. Extracted as https://bugs.webkit.org/show_bug.cgi?id=69234 Created attachment 109687 [details]
Comments addressed, test added
Still will not compile on cr-linux, V8 push should happen soon.
Attachment 109687 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/insp..." exit_code: 1
Source/WebCore/inspector/InspectorProfilerAgent.cpp:382: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5]
Source/WebCore/inspector/InspectorProfilerAgent.h:94: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5]
Total errors found: 2 in 19 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 109687 [details] Comments addressed, test added Attachment 109687 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9944529 Manually committed http://trac.webkit.org/changeset/97362 commit 3d65d067f74ec42ee5334fbf61de6a4731d001e2 Author: Mikhail Naganov <mnaganov@chromium.org> Date: Wed Aug 3 15:28:43 2011 +0100 Web Inspector: [Chromium] Add an ability to look up and explore an object from a heap profile. https://bugs.webkit.org/show_bug.cgi?id=61179 This is exteremely helpful when dealing with DOM wrappers, as their properties are mostly implemented with getters and thus not stored in heap snapshots. Reviewed by Pavel Feldman. * English.lproj/localizedStrings.js: * bindings/js/ScriptProfiler.cpp: (WebCore::ScriptProfiler::objectByHeapObjectId): * bindings/js/ScriptProfiler.h: * bindings/v8/ScriptProfiler.cpp: (WebCore::ScriptProfiler::objectByHeapObjectId): * bindings/v8/ScriptProfiler.h: * inspector/Inspector.json: * inspector/InspectorController.cpp: (WebCore::InspectorController::InspectorController): * inspector/InspectorProfilerAgent.cpp: (WebCore::InspectorProfilerAgent::create): (WebCore::InspectorProfilerAgent::InspectorProfilerAgent): (WebCore::InspectorProfilerAgent::getObjectByHeapObjectId): * inspector/InspectorProfilerAgent.h: * inspector/front-end/DetailedHeapshotGridNodes.js: (WebInspector.HeapSnapshotGridNode.prototype.hasHoverMessage.false.queryObjectContent): (WebInspector.HeapSnapshotGenericObjectNode): (WebInspector.HeapSnapshotGenericObjectNode.prototype.get data): (WebInspector.HeapSnapshotGenericObjectNode.prototype.queryObjectContent.else.formatResult): (WebInspector.HeapSnapshotGenericObjectNode.prototype.queryObjectContent): (WebInspector.HeapSnapshotGenericObjectNode.prototype.shortenWindowURL): * inspector/front-end/DetailedHeapshotView.js: (WebInspector.DetailedHeapshotView.prototype._showObjectPopover): * inspector/front-end/HeapSnapshot.js: (WebInspector.HeapSnapshotNode.prototype.get canBeQueried): (WebInspector.HeapSnapshotNode.prototype.get flags): (WebInspector.HeapSnapshotNode.prototype.get isDOMWindow): (WebInspector.HeapSnapshot.prototype._init): (WebInspector.HeapSnapshot.prototype.dispose): (WebInspector.HeapSnapshot.prototype._flagsOfNode): (WebInspector.HeapSnapshot.prototype._calculateFlags): (WebInspector.HeapSnapshot.prototype.updateStaticData): (WebInspector.HeapSnapshotNodesProvider.prototype._serialize): * inspector/front-end/HeapSnapshotProxy.js: (WebInspector.HeapSnapshotProxy.prototype.get nodeFlags): * inspector/front-end/RemoteObject.js: (WebInspector.RemoteObject.fromError): * inspector/front-end/heapProfiler.css: (.detailed-heapshot-view tr:not(.selected) td.object-column span.highlight): * inspector/profiler/heap-snapshot-expected.txt: * inspector/profiler/heap-snapshot-test.js: (initialize_HeapSnapshotTest.InspectorTest.createHeapSnapshotMockWithDOM): (initialize_HeapSnapshotTest): * inspector/profiler/heap-snapshot.html: Sorry guys, had to roll it out, since it was using the API in V8 and that has been reverted: http://trac.webkit.org/changeset/97372 Re-applied again as http://trac.webkit.org/changeset/97611 V8 has been pushed w/o the new GC, but with the required API. Comment on attachment 109687 [details] Comments addressed, test added View in context: https://bugs.webkit.org/attachment.cgi?id=109687&action=review > Source/WebCore/bindings/v8/ScriptProfiler.cpp:79 > + // As ids are unique, it doesn't matter which HeapSnapshot owns HeapGraphNode. > + // We need to find first HeapSnapshot containing a node with the specified id. > + const v8::HeapGraphNode* node = 0; > + for (int i = 0, l = v8::HeapProfiler::GetSnapshotsCount(); i < l; ++i) { > + const v8::HeapSnapshot* snapshot = v8::HeapProfiler::GetSnapshot(i); > + node = snapshot->GetNodeById(id); > + if (node) > + break; > + } The complexity of this method is N*ln2(K) where N is the number of snapshots and K is the number of nodes. As far as Id is a monotonic function you can have a maxId property in the each snapshot and find the object with complexity ln2(N) + ln2(K) (In reply to comment #23) > (From update of attachment 109687 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=109687&action=review > > > Source/WebCore/bindings/v8/ScriptProfiler.cpp:79 > > + // As ids are unique, it doesn't matter which HeapSnapshot owns HeapGraphNode. > > + // We need to find first HeapSnapshot containing a node with the specified id. > > + const v8::HeapGraphNode* node = 0; > > + for (int i = 0, l = v8::HeapProfiler::GetSnapshotsCount(); i < l; ++i) { > > + const v8::HeapSnapshot* snapshot = v8::HeapProfiler::GetSnapshot(i); > > + node = snapshot->GetNodeById(id); > > + if (node) > > + break; > > + } > > The complexity of this method is N*ln2(K) where N is the number of snapshots and K is the number of nodes. > As far as Id is a monotonic function you can have a maxId property in the each snapshot and find the object with complexity ln2(N) + ln2(K) Right, but N isn't big usually. Sure, we can optimize it anyway. (In reply to comment #24) > (In reply to comment #23) > > (From update of attachment 109687 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=109687&action=review > > > > > Source/WebCore/bindings/v8/ScriptProfiler.cpp:79 > > > + // As ids are unique, it doesn't matter which HeapSnapshot owns HeapGraphNode. > > > + // We need to find first HeapSnapshot containing a node with the specified id. > > > + const v8::HeapGraphNode* node = 0; > > > + for (int i = 0, l = v8::HeapProfiler::GetSnapshotsCount(); i < l; ++i) { > > > + const v8::HeapSnapshot* snapshot = v8::HeapProfiler::GetSnapshot(i); > > > + node = snapshot->GetNodeById(id); > > > + if (node) > > > + break; > > > + } > > > > The complexity of this method is N*ln2(K) where N is the number of snapshots and K is the number of nodes. > > As far as Id is a monotonic function you can have a maxId property in the each snapshot and find the object with complexity ln2(N) + ln2(K) > > Right, but N isn't big usually. Sure, we can optimize it anyway. I'd like to see this property in the snapshots just for minimize the complexity in my patch :) |