Closed Bug 1369276 Opened 2 years ago Closed 2 years ago
Simplify flag handling in Profile
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)
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/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.
You need to log in before you can comment on or make changes to this bug.