1919816 - 300-400% CPU load for a couple of seconds when switching to Google documents or spreadsheets

Status: RESOLVED FIXED

(Web Compatibility :: Site Reports, defect, P1)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2]]

During the last days I noticed quite a lot of CPU usage when working with Google documents and spreadsheets. Whenever I switch between such tabs the CPU load increases to roughly 300-400% for about 10-15s each. Further I see a spinner on a blank page when switching to the tab indicating that something is slow.

Here is a profile showing the issue: https://share.firefox.dev/47w7LFm (note that DMD is enabled as well)

Those docs that are public and I can share are:

https://docs.google.com/spreadsheets/d/1Cg3rifrBZClIitU3aFW_WDv64gY3ge8xPtN-HE1qzrY
https://docs.google.com/spreadsheets/d/1bkiPU5eDBCqFkx5p_VSBx_OK8gy9TeHRKQVPHKMATGQ

Comment 1

[Markus Stange [:mstange]]

Most of the CPU seems to be spent locking/unlocking a DMD mutex, so I would be curious to see a profile with DMD off.

However, this does indicate that we are doing a large number of allocations / frees. Nical, does this ring a bell?

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Here a profile with DMD disabled: https://share.firefox.dev/3XPXsZE

Comment 3

[Jeff Muizelaar [:jrmuizel]]

This is caused by hitting fallback when drawing SVG Image drawing 166x33048 https://ssl.gstatic.com/docs/common/material_common_sprite740_gm3_grey_medium.svg

We should do better.

Comment 4

[Markus Stange [:mstange]]

I think we can do better even before we have bug 1673653.

It looks like we draw this SVG image 12 times per paint. Why don't we cache it to a raster surface? My guess is it's too big.
However, if we don't cache it, then we should be able to skip painting SVG elements that are clipped out in the fallback item. Timothy, do we try to do something like that today?

Comment 5

[Jeff Muizelaar [:jrmuizel]]

We're falling back here: https://searchfox.org/mozilla-central/rev/56fd3b43187bca036141c384b809e61d51a447de/image/VectorImage.cpp#862 because 33048 > 32768

Comment 6

[Markus Stange [:mstange]]

(In reply to Markus Stange [:mstange] from comment #4)

It looks like we draw this SVG image 12 times per paint.

In Henrik's profile we seem to be painting it 35 times: https://share.firefox.dev/4e5V2Mb
(I counted the number of purple boxes that seem to align with one of the SVG URLs.)

There are two sprite sheets but one is used more than the other; 35 vs 5 times in Henrik's profile:
https://ssl.gstatic.com/docs/common/material_common_sprite740_gm3_grey_medium.svg
https://ssl.gstatic.com/docs/common/material_common_sprite740_blue.svg

(In reply to Jeff Muizelaar [:jrmuizel] from comment #5)

because 33048 > 32768

So maybe this image only recently crossed that threshold and that's when things got worse?

Comment 7

[Jeff Muizelaar [:jrmuizel]]

(In reply to Markus Stange [:mstange] from comment #6)

So maybe this image only recently crossed that threshold and that's when things got worse?

Seems plausible to me.

Comment 8

[Markus Stange [:mstange]]

I wonder if, in the meantime, we can ask Google to make the image slightly wider so that they can reduce the height and avoid this cliff in Firefox.

Comment 9

[Bob Hood [:bhood]]

Keeping Nical pinned here for any insights he can offer.

Comment 10

[Timothy Nikkel (:tnikkel)]

Before I get to the needinfo question: context as to why this might have gotten worse recently: previously due to a bug, in the case where we should have been falling back to fallback due to too large size (33048 > 32768) we just failed to draw at all, and this meant that sometimes google docs/sheets icons wouldn't draw from the large svg sprite sheet. Bug 1814878 fixed that so that instead of not drawing, we draw from fallback (which is not as efficient). So it's possible that a google change caused this to get worse recently, causing us to hit the fallback case more often.

Comment 11

[Brian Grinstead [:bgrins]]

Attachment: material_common_sprite740_gm3_grey_medium.svg

backup of the file in question for a testcase, in case it gets updated on the site

Comment 12

[Timothy Nikkel (:tnikkel)]

The icons use an abs pos img inside an overflow hidden element. So the img element is the size of the full sprite sheet, and the overflow hidden clips it to a single icon.

When we are creating the blog this line
https://searchfox.org/mozilla-central/rev/5049886134b906888fe78428bd298a94c4ee721e/gfx/layers/wr/WebRenderCommandBuilder.cpp#2434
means we use the full bounds of the image. If we instead change that to GetClippedBounds (the implications of this needs to be understood more) then it improves things somewhat but there is still a lot of inefficiencies.

When we get to draw the actual vector image we clip the context to the fill rect
https://searchfox.org/mozilla-central/rev/a6f4fbcd9b1ce352b7d6d1b87204d3e015266b57/image/VectorImage.cpp#265
so that when we paint
https://searchfox.org/mozilla-central/rev/5049886134b906888fe78428bd298a94c4ee721e/layout/painting/nsDisplayList.cpp#2148
GetPaintRect takes into account the clip on the context and so we can skip painting (recording) most of the other sprites. But we are still building the display list for all of the sprites because
https://searchfox.org/mozilla-central/rev/a6f4fbcd9b1ce352b7d6d1b87204d3e015266b57/image/VectorImage.cpp#279,291
we pass in the full viewport rect for the rect to RenderDocument. We should be able to restrict this rect to what we need to draw, but I have to factor in the transforms there to get the right rect.

Comment 13

[BugBot [:suhaib / :marco/ :calixte]]

The bug is marked as tracked for firefox131 (beta) and tracked for firefox132 (nightly). We have limited time to fix this, the soft freeze is in 2 days. However, the bug still isn't assigned.

:bhood, could you please find an assignee for this tracked bug? If you disagree with the tracking decision, please talk with the release managers.

For more information, please visit BugBot documentation.

Comment 14

[Bob Hood [:bhood]]

As I understand, this issue is being fixed in the short term on Google's end (refactoring the sprite sheet size to comply with our limit requirements).

Tim will also be keeping this on his H2 radar to see if we can improve our handling of these types of issues in the future.

Comment 15

[Timothy Nikkel (:tnikkel)]

[Tracking Requested - why for this release]: reseting tracking, suggest not tracking since google rolled out their change to avoid this

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Yeah, definitely a lot less janky switching between google suite tabs today.

Comment 17

[Randell Jesup [:jesup] (needinfo me)]

Closing since Google fixed this from their end, to mark the webcompat issue as resolved