1469208 - performance at google images after several image searches

Status: RESOLVED WORKSFORME

(Core :: DOM: CSS Object Model, defect, P2)

Description

[Mark]

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:62.0) Gecko/20100101 Firefox/62.0
Build ID: 20180617100056

Steps to reproduce:

at images.google.com I searched several times for terms like "wire strippers", "cable and wire strippers", "crimpers and wire strippers", each time clicking at the bottom to display more items.


Actual results:

While the searches all succeeded (but not showing the precise tool I was looking for), at one point when scrolling down the tab froze before painting more images and the CPU usage went to 25% (equivalent of one core) for almost a minute, and at the end of the time it returned to normal and the images finished painting.

I finally did remember to kick off the Gecko Profiler and captured https://perfht.ml/2JUtmf5


Expected results:

I would have expected, at most, a brief pause while scrolling down on the images that Google returned on my image query.

Comment 1

[Mark]

Attachment: about:support raw data

The about:support section from my Firefox.

Note: this is Nightly 62.0a1 (2018-06-17) (64-bit) Build-ID 20180617100056 on Windows 7 SP1 Home Premium (64-bit).

This problem has been noticed for quite a few months, but I think this is the first time I had the presence of mind to turn on the profiler while Firefox was grabbing a core's worth of CPU cycles during a Google Images scroll.

Comment 2

[Kanchan Kumari QA]

Hello Mike, could you please look into profile report here? Thanks!

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Interesting - we spent a pretty huge amount of time cycle-collecting one or more nsComputedDOMStyle objects... I'm going to put this under DOM for now.

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Oh, and here's the relevant part of the profile - it didn't stand out to me at first because I couldn't see any responsiveness markers:

https://perfht.ml/2K5Hq5v

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

More like a styling issue.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

I could be wrong, but this smells like a regression from bug 1203766.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Hmm, the line that shows up in the profile is:

  https://searchfox.org/mozilla-central/rev/d0a41d2e7770fc00df7844d5f840067cc35ba26f/layout/style/nsComputedDOMStyle.cpp#803

So that is removing the computed style object as a mutation observer, which we only need to invalidate the cache when the parent chain changes.

I think that caching is incorrect for Shadow DOM, since you can change the flattened tree parent without changing the light tree parent, and we'd give incorrect results then.

The annoying bit is that we can't just preserve a "parent" pointer, which was the way I was going to fix this... we care about the whole flattened tree parent chain, which is very annoying.

Cam, any ideas?

Comment 8

[Cameron McCormack (:heycam)]

Do we have many nsComputedDOMStyle objects live for the one element?  If so, when we cycle collect, we may end up removing observers from the array in a way that takes quadratic time.  Can we use a different data structure to avoid this?

As for avoiding using mutation observers for invalidating the cached style -- wouldn't we need to validate the entire parent chain even without Shadow DOM in the picture?  We could have an element in a display:none subtree, and we move its parent somewhere else.  And storing the entire parent chain sounds too burdensome.

No other good ideas so far...

Comment 9

[Cameron McCormack (:heycam)]

(In reply to Cameron McCormack (:heycam) from comment #8)
> Do we have many nsComputedDOMStyle objects live for the one element?  If so,
> when we cycle collect, we may end up removing observers from the array in a
> way that takes quadratic time.  Can we use a different data structure to
> avoid this?

If we didn't want to change how nsTObserverArray stores its data, we could register a single observer per element that has an nsComputedDOMStyle live for it, and that observer could store a hash table of nsComputedDOMStyle objects, so they can be removed more quickly.  Assuming many observers for the one element is the problem here.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

(In reply to Cameron McCormack (:heycam) from comment #8)
> As for avoiding using mutation observers for invalidating the cached style
> -- wouldn't we need to validate the entire parent chain even without Shadow
> DOM in the picture?  We could have an element in a display:none subtree, and
> we move its parent somewhere else.  And storing the entire parent chain
> sounds too burdensome.

Yes, exactly, that's why my initial idea just doesn't work :)

Comment 11

[Emilio Cobos Álvarez (:emilio)]

It was just occurring to me that bug 1381071 would also fix this... But that optimization is hard, and so broken in WebKit.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Cameron McCormack (:heycam) from comment #9)
> If we didn't want to change how nsTObserverArray stores its data, we could
> register a single observer per element that has an nsComputedDOMStyle live
> for it, and that observer could store a hash table of nsComputedDOMStyle
> objects, so they can be removed more quickly.  Assuming many observers for
> the one element is the problem here.
Or LinkedList. Store the LinkedList in the observer array (by wrapping it with a slim nsIMutationObserver object) and each nsComputedDOMStyle object would be entries in the list.
(adding and removing from list is fast, hashtable lookups tend to show up in profiles)
Annoying part would be to add nsComputedDOMStyle to the list, since one would need to iterate the
observer array to find the list object there.


Another thing, could we possibly do clearance earlier than in dtor. Like in the last release when refcnt drops to 0. That would catch the case when GC drops the JS wrapper.
As an example we do the following with Range objects https://searchfox.org/mozilla-central/rev/d0a41d2e7770fc00df7844d5f840067cc35ba26f/dom/base/nsRange.cpp#348-349

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Too bad annotating CSSStyleDeclaration with ProbablyShortLivingWrapper wouldn't perhaps make sense, since element.style sure isn't short living.
Though, we do use ProbablyShortLivingWrapper with NodeLists too, and those aren't necessarily short living.
So, maybe it could be tried. Nursery allocation for wrapper + clearance in last release might help here quite a bit.

(ProbablyShortLivingWrapper means that we allocate the wrapper from nursery and then minor GC can collect it, or minor GC will move the wrapper to tenured heap)

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

I separated that to bug 1470099.
MutationObserver bits could stay in this bug. They do seem still rather heavy to me.

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Mike, do you know if this is better after bug 1470099?

Comment 16

[Emilio Cobos Álvarez (:emilio)]

Err, meant to ni?, and Mark, not Mike (whoops).

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

FWIW, I think nsIMutationObserver usage should be fixed here anyhow. Too many observers affect to all the
DOM tree operations.

Comment 18

[Mark]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #15)
> Mark, do you know if this is better after bug 1470099?

At best, I can say _possibly_ better; I tried the steps that sometimes trigger the tab freeze, but the tab did _not_ freeze this time. (I hate these intermittent bugs; you never know for sure when they are fixed!)

I had Windows Task Manager up and running and I did notice the memory creep (expected) and a release of a bit of memory, but no tab freeze when that bit of memory was released, which is encouraging--possibly issue fixed!

I'll try again tonight or tomorrow if time permits.

I don't know if the rest of this comment is relevant: this is with a new computer:
> old, possibly failing hard drive[1] -> new hard drive
> Windows 7 (64-bit) -> Windows 10 (64-bit)
> AMD 4-core processor -> Intel 4-core processor i5-7400 (almost twice as fast)
> AMD GPU -> Intel HD Graphics 630

[1] Sorry, I didn't know the hard drive of the 6-year-old Windows 7 machine was starting to fail when I opened this bug report.

Comment 19

[Mark]

The tab freezes with Google Images seems to be fixed, or more specifically after several attempts this morning and this evening of doing multiple searches in a row within the same tab I was unable to get searches in images.google.com to momentary freeze the tab.