1360471 - Use a bitfield to represent the profiler's features

Status: RESOLVED FIXED

(Core :: Gecko Profiler, enhancement)

Description

[Nicholas Nethercote [inactive]]

The profiler has a number of features which can be enabled. It mostly uses arrays and vectors of strings to store these, which is an awful representation. I want to change it to a bitfield.

It would be nice to do it everywhere, but changing nsIProfiler is hard because it requires changing the add-on as well. So I will likely just do it within the profiler core and leave nsIProfiler unchanged.

Markus, I think nsIProfilerStartParams and nsIProfiler.startParams are only used by C++ code, which means they can be changed easily. Is that right?

Comment 1

[Markus Stange [:mstange]]

(In reply to Nicholas Nethercote [:njn] from comment #0)
> Markus, I think nsIProfilerStartParams and nsIProfiler.startParams are only
> used by C++ code, which means they can be changed easily. Is that right?

That's correct.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Make nsIProfilerStartParams only usable in C++

The patch also makes the |entries| and |interval| fields readonly, because they
are never changed.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Rename profiler_get_features() as profiler_get_all_features()

It clarifies that it's not just the features chosen in profiler_start().

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Rename various "thread name filters" identifiers as "filters"

The new names are more concise and matches common usage elsewhere (e.g.
profiler_start() arguments).

This patch is similar to bug 1358074 part 5.

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: (part 4) - Use a bitfield to represent profiler features

Currently the profiler mostly uses an array of strings to represent which
features are available and in use. This patch changes the profiler core to use
a uint32_t bitfield, which is a much simpler and faster representation.
(nsProfiler and the profiler add-on still use the array of strings, alas.) The
new ProfilerFeature type defines the values in the bitfield.

One side-effect of this change is that profiler_feature_active() now can be
used to query all features. Previously it was just a subset.

Another side-effect is that profiler_get_all_features() no longer incorrectly
indicates support for Java and stack-walking when they aren't supported. (The
handling of task tracer support is unchanged, because the old code handled it
correctly.)

Comment 6

[Markus Stange [:mstange]]

Comment on attachment 8863302 [details] [diff] [review]
(part 1) - Make nsIProfilerStartParams only usable in C++

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

I'm having second thoughts on this, because for startup profiling it would certainly be nice if the profiler add-on could display the currently active profiler settings (because they're coming from a source outside of the add-on's control), but we can think about that again once we actually add that information to the add-on.

Comment 7

[Markus Stange [:mstange]]

Comment on attachment 8863303 [details] [diff] [review]
(part 2) - Rename profiler_get_features() as profiler_get_all_features()

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

What do you think about the term "available features"? The features we expose depend on the platform, and "all features" could be read to mean "all potential features across all platforms". "Available features" would also mirror the term "available threads" from bug 1321607 nicely.

Comment 8

[Markus Stange [:mstange]]

Comment on attachment 8863305 [details] [diff] [review]
(part 4) - Use a bitfield to represent profiler features

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

::: tools/profiler/core/platform.cpp
@@ +262,5 @@
> +    , mFeatures(
> +        (
> +          aFeatures &
> +          // Filter out any requested but unsupported features.
> +          profiler_get_all_features() &

Could you break this out into a static method? I think that might make it a bit more readable.

static uint32_t FilterRequestedFeatures(uint32_t aRequestedFeatures)
{
  uint32_t availableFeatures = profiler_get_all_features();
#if defined(PROFILE_JAVA)
  // Disable Java profiling in content processes.
  if (!mozilla::jni::IsFennec()) {
    availableFeatures &= ~ProfilerFeature::Java;
  }
#endif
  // Filter out any requested but unsupported features.
  return aRequestedFeatures & availableFeatures;
}

I'm not sure what the mozilla::jni::IsFennec() check is about. We don't have e10s on Fennec yet. And maybe the check should move into profiler_get_all_features?

Comment 9

[Nicholas Nethercote [inactive]]

> What do you think about the term "available features"?

That's good. I will change it.

Comment 10

[Nicholas Nethercote [inactive]]

> I'm not sure what the mozilla::jni::IsFennec() check is about. We don't have
> e10s on Fennec yet.

I'm also not sure what it's for. It's defined in widget/android/jni/Utils.cpp and it seems to be false if java::GeckoThread::IsChildProcess() is false. I will remove the comment I added, though, in case it's wrong.

> And maybe the check should move into profiler_get_all_features?

Not sure... so I'll keep it in the constructor because that matches current behaviour.

Comment 11

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/27f2289c4623727d5b8666ad1aaa23a2ee6a619a
Bug 1360471 (part 1) - Make nsIProfilerStartParams only usable in C++. r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/4fc16f5a148ef525f8113593592fd41470fa7eb6
Bug 1360471 (part 2) - Rename profiler_get_features() as profiler_get_available_features(). r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ec6226c96adc7e2a9c4b171073892c054e393293
Bug 1360471 (part 3) - Rename various "thread name filters" identifiers as "filters". r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/bc25cff90b6ede8d18edfa2d4539c3fc2df3486a
Bug 1360471 (part 4) - Use a bitfield to represent profiler features. r=mstange.

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/27f2289c4623
https://hg.mozilla.org/mozilla-central/rev/4fc16f5a148e
https://hg.mozilla.org/mozilla-central/rev/ec6226c96adc
https://hg.mozilla.org/mozilla-central/rev/bc25cff90b6e