Closed Bug 1276349 Opened 4 years ago Closed 4 years ago

3.58 - 5.92% damp all platforms +- e10s regression on push 8dfd7c9d8639 (Wed May 25 2016)

Categories

(DevTools :: General, defect, P1)

49 Branch
defect

Tracking

(e10s-, firefox49+ fixed, firefox50 fixed)

RESOLVED FIXED
Firefox 50
Iteration:
50.3 - Jul 18
Tracking Status
e10s - ---
firefox49 + fixed
firefox50 --- fixed

People

(Reporter: jmaher, Assigned: tromey)

References

Details

(Keywords: perf, regression, Whiteboard: [talos_regression] [devtools-html])

Attachments

(1 file)

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.
Flags: needinfo?(jryans)
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.
No longer blocks: 1072442, 1271000
Flags: needinfo?(jryans)
Component: Untriaged → Developer Tools
cool!  this narrows the list halfway down!
tracking-e10s: --- → ?
[Tracking Requested - why for this release]:
perf regression
:jmaher, any news on bisecting between the patches?
Flags: needinfo?(jmaher)
Priority: -- → P1
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?
No longer blocks: 1256819, 1256916, 1273323
Flags: needinfo?(jmaher) → needinfo?(ttromey)
Yeah, I wrote it.
Assignee: nobody → ttromey
Flags: needinfo?(ttromey)
oh great!  :tromey, let me know how I can help you understand the test or validate a fix.
Tracking for 49 since this is a recent regression.
Version: 47 Branch → 49 Branch
checking in on this bug, we are 4+ weeks out now, this is on aurora and I have seen no action.
Flags: needinfo?(ttromey)
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.
Flags: needinfo?(ttromey)
thanks Tom!
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
Flags: qe-verify-
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.
Iteration: 50.2 - Jul 4 → 50.3 - Jul 18
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+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/33a91e8bceb3
Fix DAMP regression by speeding up _isURL. r=bgrins
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/33a91e8bceb3
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
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+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.