1131295 - Fix profiler frontend category mappings

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Kannan Vijayan [:djvj]]

Bug 1057082 changed profiler category mapping numbers (offset by a factor of 2).  The profiler frontend mappign codes were not changed to reflect this at the time of landing.

This needs to be fixed so that the tests are testing the correct things.

Comment 1

[Kannan Vijayan [:djvj]]

Attachment: fix-profile-mappings.patch

Comment 2

[Kannan Vijayan [:djvj]]

Try run (looking for dt green): https://treeherder.mozilla.org/#/jobs?repo=try&revision=053b7e5ec2a4

Comment 3

[Victor Porof [:vporof][:vp]]

Comment on attachment 8561678 [details] [diff] [review]
fix-profile-mappings.patch

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

CATEGORY_ID does not exist as an export. r+ with the change to invoking `CATEGORY_MASK`, not `CATEGORY_ID` in tests, and optionally the nits below that.

::: browser/devtools/profiler/test/browser_profiler_tree-view-02.js
@@ +127,5 @@
>  
>  let gSamples = [{
>    time: 5,
>    frames: [
> +    { category: CATEGORY_ID('other'),  location: "(root)" },

CATEGORY_MASK, not CATEGORY_ID.

::: browser/devtools/profiler/test/browser_profiler_tree-view-04.js
@@ +91,5 @@
>  
>  let gSamples = [{
>    time: 5,
>    frames: [
> +    { category: CATEGORY_ID('other'),  location: "(root)" },

Ditto.

::: browser/devtools/profiler/utils/global.js
@@ +55,5 @@
> + */
> +const [CATEGORY_MASK, CATEGORY_MASK_LIST] = (function () {
> +  let mappings = {};
> +  for (let category of CATEGORIES) {
> +    let numList = [];

Would be nicer to just do 

let mappings = CATEGORIES.reduce(store, category => {
  store[category.abbrev] = Object
    .keys(CATEGORY_MAPPINGS)
    .filter(e => CATEGORY_MAPPINGS[e] == category)
    .map(e => +e);

  return store;
}, {});

..instead of all this imperative spaghetti.

But this is just me getting fancy.

@@ +69,5 @@
> +
> +  return [
> +    function (name, num) {
> +      if (arguments.length == 1) {
> +        if (mappings[name].length != 1) {

This can sometimes fail if mappings[name] is undefined, e.g. for CATEGORY_MAPPINGS['fail']. Just a nit, but I'd like us to more gracefully handle that case and throw a meaningful error, even though it doesn't matter too much.

Let's use string literals while we're at it too, because we're cool.

if (!(name in mappings)) {
  throw "Given category abbreviation does not exist."
}
if (arguments.length == 1) {
  if (mappings[name].length != 1) {
    throw `Expected exactly one category number for ${name}`;
  }
  num = 1
}
return mappings[name][num];

Comment 4

[Victor Porof [:vporof][:vp]]

(In reply to Victor Porof [:vporof][:vp] from comment #3)
> Comment on attachment 8561678 [details] [diff] [review]
> 
> let mappings = CATEGORIES.reduce(store, category => {

Made a typo here, the arrow function arguments should be surrounded in parens, so ".reduce((store, category) => { ..."

Comment 5

[Kannan Vijayan [:djvj]]

Attachment: fix-profile-mappings.patch

Comment 6

[Kannan Vijayan [:djvj]]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a50110746a83

Comment 7

[Victor Porof [:vporof][:vp]]

Comment on attachment 8562916 [details] [diff] [review]
fix-profile-mappings.patch

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

Did you read comment 3?

Comment 8

[Kannan Vijayan [:djvj]]

(In reply to Victor Porof [:vporof][:vp] from comment #7)
> Did you read comment 3?

Gah!  Sorry, by the time I ended up fixing the remaining oranges, I forgot to go back and review for additional comments.  Did that.  I split the difference in the fancy-js part.  Running updated patch through try one last time before landing:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=df05d40bd4f1

Comment 9

[Kannan Vijayan [:djvj]]

Latest try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4a1ea1adb684

Landing!
https://hg.mozilla.org/integration/mozilla-inbound/rev/4dfa3991e9ca

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4dfa3991e9ca