1369276 - Simplify flag handling in ProfileEntry

Status: RESOLVED FIXED

(Core :: Gecko Profiler, enhancement)

Description

[Nicholas Nethercote [inactive]]

ProfileEntry's flags are a bit of a mess.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Fix category handling in AddPseudoEntry

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.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Convert ProfileEntry::Flags to Kind

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.

Comment 3

[Shu-yu Guo [:shu]]

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?

Comment 4

[Nicholas Nethercote [inactive]]

> > +    // 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.

Comment 5

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/70e36d9d546e22b44ec55eb5906987d95bfcbcc7
Bug 1369276 (part 1) - Fix category handling in AddPseudoEntry. r=shu.

https://hg.mozilla.org/integration/mozilla-inbound/rev/8726e36dafe1c875d86b33b5885f6b9e6d1a5a12
Bug 1369276 (part 2) - Convert ProfileEntry::Flags to Kind. r=shu.

Comment 6

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/70e36d9d546e
https://hg.mozilla.org/mozilla-central/rev/8726e36dafe1