Closed Bug 1439014 Opened 6 years ago Closed 6 years ago

Add a profiler feature to enable JIT optimization tracking

Categories

(Core :: Gecko Profiler, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: mstange, Assigned: mstange)

References

Details

Attachments

(1 file, 1 obsolete file)

Optimization tracking was disabled in bug 1330532 and it can only be enabled by setting an environment variable.
I think it would be good to have a profiler feature for it, so that we can at least test the code manually more easily.
Comment on attachment 8951929 [details]
Bug 1439014 - Add a profiler feature to enable JIT optimization tracking.

https://reviewboard.mozilla.org/r/221202/#review227088

Can you write a test for this? Profiler features are already under-tested, it would nice to avoid that trend for this new one.
Attachment #8951929 - Flags: review?(n.nethercote) → review+
Comment on attachment 8951929 [details]
Bug 1439014 - Add a profiler feature to enable JIT optimization tracking.

https://reviewboard.mozilla.org/r/221202/#review227626
Attachment #8951929 - Flags: review?(sphink) → review+
Comment on attachment 8962255 [details]
Bug 1439014 - Add a test that checks for the presence of JS tracked optimization info.

https://reviewboard.mozilla.org/r/231118/#review236504


Code analysis found 4 defects in this patch:
 - 4 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: tools/profiler/tests/chrome/test_profile_with_trackopts.html:30
(Diff revision 1)
> +  features: ["js", "threads", "trackopts"],
> +  threads: ["GeckoMain"]
> +};
> +
> +function innerFunction(x) {
> +  return x * 0.7;                          // This is line 30.

Error: Multiple spaces found before '// this is lin...'. [eslint: no-multi-spaces]

::: tools/profiler/tests/chrome/test_profile_with_trackopts.html:40
(Diff revision 1)
> +}
> +
> +function outerFunction() {
> +  let k = 0;
> +  for (let i = 0; i < 1000000; i++) {
> +    k += middleFunction(i);                // This is line 40.

Error: Multiple spaces found before '// this is lin...'. [eslint: no-multi-spaces]

::: tools/profiler/tests/chrome/test_profile_with_trackopts.html:74
(Diff revision 1)
> +      }
> +      return optimizationSite;
> +    });
> +  }
> +  function prettifyAttempts(attempts) {
> +    return Array.from((function*() {

Error: Missing space after *. [eslint: generator-star-spacing]

::: tools/profiler/tests/chrome/test_profile_with_trackopts.html:95
(Diff revision 1)
> +      }
> +    }
> +    return optimizations;
> +  }
> +  function framesForFunc(functionName) {
> +    return Array.from((function*() {

Error: Missing space after *. [eslint: generator-star-spacing]
Comment on attachment 8962255 [details]
Bug 1439014 - Add a test that checks for the presence of JS tracked optimization info.

https://reviewboard.mozilla.org/r/231118/#review236548

::: tools/profiler/tests/chrome/profiler_test_utils.js:47
(Diff revision 1)
>    SimpleTest.finish();
>  }
>  
> -async function runTest(settings, workload) {
> +function getBufferInfo() {
> +  let position = {}, totalSize = {}, generation = {};
> +  Services.profiler.GetBufferInfo(position, totalSize, generation);

(not for this patch)

Too bad this function has no documentation :(

see https://searchfox.org/mozilla-central/source/tools/profiler/gecko/nsIProfiler.idl#91

::: tools/profiler/tests/chrome/test_profile_with_trackopts.html:81
(Diff revision 1)
> +        yield {
> +          strategy: stringTable[attempts.data[i][attempts.schema.strategy]],
> +          outcome: stringTable[attempts.data[i][attempts.schema.outcome]],
> +        };
> +      }
> +    })());

maybe using `map` would be just as good here:

```
const { strategy, outcome };
return attempts.data.map(attempt => ({
  strategy: stringTable[attempt[strategy]],
  outcome: stringTable[attempt[outcome]],
}));
```

::: tools/profiler/tests/chrome/test_profile_with_trackopts.html:105
(Diff revision 1)
> +            optimizations: prettifyOptimizations(frameTable.data[i][frameTable.schema.optimizations]),
> +            line: frameTable.data[i][frameTable.schema.line],
> +          };
> +        }
> +      }
> +    })());

