1332577 - Remove all mozilla_sampler_* functions

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Description

[Nicholas Nethercote [:njn] (On Long Service Leave)]

A lot of profiler_*() functions dispatch immediately to mozilla_sampler_*() functions. This indirection is unnecessary and I have patches to remove it. The patches also get rid of some headers that are only #included from one place.

Comment 1

[Nicholas Nethercote [:njn] (On Long Service Leave)]

Attachment: (part 1) - Remove PROFILER_MAIN_THREAD_* macros

The comment suggests these are performance-critical specializations. But (a)
they look very similar to all the other macros, i.e. have no obvious
specialization, and (b) they are unused.

Comment 2

[Nicholas Nethercote [:njn] (On Long Service Leave)]

Attachment: (part 2) - Inline and remove GeckoProfilerFunc.h

It's only included once, so there's no point having a separate file.

Comment 3

[Nicholas Nethercote [:njn] (On Long Service Leave)]

Attachment: (part 3) - Inline and remove GeckoProfilerImpl.h

It's only included once, so there's no point having a separate file.

Comment 4

[Nicholas Nethercote [:njn] (On Long Service Leave)]

Attachment: (part 4) - Inline and remove GeckoProfilerTypes.h

It's only included once, so there's no point having a separate file.

Comment 5

[Nicholas Nethercote [:njn] (On Long Service Leave)]

Attachment: (part 5) - Fix indentation in platform.cpp

The patch fixes some 4 space indents and tab indents.

Comment 6

[Nicholas Nethercote [:njn] (On Long Service Leave)]

Attachment: (part 6) - Move some function definitions in GeckoProfiler.h

This patch moves the definitions of profiler_call_{entry,exit}() up so that
forward declarations aren't necessary. It also removes an unnecessary second
declaration of mozilla_sampler_add_marker().

Comment 7

[Nicholas Nethercote [:njn] (On Long Service Leave)]

Attachment: (part 7) - Remove all mozilla_sampler_*() functions

There are lots of profiler_*() functions that simply call onto equivalent or
nearly-equivalent mozilla_sampler_*() functions. This patch removes the
unnecessary indirection by removing the mozilla_sampler_*() functions.

The most important changes:

- In platform.cpp, all the mozilla_sampler_*() definitions are renamed as
  profiler_*().

- In GeckoProfiler.h, the new PROFILER_FUNC{,_VOID} macros provide a neat way
  to declare the functions that must be present whether the profiler is enabled
  or not.

- In GeckoProfiler.h, all the mozilla_sampler_*() declarations are removed, as
  are all the profiler_*() definitions that corresponded to a
  mozilla_sampler_*() function.

Other things of note:

- profiler_log(const char* str) is now defined in platform.cpp, instead of in
  GeckoProfiler.h, for consistency with all the other profiler_*() functions.
  Likewise with profiler_js_operation_callback() and
  profiler_in_privacy_mode().

- ProfilerBacktraceDestructor::operator() is treated slightly different to all
  the profiler_*() functions.

- Both variants of profiler_tracing() got some early-return conditions moved
  into them from GeckoProfiler.h.

- There were some cases where the profiler_*() and mozilla_sampler_*() name
  didn't quite match. Specifically:

  * mozilla_sampler_get_profile_data() and profiler_get_profiler_jsobject():
    name mismatch. Kept the latter.

  * mozilla_sampler_get_profile_data_async() and
    profiler_get_profile_jsobject_async(): name mismatch. Kept the latter.

  * mozilla_sampler_register_thread() and profiler_register_thread(): return
    type mismatch. Changed to void.

  * mozilla_sampler_frame_number() and profiler_set_frame_number(): name
    mismatch. Kept the latter.

  * mozilla_sampler_save_profile_to_file() and
    profile_sampler_save_profile_to_file(): the former was 'extern "C"' so it
    could be called from a debugger easily. The latter now is 'extern "C"'.

- profiler_get_buffer_info() didn't fit the patterns handled by
  PROFILER_FUNC{,VOID}, so the patch makes it call onto the new function
  profiler_get_buffer_info_helper(), which does fit the pattern.

Comment 8

[Nicholas Nethercote [:njn] (On Long Service Leave)]

Attachment: (part 8) - Rename mozilla_get_pseudo_stack() as profiler_get_pseudo_stack()

This makes it consistent with other profiler functions.

Comment 9

[Nicholas Nethercote [:njn] (On Long Service Leave)]

Attachment: (part 9) - Rename moz_profiler_*() as profiler_*()

This makes them consistent with other profiler functions.

Comment 10

[Nicholas Nethercote [:njn] (On Long Service Leave)]

I have compiled the patches with MOZ_ENABLE_PROFILER_SPS enabled and disabled. Overall the patches reduce the number of lines of code by 293.

Comment 11

[Markus Stange [:mstange]]

Comment on attachment 8828705 [details] [diff] [review]
(part 1) - Remove PROFILER_MAIN_THREAD_* macros

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

We had bug 931373 filed to actually do the optimization. We can still bring this back once we actually decide that the optimization is worth doing.

Comment 12

[Markus Stange [:mstange]]

Comment on attachment 8828712 [details] [diff] [review]
(part 7) - Remove all mozilla_sampler_*() functions

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

