Bug 30328 - Web Inspector: Migrate profiles to the injected script-based schema.
: Web Inspector: Migrate profiles to the injected script-based schema.
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated)
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To: Nobody
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-10-13 01:51 PDT by Mikhail Naganov
Modified: 2009-10-15 02:32 PDT (History)
3 users (show)

See Also:


Attachments
proposed change (17.49 KB, patch)
2009-10-13 01:59 PDT, Mikhail Naganov
pfeldman: review-
Details | Formatted Diff | Diff
proposed change, comments addressed (19.27 KB, patch)
2009-10-14 01:25 PDT, Mikhail Naganov
pfeldman: review-
Details | Formatted Diff | Diff
proposed change, comments addressed (20.49 KB, patch)
2009-10-14 03:07 PDT, Mikhail Naganov
commit-queue: review-
Details | Formatted Diff | Diff
proposed patch - now profiler tests pass (20.07 KB, patch)
2009-10-14 14:20 PDT, Mikhail Naganov
pfeldman: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mikhail Naganov 2009-10-13 01:51:56 PDT
Continuing Pavel's efforts on migrating Inspector interaction, I've migrated CPU profiles retrieval to use injected script-based schema.

What had been changed:
 - profile trees are encoded into JS objects, without 'parent' and 'head' references, for further JSONification (see InjectedScript.serializeProfile);
 - on the Inspector's side, 'parent' and 'head' properties are restored (see ProfileView._assignParentsInProfile);
 - when populating existing profiles on Inspector re-opening, they are loaded lazily; initially, only profile titles are loaded (see ProfilesPanel._populateProfiles), a full profile is retrieved when opening the view (see ProfileView constructor).

I also discovered that C++ side of profile objects and their C++/JS bridges contain unused methods and props, I will remove in the next change.
Comment 1 Mikhail Naganov 2009-10-13 01:59:42 PDT
Created attachment 41095 [details]
proposed change
Comment 2 Pavel Feldman 2009-10-13 12:58:04 PDT
Comment on attachment 41095 [details]
proposed change

We now are spending twice as much memory for the profile during serialization. Do you think we could tweak native object a bit so that it we don't need additional wrapping?

> +        ScriptFunctionCall function(m_scriptState, m_injectedScriptObj, "serializeProfile");
> +        function.appendArgument(toJS(m_scriptState, it->second.get()));
> +        ScriptValue serializedProfile = function.call();
> +        m_frontend->didGetProfile(callId, serializedProfile);

Could you extract wrapping into a separate method? (I see two usages).

Otherwise looks good.
Comment 3 Mikhail Naganov 2009-10-14 01:25:51 PDT
Created attachment 41153 [details]
proposed change, comments addressed

OK, I removed "parent", "head", and other unneeded properties from the wrapper object, so additional serialization is no more needed. This also addresses the second comment, as there is no more call to a function from InjectedScript.
Comment 4 Pavel Feldman 2009-10-14 01:41:58 PDT
Comment on attachment 41153 [details]
proposed change, comments addressed

> +    void getProfileHeaders(long callId);
> +    void getProfile(long callId, unsigned uid);

These should be private in InspectorController.

> +        function profileCallback(profile) {
> +            self.profile = profile;
> +            continueInitialization();
> +        }

