Closed Bug 1276349 Opened 4 years ago Closed 4 years ago
.58 - 5 .92% damp all platforms +- e10s regression on push 8dfd7c9d8639 (Wed May 25 2016)
58 bytes, text/x-review-board-request
Talos has detected a Firefox performance regression from push 8dfd7c9d8639. As author of one of the patches included in that push, we need your help to address this regression. This is a list of all known regressions and improvements related to the push: https://treeherder.mozilla.org/perf.html#/alerts?id=1328 On the page above you can see an alert for each affected platform as well as a link to a graph showing the history of scores for this test. There is also a link to a treeherder page showing the Talos jobs in a pushlog format. To learn more about the regressing test(s), please see: https://wiki.mozilla.org/Buildbot/Talos/Tests#DAMP Reproducing and debugging the regression: If you would like to re-run this Talos test on a potential fix, use try with the following syntax: try: -b o -p win32,linux64,macosx64,win64 -u none -t g2[Windows 7,10.10,Windows 8],g2-e10s[Windows 7,10.10,Windows 8] --rebuild 5 # add "mozharness: --spsProfile" to generate profile data (we suggest --rebuild 5 to be more confident in the results) To run the test locally and do a more in-depth investigation, first set up a local Talos environment: https://wiki.mozilla.lorg/Buildbot/Talos/Running#Running_locally_-_Source_Code Then run the following command from the directory where you set up Talos: talos --develop -e [path]/firefox -a damp (add --e10s to run tests in e10s mode) Making a decision: As the patch author we need your feedback to help us handle this regression. *** Please let us know your plans within 3 business days, or the offending patch(es) will be backed out! *** Our wiki page outlines the common responses and expectations: https://wiki.mozilla.org/Buildbot/Talos/RegressionBugsHandling
I think it's fairly safe to discard bug 1275706 (an ESLint config change) here.
No longer blocks: 1275706
thanks :jryans! here is a detailed compare view for linux64: https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=fx-team&originalRevision=73817edbdfd6f66e75b13eeb9a883994d04f178d&newProject=fx-team&newRevision=8dfd7c9d86397ed898d84933a3c39834f1db886f&originalSignature=6fb4fded0ef2af3f1e6945d9ecfc12d9f0a50ad1&newSignature=6fb4fded0ef2af3f1e6945d9ecfc12d9f0a50ad1&framework=1 ^ you can follow the links to the parent and view other platforms. There are 5 remaining patches that landed at once here, I see a few devtools related people in the commit messages. :jryans, would you have an idea of the root cause here? If not, I am happy to push to try server and bisect.
Bug 1271000 is docs only, can be ignored. Bug 1072442 is far away from DevTools, should not affect damp. The rest all seem like possible suspects.
Component: Untriaged → Developer Tools
cool! this narrows the list halfway down!
[Tracking Requested - why for this release]: perf regression
:jmaher, any news on bisecting between the patches?
Priority: -- → P1
sorry, just pushed this live: https://email@example.com&fromchange=9754870bfc621b72b86ce3afc44b86f0b834d34f&tochange=907422fccbe2f06f4285afaaaa8cad1620c3057f in about 2 hours we should have results, and I can check in and find the root cause
and the winner is.... bug 1265813: https://hg.mozilla.org/integration/fx-team/rev/31267984105e huh, the author of the patch isn't in bugzilla, but another person with the same name is. Tom, can you confirm that you wrote the patch in bug 1265813, and if so, can you take a look at this to determine why this is causing a damp regression across the board?
Yeah, I wrote it.
Assignee: nobody → ttromey
oh great! :tromey, let me know how I can help you understand the test or validate a fix.
checking in on this bug, we are 4+ weeks out now, this is on aurora and I have seen no action.
Thanks for the ping. I should have looked at this sooner. Based on the patch and examining the DAMP breakdown, I have two theories for the regression. One is that source-utils.js:parseURL is too slow now (specifically when called via getSourceNames); the other is that console-output.js:_isURL has slowed down. The latter seems particularly easy to speed up so I will try that.
The original report says: > If you would like to re-run this Talos test on a potential fix, use try with the following syntax: > try: -b o -p win32,linux64,macosx64,win64 -u none -t g2[Windows 7,10.10,Windows 8],g2-e10s[Windows 7,10.10,Windows 8] --rebuild 5 # add "mozharness: --spsProfile" to generate profile data ..but "mach try" doesn't accept this syntax.
trychooser suggests perhaps ./mach try -b o -p win32,linux64,macosx64,win64 -u none -t g2,g2-e10s --rebuild-talos 5
mach try doesn't respect all the specific platform restrictions, but editing a commit message and then |hg push try| seems to do the trick.
Status: NEW → ASSIGNED
Iteration: --- → 50.2 - Jul 4
Whiteboard: [talos_regression] → [talos_regression] [devtools-html]
The initial experiment didn't show any improvement. I'm going to re-run the talos try twice (before and after the patch) with profiling and see if that helps explain anything.
I tried a couple more experiments instead. Backing out the console-output.js change: https://hg.mozilla.org/try/rev/7754c9e7812bb13eadec20dfb4fca80a6451740b ... does make the DAMP regression go away. You can see this here: https://treeherder.mozilla.org/perf.html#/compare?originalProject=fx-team&originalRevision=73817edbdfd6&newProject=try&newRevision=d6acb19fd3978c05a97afdd9e913683696cedf15&framework=1&showOnlyImportant=0 My attempt at speedup was to try to optimize the ".includes()", using property lookup instead (like "return url.protocol in validProtocols"); but this didn't have much effect.\ I'm not sure what else to try. Maybe Set could improve it, though that seems doubtful. It's mildly surprising to me that this bit of code is hot enough to matter, so maybe looking into the callers would help.
(In reply to Tom Tromey :tromey from comment #19) > I tried a couple more experiments instead. > Backing out the console-output.js change: > https://hg.mozilla.org/try/rev/7754c9e7812bb13eadec20dfb4fca80a6451740b > ... does make the DAMP regression go away. > > You can see this here: > https://treeherder.mozilla.org/perf.html#/compare?originalProject=fx- > team&originalRevision=73817edbdfd6&newProject=try&newRevision=d6acb19fd3978c0 > 5a97afdd9e913683696cedf15&framework=1&showOnlyImportant=0 > > My attempt at speedup was to try to optimize the ".includes()", using > property lookup > instead (like "return url.protocol in validProtocols"); but this didn't have > much effect.\ > > I'm not sure what else to try. Maybe Set could improve it, though that > seems doubtful. > It's mildly surprising to me that this bit of code is hot enough to matter, > so maybe > looking into the callers would help. Indeed surprising this piece of code matters. I'm thinking we might just have live with it since this regression is limited only in the console specific probes in DAMP and the new console output frontend is in active development, which will bring in it's own set of perf improvements and regressions. The only other thing I can think of is maybe try flattening this check into a bunch of conditionals or a regexp instead of creating an array / object, or create the object at the top of the file instead of each time the function is called.
Looking at the callers I see that this is called for every token in the output (as determined by "split(/\s+/)"). So it seems plausible now that it's hot. Maybe a regexp would help, easy enough to test.
Review commit: https://reviewboard.mozilla.org/r/62690/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/62690/
Attachment #8768447 - Flags: review?(bgrinstead)
Comment on attachment 8768447 [details] Bug 1276349 - fix DAMP regression by speeding up _isURL; https://reviewboard.mozilla.org/r/62690/#review59402 Works for me. I guess this could result in slightly different behavior depending on how the URL object cleans up input but this is only called from a thing that's already splitting on whitespace, and the regex ignores case, so it should be close enough
Attachment #8768447 - Flags: review?(bgrinstead) → review+
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/33a91e8bceb3 Fix DAMP regression by speeding up _isURL. r=bgrins
verified this is fixed by inspecting the graphs! Thanks for the fix. This originally affected Firefox 49 (currently on Aurora), do we want to uplift this patch to 49?
Comment on attachment 8768447 [details] Bug 1276349 - fix DAMP regression by speeding up _isURL; Approval Request Comment [Feature/regressing bug #]: 1265813 [User impact if declined]: Perf regression in the web console [Describe test coverage new/current, TreeHerder]: Confirmed fix on talos (see Comment 28) [Risks and why]: Slight change in logic for URL parsing in the webconsole frontend. I don't know of any inputs where the behavior would differ, but it's possible. It only affects 'linkifying' in console messages (turning a string that looks like a URL into an <a> tag). [String/UUID change made/needed]:
Attachment #8768447 - Flags: approval-mozilla-aurora?
Comment on attachment 8768447 [details] Bug 1276349 - fix DAMP regression by speeding up _isURL; Perf fix for dev tools sounds good, let's uplift to aurora.
Attachment #8768447 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.