Closed Bug 1439014 Opened 7 years ago Closed 7 years ago

Add a profiler feature to enable JIT optimization tracking

Categories

(Core :: Gecko Profiler, enhancement)

Product:
Core
Component:
Gecko Profiler
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: mstange, Assigned: mstange)

References

Details

Attachments

(1 file, 1 obsolete file)

Bug 1439014 - Add a profiler feature to enable JIT optimization tracking.
59 bytes, text/x-review-board-request
n.nethercote
: review+
sfink
: review+
Details
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: 7 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: