Use a bitfield to represent the profiler's features

RESOLVED FIXED in Firefox 55

Status

()

Core
Gecko Profiler
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: njn, Assigned: njn)

Tracking

Trunk
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(4 attachments)

(Assignee)

Description

a year ago
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?
Flags: needinfo?(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.
Flags: needinfo?(mstange)
(Assignee)

Comment 2

a year ago
Created attachment 8863302 [details] [diff] [review]
(part 1) - Make nsIProfilerStartParams only usable in C++

The patch also makes the |entries| and |interval| fields readonly, because they
are never changed.
Attachment #8863302 - Flags: review?(mstange)
(Assignee)

Updated

a year ago
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
(Assignee)

Comment 3

a year ago
Created attachment 8863303 [details] [diff] [review]
(part 2) - Rename profiler_get_features() as profiler_get_all_features()

It clarifies that it's not just the features chosen in profiler_start().
Attachment #8863303 - Flags: review?(mstange)
(Assignee)

Comment 4

a year ago
Created attachment 8863304 [details] [diff] [review]
(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.
Attachment #8863304 - Flags: review?(mstange)
(Assignee)

Comment 5

a year ago
Created attachment 8863305 [details] [diff] [review]
(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.)
Attachment #8863305 - Flags: review?(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.
Attachment #8863302 - Flags: review?(mstange) → review+
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.
Attachment #8863303 - Flags: review?(mstange) → review+
Attachment #8863304 - Flags: review?(mstange) → review+
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?
Attachment #8863305 - Flags: review?(mstange) → review+
(Assignee)

Comment 9

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

That's good. I will change it.
(Assignee)

Comment 10

a year ago
> 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.
(Assignee)

Comment 11

a year ago
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

a year ago
bugherder
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
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

Updated

a year ago
Depends on: 1362598
You need to log in before you can comment on or make changes to this bug.