1635350 - Remove MOZ_GECKO_PROFILER

Status: NEW

(Core :: Gecko Profiler, task, P3)

Description

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Bug 1634784 will remove MOZ_BASE_PROFILER, and the profiler now works on all Firefox-supported platforms, and even on FreeBSD soon (bug 1634205).
So we could remove MOZ_GECKO_PROFILER and always build the profiler.

This would simplify quite a few things, including not having to use magic macros like PROFILER_ADD_MARKER that may translate to nothing.

Unsupported platforms could still build by providing no-op stack-sampling/walking.

And the --disable-profiling may still be needed when running instrumented build (e.g.: ASAN, TSAN, etc.), in which case we could make this option define a MOZ_DISABLE_PROFILING macro instead, and use no-op stack-sampling/walking in this case.

Comment 1

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Bug 1637895 also shows that "just" removing MOZ_GECKO_PROFILER would hurt unsupported platforms, so we'll need to make sure these platforms can still build (through --disable-profiling or fallback no-op implementation of platform-specific functions).

Comment 2

[Florian Quèze [:florian]]

A big benefit of avoiding MOZ_GECKO_PROFILER ifdefs would be to reduce frequency of bustage by simplifying the code using the profiler. I looked at a few ifdefs we have in the tree, and lots (of not most) of them seem to be due to calling functions like profiler_feature_active, profiler_is_active, profiler_can_accept_markers, or profiler_thread_is_being_profiled before code that collects data for a marker. I think it would be straightforward to define these functions as inline functions returning false when MOZ_GECKO_PROFILER isn't defined. I'm hoping this would give most of the benefits of always building the profiler, but require little effort; and this can be a first step towards always building the profiler.

Comment 3

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Good idea Florian.

I sort-of did that for a few functions, e.g., profiler_capture_backtrace() returns null.

Ideally we would keep all MOZ_GECKO_PROFILER declarations from Base/GeckoProfiler.h (but without the MOZ_GECKO_PROFILER anymore), and we could have a special platform-noop.cpp that gets compiled instead of platform.cpp, where we would define these no-op functions. We could make them inline if we wanted to.

What to do with the macros though? E.g., AUTO_PROFILER_LABEL is empty in non-MOZ_GECKO_PROFILER builds, which is most efficient. Should it still be empty on some platforms (but that could re-create build issues), or should it be defined the same way everywhere, but the implementation of class AutoProfilerLabel could be different by platform?

In the end I'm afraid that whatever we do, there will still be issues if we don't have CI tasks that can ring the alarm as soon as we land something that breaks unsupported platforms, instead of waiting for a 3rd party to tell us. If we had the no-op profiler fully defined inline in headers (as you suggested), I think we could create a cppunit test that includes just that and can simulate an unsupported-platform build. 🤔

(It took me some time to write all this, with different ideas popping up and being discarded as I wrote more, showing that it's not clear to me yet which is the best way to proceed. Happy to chat further...)

Comment 4

[Florian Quèze [:florian]]

Attachment: List of remaining MOZ_GECKO_PROFILER ifdefs

I looked at the remaining ifdefs, here is a break down of why they are (still) here.