Bug 70620 – [chromium] Route Console::time and Console::timeEnd to trace_event

• Component: New Bugs
• Status: RESOLVED FIXED

[Nat Duca]

[chromium] Route Console::time and Console::timeEnd to trace_event

[Nat Duca]

Created attachment 111986 [details]
Patch

[Nat Duca]

James -- do I need other reviewers?

[WebKit Review Bot]

Comment on attachment 111986 [details]
Patch

Attachment 111986 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10179962

[James Robinson]

Pavel or Yury should take a look.  Could you explain how the tracing works (or point them at the relevant docs), Nat?

[Pavel Feldman]

Comment on attachment 111986 [details]
Patch

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

What is the story behind the change? We tend to do it the other way (push information into the Web Inspector front-end through the WebCore). r- for not compiling in any case.

> Source/WebCore/page/Console.cpp:311
> +#if PLATFORM(CHROMIUM)

I really don't like these. But I don't see a better way of handling it here.

> Source/WebCore/page/Console.cpp:312
> +    if (PlatformSupport::isTraceEventEnabledForCategory("webcore.console"))

I don't see this method on ToT. See compile failures on ews.

[Nat Duca]

Yeah, I agree with the #ifs. If I find time, I'd like to get some platform abstraction of tracing concept landed. In the near term, would this be OK? Its okay if no, but it does help us chase interactions between JS and chrome innards.

The isTraceEventEnabled is b70618, marked as a dep on this bug. That needs some tweaks to land too. :)

[Nat Duca]

(In reply to comment #5)
> What is the story behind the change? We tend to do it the other way (push information into the Web Inspector front-end through the WebCore). r- for not compiling in any case.

Oh, sorry I missed this comment!

This isn't intended for use by web developers. I don't think web devlopers reading about:tracing to understand performance is the *right* thing for our platform.

trace_event ties to chrome's about:tracing, which is an implementer-facing  tool. For example, us folks working on GPU acceleration now ask developers to submit about:trace dumps with their graphics-related bug reports. Whereas inspector information is tailored for human consumption, the about:trace contains extremely detailed implementation level information about what Chrome was doing to lead to bad perf. This has been really useful for us in speeding chrome perf up and we're constantly adding instrumentation to shipping Chrome builds to make these types of perf dumps more informative.

One problem we have with these traces is that we'll see some heavy MessageLoop task in Chrome [e.g. some sync message call] but not have a clue what the JS part of the app was trying to do. Was it their renderer code? Or was it some other piece of third_party advertising code the page just happens to be using?

This patch allows us or our customers to sprinkle time/timeEnd calls throughout JS. The dumps they send us will help us see the relationship between "their code" and our code.

The reason I hooked console.time and timeEnd rather than adding a platform-specific or test-specific macro is because our traces often come from people outside running Canary. Were this to require a custom build of Chrome, it'd limit the utility of the hooking.

Hope that makes sense? I'm totally fine if you think this is a bad thing to land... it was a simple change, so I figured I'd try. :)

[Pavel Feldman]

> Hope that makes sense? I'm totally fine if you think this is a bad thing to land... it was a simple change, so I figured I'd try. :)

I think it is Ok to land it as is once you get the API it uses in place. In return, it would be great if you could experiment with the data and push developer-centric scenarios back into the inspector. I am sure there is enough we could expose to the end-user without frightening him.

[Nat Duca]

(In reply to comment #8)
> In return, it would be great if you could experiment with the data and push developer-centric scenarios back into the inspector. I am sure there is enough we could expose to the end-user without frightening him.

Definitely! That's why I was poking you the other day about how to an experimental rendering tab for inspector. Its not clear to me what we should tell the end-dev yet, but we definitely need to make something human-consumable available. :)

[Nat Duca]

Created attachment 112259 [details]
Patch

[WebKit Review Bot]

Comment on attachment 112259 [details]
Patch

Attachment 112259 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10200988

[Nat Duca]

Created attachment 112266 [details]
Should build

[Nat Duca]

Ok, it builds. Pavel, can you do the final review?

[Nat Duca]

Committed r98331: <http://trac.webkit.org/changeset/98331>