1170314 - Make console.timeStamp to add also Gecko profiler markers if Gecko profiler is active

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v1

...or we could use some other method too, but I guess reusing the existing one should be fine, at least for now.


do_GetService is a bit slow call, so want to cache the service.

Comment 1

[Benoit Girard (:BenWa)]

Comment on attachment 8613704 [details] [diff] [review]
v1

Review of attachment 8613704 [details] [diff] [review]:
-----------------------------------------------------------------

Profiler integration looks good. This matches with chrome. (unrelated) It would be nice someday to standardize a devtools performance API IMO.

This is a DOM change so, at your discretion, consider having a DOM peer review this.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8613704 [details] [diff] [review]
v1

baku, I need some way to add markers to Gecko profiler
(not the builtin devtools profiler), and this seemed to be ok-ish way to
use existing method.

Comment 3

[Jordan Santell [:jsantell] [@jsantell]]

DevTools performance profiler doesn't use the same markers, so this won't affect that tool.

If you want to piggyback the logic used for the perf tools' timeStamp marker, you can add the block to TimestampTimelineMarker's constructor, if that's possible, so we keep our marker generators in one place

Comment 4

[Benoit Girard (:BenWa)]

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #3)
> If you want to piggyback the logic used for the perf tools' timeStamp
> marker, you can add the block to TimestampTimelineMarker's constructor, if
> that's possible, so we keep our marker generators in one place

I don't think that it would be simpler than what we're doing here since just sending the string through without reusing any of the existing meta data. It's just one line of code.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Activating devtools seem to actually affect to performance profiles so much, that I don't want this code to rely on that stuff.

Comment 6

[Jordan Santell [:jsantell] [@jsantell]]

The DT marker would only trigger if the docshell is set to record, so I was wrong, probably best to keep it out of that

Comment 7

[Andrea Marchesini [:baku]]

Comment on attachment 8613704 [details] [diff] [review]
v1

Review of attachment 8613704 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/Console.cpp
@@ +870,5 @@
> +      mProfiler = do_GetService("@mozilla.org/tools/profiler;1");
> +    }
> +    if (mProfiler) {
> +      bool active = false;
> +      mProfiler->IsActive(&active);

ok... in theory it cannot fail, but we don't know how this code evolve.
I prefer:

nsresult rv = mProfiler->IsActive(&active);
if (NS_SUCCEEDED(mProfiler->IsActive(&active)) && active) {

or otherwise we should add 

%{C++
inline bool IsActive() {
  bool active;
  return NS_SUCCEEDED(IsActive(&active)) && active;
}
%}

in nsIProfiler.idl.

::: dom/base/Console.h
@@ +18,5 @@
>  #include "nsPIDOMWindow.h"
>  
>  class nsIConsoleAPIStorage;
>  class nsIXPConnectJSObjectHolder;
> +class nsIProfiler;

alphabetic order.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v2

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3f8269d2b491

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3f8269d2b491