{ on next line

> +        if (!this.profile._isStub)

Why do you end up with profiles in different states? I think you should resolve profile only here and only once.

Otherwise r+.
Comment 5 Mikhail Naganov 2009-10-14 03:07:36 PDT
Created attachment 41155 [details]
proposed change, comments addressed

OK, now when a profile is added, only header is added actually. Thus, profiles are always loaded on demand when the view is about to be shown.
Comment 6 WebKit Commit Bot 2009-10-14 06:30:21 PDT
Comment on attachment 41155 [details]
proposed change, comments addressed

Rejecting patch 41155 from review queue.

pfeldman@chromium.org does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/modules/committers.py.
Comment 7 Pavel Feldman 2009-10-14 06:32:55 PDT
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/bindings/js/JSInspectorBackendCustom.cpp
	M	WebCore/bindings/v8/custom/V8CustomBinding.h
	M	WebCore/inspector/InspectorBackend.cpp
	M	WebCore/inspector/InspectorBackend.h
	M	WebCore/inspector/InspectorBackend.idl
	M	WebCore/inspector/InspectorController.cpp
	M	WebCore/inspector/InspectorController.h
	M	WebCore/inspector/InspectorFrontend.cpp
	M	WebCore/inspector/InspectorFrontend.h
	M	WebCore/inspector/JavaScriptProfileNode.cpp
	M	WebCore/inspector/front-end/ProfileView.js
	M	WebCore/inspector/front-end/ProfilesPanel.js
	M	WebCore/inspector/front-end/inspector.js
Committed r49558
Comment 8 Pavel Feldman 2009-10-14 07:55:20 PDT
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/bindings/js/JSInspectorBackendCustom.cpp
	M	WebCore/bindings/v8/custom/V8CustomBinding.h
	M	WebCore/inspector/InspectorBackend.cpp
	M	WebCore/inspector/InspectorBackend.h
	M	WebCore/inspector/InspectorBackend.idl
	M	WebCore/inspector/InspectorController.cpp
	M	WebCore/inspector/InspectorController.h
	M	WebCore/inspector/InspectorFrontend.cpp
	M	WebCore/inspector/InspectorFrontend.h
	M	WebCore/inspector/JavaScriptProfileNode.cpp
	M	WebCore/inspector/front-end/ProfileView.js
	M	WebCore/inspector/front-end/ProfilesPanel.js
	M	WebCore/inspector/front-end/inspector.js
Committed r49560
Comment 9 Pavel Feldman 2009-10-14 07:58:15 PDT
Guess what - profiler tests rely upon the API you have changed...

fast/profiler/anonymous-event-handler.html -> failed
.
fast/profiler/anonymous-function-called-from-different-contexts.html -> failed
.
fast/profiler/anonymous-function-calls-built-in-functions.html -> failed
.
fast/profiler/anonymous-function-calls-eval.html -> failed
.
fast/profiler/anonymous-functions-with-display-names.html -> failed
.
fast/profiler/apply.html -> failed
.
fast/profiler/built-in-function-calls-anonymous.html -> failed
.
fast/profiler/built-in-function-calls-user-defined-function.html -> failed
.
fast/profiler/call-nodelist-as-function.html -> failed
..
fast/profiler/call.html -> failed
.
fast/profiler/calling-the-function-that-started-the-profiler-from-another-scope.html -> failed
.
fast/profiler/compare-multiple-profiles.html -> failed
.
fast/profiler/constructor.html -> failed
.
fast/profiler/dead-time.html -> failed
.
fast/profiler/document-dot-write.html -> failed
.
fast/profiler/event-handler.html -> failed
.
fast/profiler/execution-context-and-eval-on-same-line.html -> failed
.
fast/profiler/inline-event-handler.html -> failed
.
fast/profiler/many-calls-in-the-same-scope.html -> failed
.
fast/profiler/multiple-and-different-scoped-anonymous-function-calls.html -> failed
.
fast/profiler/multiple-and-different-scoped-function-calls.html -> failed
.
fast/profiler/multiple-anonymous-functions-called-from-the-same-function.html -> failed
.
fast/profiler/multiple-frames.html -> failed
.
fast/profiler/named-functions-with-display-names.html -> failed
.
fast/profiler/nested-anonymous-functon.html -> failed
.
fast/profiler/nested-start-and-stop-profiler.html -> failed
..
fast/profiler/one-execution-context.html -> failed
.
fast/profiler/profile-calls-in-included-file.html -> failed
.
fast/profiler/profile-with-no-title.html -> failed
.
fast/profiler/profiling-from-a-nested-location-but-stop-profiling-outside-the-nesting.html -> failed
.
fast/profiler/profiling-from-a-nested-location.html -> failed
.
fast/profiler/simple-event-call.html -> failed
.
fast/profiler/simple-no-level-change.html -> failed
.
fast/profiler/start-and-stop-profiler-multiple-times.html -> failed
.
fast/profiler/start-and-stop-profiling-in-the-same-function.html -> failed
..
fast/profiler/stop-profiling-after-setTimeout.html -> failed
.
fast/profiler/stop-then-function-call.html -> failed
.
fast/profiler/throw-exception-from-eval.html -> failed
.
fast/profiler/two-execution-contexts.html -> failed
.
fast/profiler/user-defined-function-calls-built-in-functions.html -> failed
.
fast/profiler/window-dot-eval.html -> failed
Comment 10 Darin Adler 2009-10-14 08:21:43 PDT
Maybe llround is not available on Windows?
Comment 11 Darin Adler 2009-10-14 08:23:46 PDT
Sorry, comment added to the wrong bug.
Comment 12 Mikhail Naganov 2009-10-14 14:20:38 PDT
Created attachment 41189 [details]
proposed patch - now profiler tests pass

I'm really sorry for putting you into a trouble, Pavel.

It appears that I was too aggressive in removing properties from the profile node wrapper---"visible" property is what profiler tests were missing.
Comment 13 Pavel Feldman 2009-10-15 02:32:26 PDT
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/bindings/js/JSInspectorBackendCustom.cpp
	M	WebCore/bindings/v8/custom/V8CustomBinding.h
	M	WebCore/inspector/InspectorBackend.cpp
	M	WebCore/inspector/InspectorBackend.h
	M	WebCore/inspector/InspectorBackend.idl
	M	WebCore/inspector/InspectorController.cpp
	M	WebCore/inspector/InspectorController.h
	M	WebCore/inspector/InspectorFrontend.cpp
	M	WebCore/inspector/InspectorFrontend.h
	M	WebCore/inspector/JavaScriptProfileNode.cpp
	M	WebCore/inspector/front-end/ProfileView.js
	M	WebCore/inspector/front-end/ProfilesPanel.js
	M	WebCore/inspector/front-end/inspector.js
Committed r49615