Remove all mozilla_sampler_* functions

RESOLVED FIXED in Firefox 53

Status

()

Core
Gecko Profiler
RESOLVED FIXED
3 months ago
3 months ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla53
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(9 attachments)

(Assignee)

Description

3 months ago
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.
(Assignee)

Comment 1

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

Updated

3 months ago
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
(Assignee)

Comment 2

3 months ago
Created attachment 8828706 [details] [diff] [review]
(part 2) - Inline and remove GeckoProfilerFunc.h

It's only included once, so there's no point having a separate file.
Attachment #8828706 - Flags: review?(mstange)
(Assignee)

Comment 3

3 months ago
Created attachment 8828707 [details] [diff] [review]
(part 3) - Inline and remove GeckoProfilerImpl.h

It's only included once, so there's no point having a separate file.
Attachment #8828707 - Flags: review?(mstange)
(Assignee)

Comment 4

3 months ago
Created attachment 8828709 [details] [diff] [review]
(part 4) - Inline and remove GeckoProfilerTypes.h

It's only included once, so there's no point having a separate file.
Attachment #8828709 - Flags: review?(mstange)
(Assignee)

Comment 5

3 months ago
Created attachment 8828710 [details] [diff] [review]
(part 5) - Fix indentation in platform.cpp

The patch fixes some 4 space indents and tab indents.
Attachment #8828710 - Flags: review?(mstange)
(Assignee)

Comment 6

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

Comment 7

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

Comment 8

3 months ago
Created attachment 8828713 [details] [diff] [review]
(part 8) - Rename mozilla_get_pseudo_stack() as profiler_get_pseudo_stack()

This makes it consistent with other profiler functions.
Attachment #8828713 - Flags: review?(mstange)
(Assignee)

Comment 9

3 months ago
Created attachment 8828716 [details] [diff] [review]
(part 9) - Rename moz_profiler_*() as profiler_*()

This makes them consistent with other profiler functions.
Attachment #8828716 - Flags: review?(mstange)
(Assignee)

Comment 10

3 months ago
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 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.
Attachment #8828705 - Flags: review?(mstange) → review+
Attachment #8828706 - Flags: review?(mstange) → review+
Attachment #8828707 - Flags: review?(mstange) → review+
Attachment #8828709 - Flags: review?(mstange) → review+
Attachment #8828710 - Flags: review?(mstange) → review+
Attachment #8828711 - Flags: review?(mstange) → review+
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.
Attachment #8828712 - Flags: review?(mstange) → review+
Attachment #8828713 - Flags: review?(mstange) → review+
Attachment #8828716 - Flags: review?(mstange) → review+
So what's next? Should we put these functions into a mozilla::profiler namespace and make them use CamelCase? :)
(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!
(In reply to Nathan Froyd [:froydnj] from comment #14)
> I think the second option is the best

I agree!
(Assignee)

Comment 16

3 months ago
> |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?
(Assignee)

Comment 17

3 months ago
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.
(Assignee)

Comment 18

3 months ago
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.
(Assignee)

Updated

3 months ago
Keywords: leave-open

Comment 19

3 months ago
bugherder
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
(Assignee)

Comment 20

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

Comment 21

3 months ago
(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().
Keywords: leave-open

Comment 22

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5267191b229f
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
(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.
(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?
You need to log in before you can comment on or make changes to this bug.