Closed Bug 1429623 Opened 2 years ago Closed 2 years ago

Allow the Gecko Profiler to report tracing instrumentation to VTune

Categories

(Core :: Gecko Profiler, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: bas.schouten, Assigned: bas.schouten)

Details

Attachments

(1 file)

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 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.
Attachment #8941694 - Flags: review?(mstange) → review+
Attachment #8941694 - Flags: review?(ted) → review?(core-build-config-reviews)
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.
Attachment #8941694 - Flags: review-
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
Attachment #8941694 - Flags: review?(ted) → review+
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.
Pushed by bschouten@mozilla.com:
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
Pushed by bschouten@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/8a1199a25c00
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.