Closed Bug 1369276 Opened 2 years ago Closed 2 years ago

Simplify flag handling in ProfileEntry

Categories

(Core :: Gecko Profiler, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: njn, Assigned: njn)

Details

Attachments

(2 files)

ProfileEntry's flags are a bit of a mess.
The category handling code at the end of AddPseudoEntry has two problems.

- The assertion checks |category| for the IS_CPP_ENTRY flag. This represents a
  confusion between the entry's |category| and its |flags|. They're both stored
  in a single uint32_t, but are conceptually different types. So the assertion
  is vacuously satisfied.

  Furthermore, there's no clear way to fix the assertion -- it doesn't make
  sense to check the entry's flags for IS_CPP_ENTRY, because this code can
  clearly take C++ or JS entries. So the patch just removes the assertion.

- The category is compared to zero. This also doesn't make sense, because zero
  isn't a valid category. The patch removes this comparison.
Attachment #8873298 - Flags: review?(shu)
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
There are three flags in ProfileEntry::Flags, which suggests there are 2**3 = 8
combinations. But there are only actually 4 valid combinations.

This patch converts the three flags to a single "kind" enum, which makes things
clearer. Note also that the patch moves the condition at the start of
AddPseudoEntry() to its callsite, for consistency with the earlier JS_OSR entry
kind check.
Attachment #8873301 - Flags: review?(shu)
Attachment #8873298 - Flags: review?(shu) → review+
Comment on attachment 8873301 [details] [diff] [review]
(part 2) - Convert ProfileEntry::Flags to Kind

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

::: js/public/ProfilingStack.h
@@ +54,5 @@
>  
>      // Line number for non-JS entries, the bytecode offset otherwise.
>      int32_t volatile lineOrPcOffset;
>  
> +    // Bits 0..1 hold the Kind. Bits 2..3 are unused. Bits 4..12 hold the

Why leave bits 2 and 3 unused, to avoid shifting the category around?
Attachment #8873301 - Flags: review?(shu) → review+
> > +    // Bits 0..1 hold the Kind. Bits 2..3 are unused. Bits 4..12 hold the
> 
> Why leave bits 2 and 3 unused, to avoid shifting the category around?

Yes. Category has this comment:

>   // Keep these in sync with devtools/client/performance/modules/categories.js

I wasn't sure if changing those values would have any knock-on effects, so I decided to leave it unchanged.
https://hg.mozilla.org/mozilla-central/rev/70e36d9d546e
https://hg.mozilla.org/mozilla-central/rev/8726e36dafe1
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.