Bug 144808 – Web Inspector: REGRESSION (r175203): No profile information is shown in Inspector

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Matt Baker]

Created attachment 252736 [details]
[TEST] profile test page

* SUMMARY
No profile information is shown in Inspector.

Recording a profile adds a profile parent record to the JavaScript & Events timeline, but it has no child records.

* STEPS TO REPRODUCE
1. Inspect attached test page (profile-test.html)
2. Begin a new timeline recording from the Inspector.
3. Run the test in test page
4. Select the JavaScript & Events timeline, expand all records.
  => "Test" Profile Recorded is visible, but it has no child records (see attached screenshot).

[Matt Baker]

<rdar://problem/20622499>

[Matt Baker]

Created attachment 252737 [details]
[IMAGE] missing profile records

[Matt Baker]

Created attachment 252764 [details]
[Patch] WIP

Ready for feedback, not ready to land

[Matt Baker]

Created attachment 252765 [details]
[Image] timing mismatch

When a profile is started after recording has already begun, there will be two entries for each profiled function call: one for the outer function that called console.profile, and a second for the profile itself.

I noticed that the total, self, and average times reported for the profiled functions are different for the two cases described above (see screenshot).

This seems like a bug, since the profiled code executed only once. The start times also differ.

[Brian Burg]

Comment on attachment 252764 [details]
[Patch] WIP

Ugh, I regret messing with this code :)

It seems to me that we aren't gaining much by using independent stopwatches at all. The only way it would make sense in the UI is if console profiles created a new Timeline recording, but I'm not sure the tiny number of users for this function justifies the complexity.

The start time doesn't show up as "0" even with how the code is now. Is it possible to always use the shared stopwatch?

[Darin Adler]

Comment on attachment 252764 [details]
[Patch] WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=252764&action=review

Patch is OK, but there are two issues we should fix.

> Source/JavaScriptCore/profiler/Profile.h:32
> +#include <wtf/Stopwatch.h>

Should not include the header. Just need a forward declaration here.

> Source/JavaScriptCore/profiler/Profile.h:39
> +    static PassRefPtr<Profile> create(const String& title, unsigned uid, PassRefPtr<Stopwatch>);

New code should not use PassRefPtr. See <http://www.webkit.org/coding/RefPtr.html>. In this case, the argument should be a Stopwatch& because the function does not take ownership of it and it can’t be null.

[Matt Baker]

> > Source/JavaScriptCore/profiler/Profile.h:39
> > +    static PassRefPtr<Profile> create(const String& title, unsigned uid, PassRefPtr<Stopwatch>);
> 
> New code should not use PassRefPtr. See
> <http://www.webkit.org/coding/RefPtr.html>. In this case, the argument
> should be a Stopwatch& because the function does not take ownership of it
> and it can’t be null.

Now I'm thinking ProfileGenerator should just sample its stopwatch and pass the start time to Profile::create, eliminating the need for a Stopwatch reference entirely.

[Matt Baker]

Created attachment 252832 [details]
[Patch] Proposed Fix

[Darin Adler]

Comment on attachment 252832 [details]
[Patch] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=252832&action=review

Looks OK.

> Source/WebCore/inspector/InspectorTimelineAgent.cpp:260
> +    startProfiling(exec, title, m_instrumentingAgents->inspectorEnvironment().executionStopwatch());

This should change from a double to a std::chrono at some point.

[WebKit Commit Bot]

Comment on attachment 252832 [details]
[Patch] Proposed Fix

Clearing flags on attachment: 252832

Committed r184117: <http://trac.webkit.org/changeset/184117>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.