Thank you! I've wasted a lot of time chasing the indirection here, over the course of the years.

I approve of all your choices in the mismatch resolutions. I also like the PROFILER_FUNC macro idea.

Comment 13

[Markus Stange [:mstange]]

So what's next? Should we put these functions into a mozilla::profiler namespace and make them use CamelCase? :)

Comment 14

[Nathan Froyd [:froydnj]]

(In reply to Markus Stange [:mstange] from comment #13)
> So what's next? Should we put these functions into a mozilla::profiler
> namespace and make them use CamelCase? :)

Over in bug 1322656, we'd like to make the profiler functions callable from Rust so that Stylo-enabled builds can provide profiler information.  I'm not sure whether the right thing is to make all of the current profiler functions |extern "C"| (which I think means not having them return non-POD objects, so some of the UniquePtr work would have to be undone) or writing |extern "C"| wrapper functions and providing for Rust's purposes.

I think the second option is the best, so a mozilla::profiler namespace would be OK, but if you have better options, I'd love to hear them!

Comment 15

[Markus Stange [:mstange]]

(In reply to Nathan Froyd [:froydnj] from comment #14)
> I think the second option is the best

I agree!

Comment 16

[Nicholas Nethercote [:njn] (On Long Service Leave)]

> |extern "C"| wrapper functions

So I just removed a bunch of wrapper functions and now we're going to introduce a bunch of new ones?

Comment 17

[Nicholas Nethercote [:njn] (On Long Service Leave)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cd3cab137228831aa9f48d711e9b24ff9fa09fc8
Bug 1332577 (part 1) - Remove PROFILER_MAIN_THREAD_* macros. r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c2ebea71ca97f6022407e32389ea70c0752d3458
Bug 1332577 (part 2) - Inline and remove GeckoProfilerFunc.h. r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/814905af775483901d36ea02eef8c93c9a90688e
Bug 1332577 (part 3) - Inline and remove GeckoProfilerImpl.h. r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/5601e95a30ff9d8bcbb66bcc6554c2a9fc03fde5
Bug 1332577 (part 4) - Inline and remove GeckoProfilerTypes.h. r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/03d2b4964c1604f1331733f79b78ef532afc452d
Bug 1332577 (part 5) - Fix indentation in platform.cpp. r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/6f893e5c9e874300e4bc5523e821e8b3948bb80d
Bug 1332577 (part 6) - Move some function definitions in GeckoProfiler.h. r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/1e467949bdad0839aca0ec6b51a6d1b8f8e25226
Bug 1332577 (part 7) - Rename mozilla_get_pseudo_stack() as profiler_get_pseudo_stack(). r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/6c6ecff45a92f07925d9fae12681f2a143efd392
Bug 1332577 (part 8) - Rename moz_profiler_*() as profiler_*(). r=mstange.

Comment 18

[Nicholas Nethercote [:njn] (On Long Service Leave)]

I landed parts 1--6 and 8--9 (with the latter two renamed as 7--8). The old part 7 (which is the most complex and important patch) is causing some devtools test failures which I will investigate on Monday.

Comment 19

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/cd3cab137228
https://hg.mozilla.org/mozilla-central/rev/c2ebea71ca97
https://hg.mozilla.org/mozilla-central/rev/814905af7754
https://hg.mozilla.org/mozilla-central/rev/5601e95a30ff
https://hg.mozilla.org/mozilla-central/rev/03d2b4964c16
https://hg.mozilla.org/mozilla-central/rev/6f893e5c9e87
https://hg.mozilla.org/mozilla-central/rev/1e467949bdad
https://hg.mozilla.org/mozilla-central/rev/6c6ecff45a92

Comment 20

[Nicholas Nethercote [:njn] (On Long Service Leave)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5267191b229f425a34f26f0d7a96fd5391f3105d
Bug 1332577 (part 9) - Remove all mozilla_sampler_*() functions. r=mstange.

Comment 21

[Nicholas Nethercote [:njn] (On Long Service Leave)]

(In reply to Nicholas Nethercote [:njn] from comment #18)
> The old part 7 (which is the most complex and important patch) is causing some
> devtools test failures which I will investigate on Monday.

Turns out I forgot to call profiler_get_buffer_info_helper() from profiler_get_buffer_info().

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5267191b229f

Comment 23

[Markus Stange [:mstange]]

(In reply to Nicholas Nethercote [:njn] from comment #16)
> > |extern "C"| wrapper functions
> 
> So I just removed a bunch of wrapper functions and now we're going to
> introduce a bunch of new ones?

If those wrappers are just part of the binding glue code, I don't think it'll be too bad. It's better than restricting ourselves to using POD types for any function that might be needed for rust code.

Comment 24

[Nathan Froyd [:froydnj]]

(In reply to Markus Stange [:mstange] from comment #23)
> (In reply to Nicholas Nethercote [:njn] from comment #16)
> > > |extern "C"| wrapper functions
> > 
> > So I just removed a bunch of wrapper functions and now we're going to
> > introduce a bunch of new ones?
> 
> If those wrappers are just part of the binding glue code, I don't think
> it'll be too bad. It's better than restricting ourselves to using POD types
> for any function that might be needed for rust code.

Indeed.  We could also use bindgen to generate the necessary Rust bindings automatically?