1429623 - Allow the Gecko Profiler to report tracing instrumentation to VTune

Status: RESOLVED FIXED

(Core :: Gecko Profiler, enhancement)

Description

[Bas Schouten (:bas.schouten)]

VTune offers an API to allow instrumentation of programs, see: https://software.intel.com/en-us/vtune-amplifier-help-instrumentation-and-tracing-technology-apis. This can be used to display markers in the program timeline for things like rasterization and style processing. As well as allowing numerous other task/event based features inside the VTune UI. Since we already collect some of this information for the Gecko profiler, it should be pretty easy to add a build flag that allows reporting this information to VTune.

Comment 1

[Bas Schouten (:bas.schouten)]

Attachment: Bug 1429623: Report tracing events and thread registration to VTune when --enable-vtune-instrumentation is enabled.

Review commit: https://reviewboard.mozilla.org/r/211936/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/211936/

Comment 2

[Markus Stange [:mstange]]

Comment on attachment 8941694 [details]
Bug 1429623: Report tracing events and thread registration to VTune when --enable-vtune-instrumentation is enabled.

https://reviewboard.mozilla.org/r/211936/#review217774

Looks good!

::: toolkit/moz.configure:41
(Diff revision 1)
> +option('--enable-vtune-instrumentation', env='MOZ_VTUNE_PROFILING',
> +       help='Enable vtune instrumentation API')
> +
> +@depends('--enable-vtune-instrumentation')
> +def vtune_instrumentation(value):
> +    if value:
> +        return True
> +
> +set_config('MOZ_VTUNE_INSTRUMENTATION', vtune_instrumentation)
> +set_define('MOZ_VTUNE_INSTRUMENTATION', vtune_instrumentation)
> +imply_option('--enable-profiling', vtune_instrumentation)
> +imply_option('--enable-vtune', vtune_instrumentation)

This part probably needs review from a build peer as well.

::: tools/profiler/core/VTuneProfiler.h:44
(Diff revision 1)
> +  static void Trace(const char* aName, TracingKind aKind) { if (mInstance) { mInstance->TraceInternal(aName, aKind); } }
> +  static void RegisterThread(const char* aName) { if (mInstance) { mInstance->RegisterThreadInternal(aName); } }

These lines are a bit long.

::: tools/profiler/core/VTuneProfiler.cpp:68
(Diff revision 1)
> +    case GeckoProcessType::GeckoProcessType_Default:
> +      __itt_thread_set_nameA("Main Process");
> +      break;
> +    case GeckoProcessType::GeckoProcessType_Content:
> +      __itt_thread_set_nameA("Content Process");
> +      break;
> +    case GeckoProcessType::GeckoProcessType_GMPlugin:
> +      __itt_thread_set_nameA("Plugin Process");
> +      break;
> +    case GeckoProcessType::GeckoProcessType_GPU:
> +      __itt_thread_set_nameA("GPU Process");
> +      break;
> +    default:
> +      __itt_thread_set_nameA("Unknown Process");

I think Gecko style wants one more level of indentation here.

Comment 3

[Bas Schouten (:bas.schouten)]

Comment on attachment 8941694 [details]
Bug 1429623: Report tracing events and thread registration to VTune when --enable-vtune-instrumentation is enabled.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/211936/diff/1-2/

Comment 4

[(not currently active) Ted Mielczarek]

Comment on attachment 8941694 [details]
Bug 1429623: Report tracing events and thread registration to VTune when --enable-vtune-instrumentation is enabled.

https://reviewboard.mozilla.org/r/211936/#review218986

I'm not convinced we need a new build option for this.

::: toolkit/moz.configure:41
(Diff revision 2)
>          return True
>  
>  set_config('MOZ_GECKO_PROFILER', gecko_profiler_define)
>  set_define('MOZ_GECKO_PROFILER', gecko_profiler_define)
>  
> +option('--enable-vtune-instrumentation', env='MOZ_VTUNE_PROFILING',

Is there any actual benefitto making this a separate build option? Are there going to be people that want to build `--enable-vtune` but not `--enable-vtune-instrumentation`? It seems like you could just lump this under `--enable-vtune`.

::: toolkit/moz.configure:50
(Diff revision 2)
> +def vtune_instrumentation(value):
> +    if value:
> +        return True
> +
> +set_config('MOZ_VTUNE_INSTRUMENTATION', vtune_instrumentation)
> +set_define('MOZ_VTUNE_INSTRUMENTATION', vtune_instrumentation)

If this define is only being used in tools/profiler, I'd recommend just setting it in that moz.build, like:
```
if CONFIG['MOZ_VTUNE_INSTRUMENTATION']:
  DEFINES['MOZ_VTUNE_INSTRUMENTATION'] = True
```

Adding global defines means that changing this build option will wind up rebuilding the world, where a local define would only have to rebuild the objects in tools/profiler.

Comment 5

[Bas Schouten (:bas.schouten)]

Comment on attachment 8941694 [details]
Bug 1429623: Report tracing events and thread registration to VTune when --enable-vtune-instrumentation is enabled.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/211936/diff/2-3/

Comment 6

[Bas Schouten (:bas.schouten)]

Comment on attachment 8941694 [details]
Bug 1429623: Report tracing events and thread registration to VTune when --enable-vtune-instrumentation is enabled.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/211936/diff/3-4/

Comment 7

[Bas Schouten (:bas.schouten)]

Comment on attachment 8941694 [details]
Bug 1429623: Report tracing events and thread registration to VTune when --enable-vtune-instrumentation is enabled.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/211936/diff/4-5/

Comment 8

[(not currently active) Ted Mielczarek]

Comment on attachment 8941694 [details]
Bug 1429623: Report tracing events and thread registration to VTune when --enable-vtune-instrumentation is enabled.

https://reviewboard.mozilla.org/r/211936/#review220026

Comment 9

[Mike Hommey [:glandium]]

Comment on attachment 8941694 [details]
Bug 1429623: Report tracing events and thread registration to VTune when --enable-vtune-instrumentation is enabled.

https://reviewboard.mozilla.org/r/211936/#review220074

::: commit-message-21504:1
(Diff revision 5)
> +Bug 1429623: Report tracing events and thread registration to VTune when --enable-vtune-instrumentation is enabled. r=mstange r=ted

You still have references here and in some file to an option that now doesnt exist.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b3656db42a7
Report tracing events and thread registration to VTune when --enable-vtune is enabled. r=mstange r=ted

Comment 11

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/af59a3ed2fcc
Backout 9b3656db42a7 for bustage.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a1199a25c00
Report tracing events and thread registration to VTune when --enable-vtune is enabled. r=mstange r=ted

Comment 13

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/8a1199a25c00