Closed Bug 1004899 Opened 8 years ago Closed 8 years ago

Add stderr to profile output

Categories

(Core :: Gecko Profiler, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: BenWa, Assigned: BenWa)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 3 obsolete files)

We're logging a lot of rich data right now like DisplayList/LayersDump but mapping between logcat and profiles is a nightmare.

My goal is to patch gecko so that profiles can receive profile information. Once that's done we can display the logs in cleopatra and annotate the timeline along with it.

Then in a follow up we can allow people to grep for their log output and write a custom visualizer.
Attached image Prototype
Assignee: nobody → bgirard
Status: NEW → ASSIGNED
Attached patch patch (obsolete) — Splinter Review
Attachment #8416595 - Flags: review?(ehsan)
Comment on attachment 8416595 [details] [diff] [review]
patch

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

::: gfx/layers/composite/LayerManagerComposite.cpp
@@ +443,5 @@
>      NS_WARNING("Call on destroyed layer manager");
>      return;
>    }
>  
> +  if (true || gfxPrefs::LayersDump()) {

Please remove this hunk.

::: gfx/thebes/gfxASurface.cpp
@@ +894,5 @@
>           cStr += 140;
>         }
>       }
>  #endif
> +    fprintf_stderr(aFile, "%s", string.BeginReading());

This one too.

::: xpcom/glue/nsCRTGlue.cpp
@@ +275,5 @@
> +{
> +  if (profiler_is_active()) {
> +    char buf[2048];
> +    va_list argsCpy;
> +    va_copy(argsCpy, args);

No need to va_copy this.

@@ +280,5 @@
> +    int required = vsnprintf(buf, sizeof(buf), fmt, argsCpy);
> +    va_end(argsCpy);
> +    buf[sizeof(buf) - 1] = '\0';
> +
> +    if (required < 2048) {

|required| can be negative here, you need to handle that case too I think.

@@ +283,5 @@
> +
> +    if (required < 2048) {
> +      profiler_tracing("log", buf, TRACING_EVENT);
> +    } else {
> +      char* heapBuf = new char[required+1];

You forgot to free this.

@@ +285,5 @@
> +      profiler_tracing("log", buf, TRACING_EVENT);
> +    } else {
> +      char* heapBuf = new char[required+1];
> +      va_list argsCpy2;
> +      va_copy(argsCpy2, args);

No need to va_copy this.

@@ +292,5 @@
> +      // EVENT_BACKTRACE could be used to get a source
> +      // for all log events. This could be a runtime
> +      // flag later.
> +      profiler_tracing("log", heapBuf, TRACING_EVENT);
> +    }

So, why not use nsAutoCString and AppendPrintf instead of all of this?

@@ +313,4 @@
>    if (IsDebuggerPresent()) {
>      char buf[2048];
>      va_list argsCpy;
>      va_copy(argsCpy, args);

You don't need to va_copy this twice.  Just move this to the outer block, and pass argsCpy to both call sites.
Attachment #8416595 - Flags: review?(ehsan) → review-
Ouuch, sorry. Thanks for the review!
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #3)
> @@ +313,4 @@
> >    if (IsDebuggerPresent()) {
> >      char buf[2048];
> >      va_list argsCpy;
> >      va_copy(argsCpy, args);
> 
> You don't need to va_copy this twice.  Just move this to the outer block,
> and pass argsCpy to both call sites.

It's unlikely we will have both a debugger and a profiler attached. It means that we would be doing a single copy in every case.
Attached patch patch v2 (obsolete) — Splinter Review
I tried what you suggested for the va_list but it crashes. I tried a few iterations but I think this is the minimal one. Calling vsnprintf modifies the va_list so it must be copied before using it.
Attachment #8416595 - Attachment is obsolete: true
Attachment #8416711 - Flags: review?(ehsan)
(In reply to comment #5)
> (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> #3)
> > @@ +313,4 @@
> > >    if (IsDebuggerPresent()) {
> > >      char buf[2048];
> > >      va_list argsCpy;
> > >      va_copy(argsCpy, args);
> > 
> > You don't need to va_copy this twice.  Just move this to the outer block,
> > and pass argsCpy to both call sites.
> 
> It's unlikely we will have both a debugger and a profiler attached. It means
> that we would be doing a single copy in every case.

Well, va_copy is only needed across function calls.  You should be able to va_copy once and pass it to multiple functions, what you're not allowed to do is to not va_copy and pass a va_list to another function.  (This is needed because va_list might be implemented as a relative offset to your stack frame.)
va_arg changes the state of the va_list so it's statefull. To be safe I should probably go back to aggressive va_copy to be portable. It should be cheap I imagine?
(In reply to Benoit Girard (:BenWa) from comment #8)
> va_arg changes the state of the va_list so it's statefull. To be safe I
> should probably go back to aggressive va_copy to be portable. It should be
> cheap I imagine?

Ah, yeah, va_arg does change the list...

Don't worry about the cost, I would expect this to be a bunch of pointer copies on most platforms.
Comment on attachment 8416711 [details] [diff] [review]
patch v2

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

::: xpcom/glue/nsCRTGlue.cpp
@@ +288,5 @@
> +      profiler_tracing("log", buf, TRACING_EVENT);
> +    } else {
> +      char* heapBuf = new char[required+1];
> +    va_list argsCpy;
> +    va_copy(argsCpy, args);

Nit: indentation
Attachment #8416711 - Flags: review?(ehsan) → review+
Alright for this to work it can't be in nsCRTGlue.cpp since it gets linked in code where the profiler isn't available. Glandium tells me we have to move the code to mozglue to work around this.
For future reference this was the test push that included this patch and failed:
https://tbpl.mozilla.org/?tree=Try&rev=f81ab48d8670
Attached patch patch v3 (obsolete) — Splinter Review
Attachment #8416711 - Attachment is obsolete: true
Attachment #8417574 - Flags: review?(ehsan)
Comment on attachment 8417574 [details] [diff] [review]
patch v3

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

Please file a follow-up on fixing the linkage issue properly.  r=me with that and the comment below.

::: tools/profiler/platform.cpp
@@ +436,5 @@
>  }
>  
> +#if defined(XP_WIN)
> +#define va_copy(dest, src) (dest = src)
> +#endif

You need to do something like this:

http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsTextFormatter.cpp#40
Attachment #8417574 - Flags: review?(ehsan) → review+
Depends on changes from bug 926922 because it has some profiler changes rolled into it that this uses. Going to wait for that to land.
Depends on: 926922
Attached patch patch v4Splinter Review
Attachment #8417574 - Attachment is obsolete: true
Blocks: 1008243
https://hg.mozilla.org/mozilla-central/rev/ddd5e6e4c6e3
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.