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)
Core
Gecko Profiler
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 hidden (mozreview-request) |
Comment 2•6 years ago
|
||
mozreview-review |
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 3•6 years ago
|
||
mozreview-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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 6•6 years ago
|
||
mozreview-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 7•6 years ago
|
||
mozreview-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/#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+
Comment hidden (mozreview-request) |
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
Assignee | ||
Comment 10•6 years ago
|
||
Thanks for the review! I switched to use filter and map as you suggested, and it's much better now.
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → mstange
Status: NEW → ASSIGNED
Comment 11•6 years ago
|
||
Backed out 2 changesets (bug 1439014) for failing chrome failures and test verify tests on tools/profiler/tests/chrome/test_profile_with_trackopts.htm on a CLOSED TREE Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=170365143&repo=autoland&lineNumber=2460 Backout revision https://hg.mozilla.org/integration/autoland/rev/ad5016872bf5a63dbb04d75402b0decff41f3ac3 Failed push https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=f01eb749b3f99be8bc23aa64cd6bb7ec9a767d95&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified
Flags: needinfo?(mstange)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 14•6 years ago
|
||
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
Comment 15•6 years ago
|
||
Backed out 2 changesets (bug 1439014) for failing mochitest chrome and test verify on tools/profiler/tests/chrome/test_profile_with_trackopts.html on a CLOSED TREE Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=170405624&repo=autoland&lineNumber=2465 Backout revision: https://hg.mozilla.org/integration/autoland/rev/4d09e6d03883ca03875822c43d16ea6346460fda Failed push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=226c8c740c34e5e53be3282196032504a9665c2e&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified
Comment hidden (mozreview-request) |
Assignee | ||
Comment 17•6 years ago
|
||
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)
Comment 18•6 years ago
|
||
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
Comment 19•6 years ago
|
||
Backed out 2 changesets (bug 1439014) for failing mochitest chrome and test verify on tools/profiler/tests/chrome/test_profile_with_trackopts.html Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=170445587&repo=autoland Backout revision: https://hg.mozilla.org/integration/autoland/rev/e705ac6082f86f1e7ca2c6764576ab7835eb554c Failed push:https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=57426696adaf08298af3027fa77486fee0633b13&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified
Flags: needinfo?(mstange)
Comment 20•6 years ago
|
||
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
Assignee | ||
Comment 21•6 years ago
|
||
(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)
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8962255 -
Attachment is obsolete: true
Comment 23•6 years ago
|
||
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
Comment 24•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/05c3ff148ac8
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Updated•6 years ago
|
status-firefox60:
affected → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•