Closed Bug 850833 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: Gecko Profiler, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: dholbert, Assigned: BenWa)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.
[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.
Assignee: dholbert → nobody
Flags: needinfo?(jseward)
alternately, there are rumors in #developers that BenWa is about to clean this up.
Assignee: nobody → bgirard
Attachment #725036 - Flags: review?(ehsan)
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.
Attachment #725036 - Flags: review?(ehsan) → review+
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.
[canceling comment 1's needinfo request]
Flags: needinfo?(jseward)
https://hg.mozilla.org/mozilla-central/rev/f766a0b46868
https://hg.mozilla.org/mozilla-central/rev/9d96ddb6f022
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Blocks: fx-noise
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.
Abovementioned patch is in bug 857242.
(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.
You need to log in before you can comment on or make changes to this bug.