optional nit: you can use `filter` and `map` + extract the schema data into variables like I suggest above, to make this more readable ?

::: tools/profiler/tests/chrome/test_profile_with_trackopts.html:198
(Diff revision 1)
> +  // ]
> +
> +  ok(outerFunctionFrames.length > 0, "should have sampled at least one frame of outerFunction() running");
> +  const outerFunctionIonFrames = outerFunctionFrames.filter(frame => frame.implementation == "ion");
> +  ok(outerFunctionIonFrames.length > 0, "should have observed outerFunction() running in ion");
> +  const outerFunctionIonFramesWithOptimizations = outerFunctionIonFrames.filter(frame => frame.optimizations != undefined);

nit: either use `!==` or just use `frame.optimizations` (because this isn't a string or an integer, this is enough)

::: tools/profiler/tests/chrome/test_profile_with_trackopts.html:208
(Diff revision 1)
> +  // get useful optimization information, we don't care about how exactly this
> +  // JS code was optimized.
> +  ok(outerFunctionIonFramesWithOptimizations.some(frame => {
> +    return frame.optimizations.line == 40 && frame.optimizations.attempts.some(attempt => {
> +      return attempt.strategy == "BinaryArith_SpecializedTypes" &&
> +             attempt.outcome == "GenericSuccess";

nit: please use strict equality checks (and the same below and above)
Attachment #8962255 - Flags: review?(felash) → review+
Pushed by mstange@themasta.com:
https://hg.mozilla.org/integration/autoland/rev/6d264bbb2669
Add a profiler feature to enable JIT optimization tracking. r=njn,sfink
https://hg.mozilla.org/integration/autoland/rev/f01eb749b3f9
Add a test that checks for the presence of JS tracked optimization info. r=julienw
Thanks for the review! I switched to use filter and map as you suggested, and it's much better now.
Assignee: nobody → mstange
Status: NEW → ASSIGNED
Pushed by mstange@themasta.com:
https://hg.mozilla.org/integration/autoland/rev/84c9b5ab7de9
Add a profiler feature to enable JIT optimization tracking. r=njn,sfink
https://hg.mozilla.org/integration/autoland/rev/226c8c740c34
Add a test that checks for the presence of JS tracked optimization info. r=julienw
Sorry about that. I hope I've fixed all the mistakes now.
Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f7f62e1b6f55511009abc97c5d91b8db59033f65
Flags: needinfo?(mstange)
Pushed by mstange@themasta.com:
https://hg.mozilla.org/integration/autoland/rev/a13074f87bb0
Add a profiler feature to enable JIT optimization tracking. r=njn,sfink
https://hg.mozilla.org/integration/autoland/rev/57426696adaf
Add a test that checks for the presence of JS tracked optimization info. r=julienw
Markus, looks like this fails on ARM -- maybe we don't have the same optimizations there?

Also I noticed you left some console.log in the test (see [1]). I think this clutters the output and you may want to remove it completely, or at least clamp it to some length.

[1] https://treeherder.mozilla.org/logviewer.html#?job_id=170444443&repo=autoland&lineNumber=7375
(In reply to Julien Wajsberg [:julienw] from comment #20)
> Markus, looks like this fails on ARM -- maybe we don't have the same
> optimizations there?

That would surprise me. I think the problem is that the profiler didn't take a sample while my JS functions were on the stack. It looks like we really do need a solid mechanism to make sure we sampled at the right time before we can really test this.

I'm going to land this without the test and file a new bug for adding the required test infrastructure.

> Also I noticed you left some console.log in the test (see [1]). I think this
> clutters the output and you may want to remove it completely, or at least
> clamp it to some length.

My new console.log statements are commented out; I think this is coming from the console.log that you left in profiler_test_utils.js:
https://searchfox.org/mozilla-central/rev/49cc27555d5b7ed2eb06baf156693dd578daa06f/tools/profiler/tests/chrome/profiler_test_utils.js#24-26
Flags: needinfo?(mstange)
Attachment #8962255 - Attachment is obsolete: true
Pushed by mstange@themasta.com:
https://hg.mozilla.org/integration/autoland/rev/05c3ff148ac8
Add a profiler feature to enable JIT optimization tracking. r=njn,sfink
https://hg.mozilla.org/mozilla-central/rev/05c3ff148ac8
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: