Process names, and predictable thread and process ordering in about:tracing

Process names, and predictable thread and process ordering in about:tracing

R=dsinclair

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=210437

[nduca, 2013-06-20 09:13:25]

wip, but worth a quick look as a sanity check

[dsinclair, 2013-06-20 15:01:47]

https://codereview.chromium.org/17451017/diff/1/content/gpu/gpu_main.cc
File content/gpu/gpu_main.cc (right):

https://codereview.chromium.org/17451017/diff/1/content/gpu/gpu_main.cc#newco...
content/gpu/gpu_main.cc:88:
base::debug::TraceLog::GetInstance()->SetProcessSortIndex(3);
Would it be useful to move some of these magic numbers into trace_event.h?

kGpuSortIndex, kBrowserSortIndex, etc? Keeps them major ones in one place so we
can easily see the sort order.

https://codereview.chromium.org/17451017/diff/1/content/renderer/render_view_...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/17451017/diff/1/content/renderer/render_view_...
content/renderer/render_view_impl.cc:959: reinterpret_cast<int>(this),
std::string());
The render view uses the pointer address as the number, the other processes all
use 0, could we make them consistently use the pointer address?

https://codereview.chromium.org/17451017/diff/1/content/renderer/renderer_mai...
File content/renderer/renderer_main.cc (right):

https://codereview.chromium.org/17451017/diff/1/content/renderer/renderer_mai...
content/renderer/renderer_main.cc:112:
static_cast<int>(base::PlatformThread::CurrentId()), -1);
If we're always call SetThreadSortIndex from the thread, we could move the
base::PlatformTrhead::CurrentId() into the method itself, keeps the signature
the same as the SetProcessSortIndex method which is nice.

[nduca, 2013-06-21 10:10:21]

I moved the order values to a content level constants file. I think that keeps
things appropriately localized.

> The render view uses the pointer address as the number, the other processes
all
> use 0, could we make them consistently use the pointer address?
I ended up switching processes to have both names and labels. I think it makes
it clearer, and lets the UI do "Renderer: google search, gmail" rather then
"Renderer, Google search, gmail"

> static_cast<int>(base::PlatformThread::CurrentId()), -1);
I moved the API to take a PlatformThreadId. Its cleaner. But, I still store int
internally in case we ever need to give one of the fake threads sort order.

[dsinclair, 2013-06-21 14:22:51]

https://codereview.chromium.org/17451017/diff/20001/base/debug/trace_event_im...
File base/debug/trace_event_impl.cc (right):

https://codereview.chromium.org/17451017/diff/20001/base/debug/trace_event_im...
base/debug/trace_event_impl.cc:1306: int num_args = 1;
nit: indenting

https://codereview.chromium.org/17451017/diff/20001/base/debug/trace_event_im...
base/debug/trace_event_impl.cc:1361: }
Not sure if this is possible but, could we use a Convertable here?

Instead of adding N metadata events, we add 1 per process. It would have a name
of 'process', and would be convertable. When converted we'd get something like:

"args": {
  "sort_index": 1,
  "name": "Renderer",
  "labels": [ "Gmail", "Docs" ],
  "threads": {
    "12345": {
      "name": "thread",
      "sort_index": 7
    },
    "2345": {
      "name": "io",
      "sort_index": -100
    }
  }
}

https://codereview.chromium.org/17451017/diff/20001/base/debug/trace_event_im...
base/debug/trace_event_impl.cc:1412: return RemoveProcessLabel(label_id);
I think this should be a DCHECK. We have RemoveProcessLabel so you shouldn't be
using UpdateProcessLabel for this purpose.

https://codereview.chromium.org/17451017/diff/20001/base/debug/trace_event_im...
base/debug/trace_event_impl.cc:1421: if (it == process_labels_.end())
DCHECK?

https://codereview.chromium.org/17451017/diff/20001/base/debug/trace_event_im...
File base/debug/trace_event_impl.h (right):

https://codereview.chromium.org/17451017/diff/20001/base/debug/trace_event_im...
base/debug/trace_event_impl.h:460: void SetProcessName(const std::string&
current_label);
s/current_label/name/

https://codereview.chromium.org/17451017/diff/20001/base/debug/trace_event_im...
base/debug/trace_event_impl.h:460: void SetProcessName(const std::string&
current_label);
Should probably mention that this string needs to live forever.

https://codereview.chromium.org/17451017/diff/20001/base/debug/trace_event_im...
base/debug/trace_event_impl.h:464: void UpdateProcessLabel(int label_id, const
std::string& current_label);
Should probably mention living forever here as well.

[nduca, 2013-07-03 06:45:08]

>
https://codereview.chromium.org/17451017/diff/20001/base/debug/trace_event_im...
> base/debug/trace_event_impl.cc:1361: }
> Not sure if this is possible but, could we use a Convertable here?

I'd actually rather keep it separate. The way its done, we can explain each each
metadata "name" and its meaning, rahter than having to explain a complex object
hierarchy.

It also keeps us open to sending labels immediately when we get them, when
tracing is on. Which is something I'm thinking we might eventually do. When its
a one-off data structure, it'd be a more disruptive change to do that.

>
https://codereview.chromium.org/17451017/diff/20001/base/debug/trace_event_im...
> base/debug/trace_event_impl.h:460: void SetProcessName(const std::string&
> current_label);
> Should probably mention that this string needs to live forever.
Why? We take it and put it into a std::string...

[dsinclair, 2013-07-03 15:10:41]

lgtm 


On 2013/07/03 06:45:08, nduca wrote:
> >
>
https://codereview.chromium.org/17451017/diff/20001/base/debug/trace_event_im...
> > base/debug/trace_event_impl.cc:1361: }
> > Not sure if this is possible but, could we use a Convertable here?
> 
> I'd actually rather keep it separate. The way its done, we can explain each
each
> metadata "name" and its meaning, rahter than having to explain a complex
object
> hierarchy.
> 
> It also keeps us open to sending labels immediately when we get them, when
> tracing is on. Which is something I'm thinking we might eventually do. When
its
> a one-off data structure, it'd be a more disruptive change to do that.
> 

Ah, good point.

> >
>
https://codereview.chromium.org/17451017/diff/20001/base/debug/trace_event_im...
> > base/debug/trace_event_impl.h:460: void SetProcessName(const std::string&
> > current_label);
> > Should probably mention that this string needs to live forever.
> Why? We take it and put it into a std::string...

Oh, I was assuming it worked like other parts of the API where we just keep a
pointer.

[nduca, 2013-07-03 19:23:19]

+piman for content

[piman, 2013-07-03 19:35:55]

https://codereview.chromium.org/17451017/diff/33001/content/common/content_co...
File content/common/content_constants_internal.cc (right):

https://codereview.chromium.org/17451017/diff/33001/content/common/content_co...
content/common/content_constants_internal.cc:14: const int
kTraceEventGpuProcessSortIndex = -1;
neato. Should we also add this for Pepper (ppapi_plugin_main.cc), maybe also
workers/utility/PPAPI broker?

https://codereview.chromium.org/17451017/diff/33001/content/renderer/render_v...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/17451017/diff/33001/content/renderer/render_v...
content/renderer/render_view_impl.cc:972: reinterpret_cast<int>(this));
nit: Is the label id just an arbitrary unique, per-process id?
Should we use the routind_id which should have this property? Feels a bit less
of a hack.

[nduca, 2013-07-03 19:47:19]

great ideas, all used in latest patch.

[piman, 2013-07-03 19:53:21]

lgtm

[commit-bot: I haz the power, 2013-07-08 20:13:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nduca@chromium.org/17451017/63001

[commit-bot: I haz the power, 2013-07-08 23:14:42]

Change committed as 210437

[scottmg, 2013-07-09 00:08:14]

This broke "Win Split" on the waterfall, but I'm going to delete that from the
waterfall for anyone looking here.

[nduca, 2013-07-09 08:10:23]

Do I need to fix anything Scott? Happy to do whatever neeeds doing, definitely
didn't mean to mess you up.