Remove all mozilla_sampler_* functions

RESOLVED FIXED in Firefox 53

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla53
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(9 attachments)

(Assignee)

Description

2 years 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

2 years ago
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

2 years ago
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
(Assignee)

Comment 2

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

Comment 3

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

Comment 4

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

Comment 5

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

Comment 6

2 years ago
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

2 years ago
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

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

Comment 9

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

Comment 10

2 years 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

2 years 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 18

2 years 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

2 years ago
Keywords: leave-open
(Assignee)

Comment 21

2 years 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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5267191b229f
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years 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.