Closed Bug 1131295 Opened 5 years ago Closed 5 years ago

Fix profiler frontend category mappings

Categories

(Core :: JavaScript Engine: JIT, defect)

x86_64
Windows 8.1
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: djvj, Assigned: djvj)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Depends on: 1057082
Attached patch fix-profile-mappings.patch (obsolete) — Splinter Review
Attachment #8561678 - Flags: review?(vporof)
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];
Attachment #8561678 - Flags: review?(vporof) → review+
(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) => { ..."
Attachment #8561678 - Attachment is obsolete: true
Attachment #8562916 - Flags: review?(vporof)
Comment on attachment 8562916 [details] [diff] [review]
fix-profile-mappings.patch

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

Did you read comment 3?
Attachment #8562916 - Flags: review?(vporof) → review+
(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
https://hg.mozilla.org/mozilla-central/rev/4dfa3991e9ca
Assignee: nobody → kvijayan
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.