Closed Bug 1375392 Opened 2 years ago Closed 2 years ago

Tweak the PROFILER_LABEL* macros

Categories

(Core :: Gecko Profiler, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: njn, Assigned: njn)

Details

Attachments

(1 file)

The PROFILER_LABEL* macros can be improved. Apologies in advance for the huge
and tedious patch.
This patch makes the following changes to the macros.

- Removes PROFILER_LABEL_FUNC. It's only suitable for use in functions outside
  classes, due to PROFILER_FUNCTION_NAME not getting class names, and it was
  mostly misused.

- Removes PROFILER_FUNCTION_NAME. It's no longer used, and __func__ is
  universally available now anyway.

- Combines the first two string literal arguments of PROFILER_LABEL and
  PROFILER_LABEL_DYNAMIC into a single argument. There was no good reason for
  them to be separate, and it forced a '::' in the label, which isn't always
  appropriate. Also, the meaning of the "name_space" argument was interpreted
  in an interesting variety of ways.

- Adds an "AUTO_" prefix to PROFILER_LABEL and PROFILER_LABEL_DYNAMIC, to make
  it clearer they construct RAII objects rather than just being function calls.
  (I myself have screwed up the scoping because of this in the past.)

- Fills in the 'js::ProfileEntry::Category::' qualifier within the macro, so
  the caller doesn't need to. This makes a *lot* more of the uses fit onto a
  single line.

The patch also makes the following changes to the macro uses (beyond those
required by the changes described above).

- Fixes a bunch of labels that had gotten out of sync with the name of the
  class and/or function that encloses them.

- Removes a useless PROFILER_LABEL use within a trivial scope in
  EventStateManager::DispatchMouseOrPointerEvent(). It clearly wasn't serving
  any useful purpose. It also serves as extra evidence that the AUTO_ prefix is
  a good idea.

- Tweaks DecodePool::SyncRunIf{Preferred,Possible} so that the labelling is
  done within them, instead of at their callsites, because that's a more
  standard way of doing things.
Attachment #8880258 - Flags: review?(mstange)
(In reply to Nicholas Nethercote [:njn] from comment #1)
> - Removes a useless PROFILER_LABEL use within a trivial scope in
>   EventStateManager::DispatchMouseOrPointerEvent(). It clearly wasn't serving
>   any useful purpose.

That's a fun one. Looks like this one started out correct but then got broken in https://hg.mozilla.org/mozilla-central/rev/37f1942487842f8eeccacda76425b96532007d72#l2.128
Attachment #8880258 - Flags: review?(mstange) → review+
(In reply to Nicholas Nethercote [:njn] from comment #1)

> - Fixes a bunch of labels that had gotten out of sync with the name of the
>   class and/or function that encloses them.

For the labels that I added, it was intentional, to have a name that would me meaningful to front-end engineers.
eg. "Components.utils.import" is readable, "nsXPCComponents_Utils::Import" exposes an implementation detail.
"LoadSubScript" is also a lot more meaningful than "DoLoadSubScriptWithOptions" when the JS call triggering this was "Services.scriptloader.loadSubScript"
https://hg.mozilla.org/mozilla-central/rev/aa1693a26a15
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.