832998 - web page is MUCH slower in FF than in Chrome

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[jrwren]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:20.0) Gecko/20130118 Firefox/20.0
Build ID: 20130118131739

Steps to reproduce:

visit http://carlcamera.com/codemash/


Actual results:

browser not responsive for 10s or so.


Expected results:

browser should be responsive

Comment 1

[Ioana (away)]

Confirmed on the latest Nightly, on Windows 7 32bit:
Mozilla/5.0 (Windows NT 6.1; rv:21.0) Gecko/20130121 Firefox/21.0

Comment 2

[Josh Matthews [:jdm]]

A profile would be useful: https://developer.mozilla.org/en/Performance/Profiling_with_the_Built-in_Profiler

Comment 3

[Boris Zbarsky [:bzbarsky]]

Profile shows the following:

40% of the time is taken by restyling triggered by .offsetWidth and .offsetHeight gets.

60% of the time is taken by computed style gets triggering restyling and reflow.

It would be good to have a somewhat reduced testcase to see why the above things, which are usually the same speed in Firefox and Chrome, are being slow.  And in particular, to see whether the page is running the same script in Firefox and Chrome...

Comment 4

[Loic]

It's a regression in Firefox 20:

m-c
good=2012-12-11
bad=2012-12-12
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=4dfe323a663d&tochange=634180132e68

Result:
A script on this page may be busy, or it may have stopped responding. You can stop the script now, or you can continue to see if the script will complete.
Script: http://ajax.googleapis.com/ajax/libs/jquery/1.8/jquery.min.js:2

Before 2012-12-12, Firefox is able to respond and doesn't display the window to stop/continue the script.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Hrm.  That's a pretty huge range.  :(

Could you try inbound nightlies to see when this changed there?  Wish we still had hourlies from back then....

Comment 6

[Loic]

What's the repo? --repo=mozilla-inbound? Because since a change in build system in July/August, mozreg is not able to use m-i anymore as repo.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

My guess would be throttled OMTA.  I noticed a test there that was backwards-ish (shouldn't have a ! on the first half, and should have && rather than ||... which then means no inner () are needed), and had been meaning to file a bug:
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/84d356ad5fb9/backwards-throttling-enabled-check

Comment 8

[Boris Zbarsky [:bzbarsky]]

I'm not sure how to tell the right thing to mozregression here, sorry.  The repo is at http://hg.mozilla.org/integration/mozilla-inbound/ for what that's worth, and there are certainly nightly builds up on ftp; looking at those for the 3-4 days involved may be simpler than fighting mozregression.

Maybe try mozilla-inbound-debug as the repo name, actually?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Though debug builds for perf stuff.. not helpful.

David, do we have prefs we could toggle to turn off throttled OMTA and see whether that's it?  I suppose that might not turn off all the relevant code...

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

that is:
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/47f196dd72a3/backwards-throttling-enabled-check

(If that does fix it, we should check would be if it's an actual problem with OMTA, or if it's only showing up because we don't tick the LastStyleUpdateForAllAnimations when we don't actually have OMTA.)

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Boris Zbarsky (:bz) from comment #9)
> David, do we have prefs we could toggle to turn off throttled OMTA and see
> whether that's it?  I suppose that might not turn off all the relevant
> code...

Try the patch instead (and see the patch for why the pref wouldn't help)... or I will in a bit, once I push some bug 828312 debugging off the stack.

Comment 12

[Loic]

About OMTA, there are these bugs in the changeset:
ebc71a3e489b	Nicholas Cameron — Bug 797520; improve OMTA logging. r=cjones
	2db54808d08e	Nicholas Cameron — Bug 814921; don't flush styles if there are no active animations. r=dholbert
	dd737056538a	Nicholas Cameron — Bug 780692; throttle OMTA (rollup patch). r=dbaron,bz

Comment 13

[Boris Zbarsky [:bzbarsky]]

> Try the patch instead 

Yeah, the patch linked in comment 10 fixes things for me.

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

Then I think we should do this for now to remove the OMTA regression from non-OMTA platforms, but we need to figure out if this is also a problem with OMTA.  I suspect it is.  (Which is disturbing given that these lines:
           !mPresContext->StyleUpdateForAllAnimationsIsUpToDate()) {
         mPresContext->TickLastStyleUpdateForAllAnimations();
ought to ensure we do this at most once per refresh driver tick.  I'm also curious what animations are involved in this page.)

Comment 15

[Nick Cameron [:nrc]]

Comment on attachment 704946 [details] [diff] [review]
patch

Review of attachment 704946 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, the change makes logical sense. I can't remember why it was the way it was - whether it was deliberate or a mistake, sorry. I ran all my tests and I don't see any difference in the number of flushes or the output, so I think it is fine.

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to David Baron [:dbaron] from comment #14)
> Then I think we should do this for now to remove the OMTA regression from
> non-OMTA platforms, but we need to figure out if this is also a problem with
> OMTA.  I suspect it is.  (Which is disturbing given that these lines:
>            !mPresContext->StyleUpdateForAllAnimationsIsUpToDate()) {
>          mPresContext->TickLastStyleUpdateForAllAnimations();
> ought to ensure we do this at most once per refresh driver tick.  I'm also
> curious what animations are involved in this page.)

Er, actually, it's not so disturbing.

With OMTA enabled, the condition is the same both before and after the patch:

if (aFlush.mFlushAnimations && !mPresContext->StyleUpdateForAllAnimationsIsUpToDate())

The problem was that with OMTA disabled, before the patch, the condition was just:

if (aFlush.mFlushAnimations)

which can potentially run a *lot*, whereas after the patch, with OMTA disabled, the condition is now:

if (false)

So with OMTA disabled, we were skipping the once-per-refresh-tick aspect.

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c7e467b8fe38

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

And the code in question was landed on aurora for 19, so changing tracking-firefox19 flag.  (Please actually check that a branch is unaffected before marking it unaffected.)

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 704946 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 780692 (throttling OMTA)
User impact if declined: significant performance degradation on some pages
Testing completed: tested testcase, on mozilla-inbound
Risk to taking this patch (and alternatives if risky):  very low risk; it restores the non-OMTA codepath to the way it was before OMTA throttling landing byfixng a backwards check for whether OMTA is enabled (which is only the case for B2G).  I think it's worth landing on b2g18 in case somebody builds something with OMTA off from that branch.
String or UUID changes made by this patch: none

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c7e467b8fe38

Comment 21

[Lukas Blakk [:lsblakk] use ?needinfo]

Tracking for releases, not for b2g18 - there are no plans to build anything other than b2g off that branch, least of all with OMTA off, and we don't want more churn there at this critical juncture.  Will approve for branch uplift to get this regression fix into 19 in time.

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2103dd3101ce

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://hg.mozilla.org/releases/mozilla-beta/rev/cfa438fea2b9

Comment 24

[Ioana (away)]

Verified fixed on Firefox 19 Beta 4 - 20130130080006:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/20100101 Firefox/19.0
Mozilla/5.0 (X11; Linux i686; rv:19.0) Gecko/20100101 Firefox/19.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:19.0) Gecko/20100101 Firefox/19.0

Comment 25

[Mitza]

Verified fixed on Firefox 20 Beta 5 - 20130313170052:
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:20.0) Gecko/20100101 Firefox/20.0
Mozilla/5.0 (X11; Linux i686; rv:20.0) Gecko/20100101 Firefox/20.0
Mozilla/5.0 (Windows NT 6.2; rv:20.0) Gecko/20100101 Firefox/20.0