850833 - Builds now print this at startup: "Profiler: ----------------- MOZ_PROFILER_NEW not set -----------------"

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Description

[Daniel Holbert [:dholbert]]

Starting recently, my (debug) build spams this to stderr or stdout on startup:

> Profiler: ----------------- MOZ_PROFILER_NEW not set -----------------

Looks like this is from this line of MXR:
https://mxr.mozilla.org/mozilla-central/source/tools/profiler/TableTicker.cpp#1037
which was added in bug 779291.

Do we really want that logging code in m-c, or was it just for debugging while the patch was being developed?

If we want to keep it, it should be using the prlog.h system so that it can be conditionally enabled using environmental vars.

Comment 1

[Daniel Holbert [:dholbert]]

[oops, didn't mean to assign this to myself]

jseward, can you take this? Looks like you wrote the code in question, so you're in the best position to know whether / when it's needed.

Comment 2

[Daniel Holbert [:dholbert]]

alternately, there are rumors in #developers that BenWa is about to clean this up.

Comment 3

[Benoit Girard (:BenWa)]

Attachment: patch - Turn off logging on desktop, introduce SPS_FORCE_LOG

Comment 4

[(no longer active)]

Comment on attachment 725036 [details] [diff] [review]
patch - Turn off logging on desktop, introduce SPS_FORCE_LOG

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

::: tools/profiler/platform.h
@@ +28,5 @@
>  
>  #ifndef TOOLS_PLATFORM_H_
>  #define TOOLS_PLATFORM_H_
>  
> +//#define SPS_FORCE_LOG

Perhaps change this to a comment saying that defining SPS_FORCE_LOG will give you logging, or something.

Comment 5

[Benoit Girard (:BenWa)]

For the record I didn't use prlog because we have to create a log module which I didn't feel like further adding to the cost for logging that is rarely used and it's not convenient. If someone wants runtime logging in the future feel to reverse this.

Comment 6

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f766a0b46868

Comment 7

[Daniel Holbert [:dholbert]]

[canceling comment 1's needinfo request]

Comment 8

[Benoit Girard (:BenWa)]

unused function warning fix:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d96ddb6f022

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f766a0b46868
https://hg.mozilla.org/mozilla-central/rev/9d96ddb6f022

Comment 10

[Julian Seward [:jseward]]

Sorry to arrive late to the party.

This does successfully kill off all the SPS-side logging, but it's kinda
ungood from a debugging-the-profiler point of view.  How would people feel
about changing it from a #define in a header file, to being controlled by
an environment variable, possibly MOZ_PROFILER_LOG, to be consistent
with other profiler control vars?

If MOZ_PROFILER_LOG is unset it remains silent (as now).  Setting it to any
value will cause it to behave as it did before this patch landed.

If that would be considered acceptable I will happily make a patch.

Comment 11

[Julian Seward [:jseward]]

Abovementioned patch is in bug 857242.

Comment 12

[(no longer active)]

(In reply to comment #10)
> Sorry to arrive late to the party.
> 
> This does successfully kill off all the SPS-side logging, but it's kinda
> ungood from a debugging-the-profiler point of view.  How would people feel
> about changing it from a #define in a header file, to being controlled by
> an environment variable, possibly MOZ_PROFILER_LOG, to be consistent
> with other profiler control vars?
> 
> If MOZ_PROFILER_LOG is unset it remains silent (as now).  Setting it to any
> value will cause it to behave as it did before this patch landed.
> 
> If that would be considered acceptable I will happily make a patch.

Yes, that is acceptable.