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

Status: RESOLVED FIXED

(DevTools :: General, defect, P1)

Description

[Joel Maher ( :jmaher ) (UTC -8)]

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

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

I think it's fairly safe to discard bug 1275706 (an ESLint config change) here.

Comment 2

[Joel Maher ( :jmaher ) (UTC -8)]

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.

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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.

Comment 4

[Joel Maher ( :jmaher ) (UTC -8)]

cool!  this narrows the list halfway down!

Comment 5

[Jim Mathies [:jimm]]

[Tracking Requested - why for this release]:
perf regression

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

:jmaher, any news on bisecting between the patches?

Comment 7

[Joel Maher ( :jmaher ) (UTC -8)]

sorry, just pushed this live:
https://treeherder.mozilla.org/#/jobs?repo=try&author=[e-mail]&fromchange=9754870bfc621b72b86ce3afc44b86f0b834d34f&tochange=907422fccbe2f06f4285afaaaa8cad1620c3057f

in about 2 hours we should have results, and I can check in and find the root cause

Comment 8

[Joel Maher ( :jmaher ) (UTC -8)]

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?

Comment 9

[Tom Tromey :tromey]

Yeah, I wrote it.

Comment 10

[Joel Maher ( :jmaher ) (UTC -8)]

oh great!  :tromey, let me know how I can help you understand the test or validate a fix.

Comment 11

[Marcia Knous [:marcia]]

Tracking for 49 since this is a recent regression.

Comment 12

[Joel Maher ( :jmaher ) (UTC -8)]

checking in on this bug, we are 4+ weeks out now, this is on aurora and I have seen no action.

Comment 13

[Tom Tromey :tromey]

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.

Comment 14

[Joel Maher ( :jmaher ) (UTC -8)]

thanks Tom!

Comment 15

[Tom Tromey :tromey]

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.

Comment 16

[Tom Tromey :tromey]

trychooser suggests perhaps

./mach try -b o -p win32,linux64,macosx64,win64 -u none -t g2,g2-e10s --rebuild-talos 5

Comment 17

[Joel Maher ( :jmaher ) (UTC -8)]

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.

Comment 18

[Tom Tromey :tromey]

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.

Comment 19

[Tom Tromey :tromey]

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.

Comment 20

[Brian Grinstead [:bgrins]]

(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.

Comment 21

[Tom Tromey :tromey]

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.

Comment 22

[Tom Tromey :tromey]

Attachment: Bug 1276349 - fix DAMP regression by speeding up _isURL;

Review commit: https://reviewboard.mozilla.org/r/62690/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/62690/

Comment 23

[Tom Tromey :tromey]

That seems to work ok, e.g.: https://treeherder.mozilla.org/perf.html#/compare?originalProject=fx-team&originalRevision=5abbfd6f2779&newProject=try&newRevision=f5f57b307fc6991a7e0864f471177e7183ea101a&framework=1&showOnlyImportant=0

Comment 24

[Brian Grinstead [:bgrins]]

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

Comment 25

[Tom Tromey :tromey]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=70f195f5ea1a

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/33a91e8bceb3
Fix DAMP regression by speeding up _isURL. r=bgrins

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/33a91e8bceb3

Comment 28

[Joel Maher ( :jmaher ) (UTC -8)]

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 29

[Brian Grinstead [:bgrins]]

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]:

Comment 30

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4a42aa8c44a0