1468133 - EnsureNonSVGUserAgentStyleSheetsLoaded looks pretty bogus and slow for the IsBeingUsedAsImage() case.

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P3)

Description

[Emilio Cobos Álvarez (:emilio)]

In the IsBeingUsedAsImage case, we pull from the category manager service and such, and start loading and parsing those as builtin "on-demand" UA sheets.

This has problems. As noted that needs to "append" to the UA sheet list. But right now we prepend and rely on loading it before other sheets.

However, nothing guarantees that we haven't loaded svg.css before (indeed we probably have), or any other sheet for that matter, so we're prepending those before the already-loaded ones. That looks bad.

Also, we're re-loading and reparsing the sheets even though nsStyleSheetService has a cached already-parsed copy of those. That's unnecessary work, cpu usage, and memory usage...

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Actually, I wonder if that optimization is relevant anymore... This was done as part of bug 1008455 and similar followups. But that predates bug 77999, and the relevant UA stylesheet cache that we have:

  https://searchfox.org/mozilla-central/rev/e6a0a192ea8691f7eb466506301da73fabe8fd23/servo/components/style/stylist.rs#65

I think we should try to remove it, that would fix, e.g., text-selection on Android, due to the accessible caret stuff...

Comment 2

[Emilio Cobos Álvarez (:emilio)]

I pushed the removal to awsy and talos: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3cbd16216273eb9b2e48a250a98d8b02cf1613e8

So ni? myself to check it.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1468133: Remove the optimization to lazily load non-SVG styles since it's not relevant anymore.

This was a memory-saving optimization introduced as part of dependencies for bug
686875, but a more general system landed in bug 77999 for Gecko and
https://github.com/servo/servo/pull/18509 for Servo.

So now it's probably even a bit of a pessimization (though probably not huge),
and given this causes bugs like bug 1462742, bug 1157592, and bug 1468145, and
fishiness like the one pointed out in this bug, we may as well remove it.

The performance impact of having to lookup through more rules should be minimal
given the bloom filter and the rule hash optimizations.

This makes me wonder whether we could remove the whole concept of on-demand UA
sheets, since they've caused pain, for example, when the frontend people try
loading <svg>s from NAC (since that triggers sheet loading from frame
construction, which is not good). I'm not concerned about loading mathml.css and
svg.css everywhere, though xul.css may not be as doable since it adds a bunch of
attribute-dependent selectors. Though on the other hand I asserted in the
xul.css code and we don't load it in content with <video> / <input
type="date/time/etc"> and such, afaict, so maybe now that legacy addons are gone
we can remove that sheet from content processes altogether. Maybe worth at least
filing a followup?

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

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Talos shows mostly noise on windows, nothing relevant otherwise. AWSY similarly nothing terrible, though it doesn't seem like the "image" test-cases were run, which are probably the interesting ones.

I forgot to repeat the talos runs, thus the noise, I'll try to push a better run, but I'm almost sure this is ok to remove nowadays :)

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Talos looks fine:

  https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=66314a598d526a27e88d442d8c949ee761e764f2&framework=1&showOnlyConfident=1&selectedTimeRange=172800

And no memory regression either:

  https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=66314a598d526a27e88d442d8c949ee761e764f2&framework=4&selectedTimeRange=172800

Comment 6

[Cameron McCormack (:heycam)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #4)
> Talos shows mostly noise on windows, nothing relevant otherwise. AWSY
> similarly nothing terrible, though it doesn't seem like the "image"
> test-cases were run, which are probably the interesting ones.

Doesn't like in what way?  Was there a regression?  As you say, tests involving SVG images are probably relevant here...

Comment 7

[Emilio Cobos Álvarez (:emilio)]

I didn't mention "doesn't like". I mentioned that there doesn't seem like they ran on the first place, but they did run on the push in comment 5, which shows no regression.

Comment 8

[Cameron McCormack (:heycam)]

Sorry, for some reason my brain skipped over the "seem" in your sentence, so I though I read that it doesn't like the tests!

Comment 9

[Cameron McCormack (:heycam)]

Comment on attachment 8984799 [details]
Bug 1468133: Remove the optimization to lazily load non-SVG styles since it's not relevant anymore.

https://reviewboard.mozilla.org/r/250600/#review258064

Just to confirm, SVG as images will now load with a set of style sheets that will definitely use the same cached UA CasacadeData entry that we would get for a regular HTML document?  r=me if so.

::: commit-message-af2b3:14
(Diff revision 1)
> +This makes me wonder whether we could remove the whole concept of on-demand UA
> +sheets, since they've caused pain, for example, when the frontend people try
> +loading <svg>s from NAC (since that triggers sheet loading from frame
> +construction, which is not good). I'm not concerned about loading mathml.css and
> +svg.css everywhere, though xul.css may not be as doable since it adds a bunch of
> +attribute-dependent selectors. Though on the other hand I asserted in the
> +xul.css code and we don't load it in content with <video> / <input
> +type="date/time/etc"> and such, afaict, so maybe now that legacy addons are gone
> +we can remove that sheet from content processes altogether. Maybe worth at least
> +filing a followup?

Filing a followup with the contents of this paragraph sounds good.

::: dom/svg/SVGDocument.cpp
(Diff revision 1)
> -    EnsureOnDemandBuiltInUASheet(cache->NoFramesSheet());
> -  }
> -  if (nsLayoutUtils::ShouldUseNoScriptSheet(this)) {
> -    EnsureOnDemandBuiltInUASheet(cache->NoScriptSheet());
> -  }
> -  EnsureOnDemandBuiltInUASheet(cache->UASheet());

There is a comment in ua.css "SVG documents don't always load this file [...]".  Can you remove that?

And there are some rules that are duplicated from ua.css in svg.css.  Can you remove those?

Comment 10

[Emilio Cobos Álvarez (:emilio)]

(In reply to Cameron McCormack (:heycam) from comment #9)
> Comment on attachment 8984799 [details]
> Bug 1468133: Remove the optimization to lazily load non-SVG styles since
> it's not relevant anymore.
> 
> https://reviewboard.mozilla.org/r/250600/#review258064
> 
> Just to confirm, SVG as images will now load with a set of style sheets that
> will definitely use the same cached UA CasacadeData entry that we would get
> for a regular HTML document?  r=me if so.

I think that's right as of this patch, but I don't think that's absolutely guaranteed right now given media queries in svg.css. Though right now we don't load svg.css upfront anywhere, that's about to change because it blocks the frontend team (see bug 1469176).

Still I think this patch is worth taking as-is. I can analyze the UA cache in the content process and try to optimize for it (in particular, it's not clear to me that the media-query introduced in bug 686581 is terribly useful, see Dao's and Roc's discussion at the end), but I don't think it should block this. wdyt?

Comment 11

[Cameron McCormack (:heycam)]

Verifying that this is all sharing properly sounds fine to me, thanks!

Comment 12

[Emilio Cobos Álvarez (:emilio)]

I filed bug 1470143 for the sharing stuff.

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8984799 [details]
Bug 1468133: Remove the optimization to lazily load non-SVG styles since it's not relevant anymore.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/250600/diff/1-2/

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Attachment: fixup.patch

Had to do this as well to avoid calling nsPermissionManager early on when loading SVG images, since it was hitting some assertions. It effectively preserves the existing behavior, though I can also change AllowXULXBL if you prefer.

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Attachment: another-fixup.patch

Just to avoid a few assertions for AllowXULXBL now returning true, but LoadsXULStyleSheetUpFront being false. No behavior change. Before this bug the elements would be equally unstyled.

I can also just annotate the assertions if preferred, they're quite niche:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e8ddcc193db19d06aa0ab7c0e7027cb16a98be6b&selectedJob=184185578

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c8f6b544de1
Remove the optimization to lazily load non-SVG styles since it's not relevant anymore. r=heycam

Comment 17

[Jonathan Watt [:jwatt]]

From the commit message:
> The performance impact of having to lookup through more rules should
> be minimal given the bloom filter and the rule hash optimizations.

Fingers crossed. :)

> I asserted in the xul.css code and
> we don't load it in content with <video> /
> <input type="date/time/etc"> and such, afaict, so maybe now that
> legacy addons are gone we can remove that sheet from content processes
> altogether.

The purpose of minimal-xul.css is to have all the rules that HTML/SVG needs in order to function correctly (scrollbars, form controls, etc.). So we shouldn't need xul.css in the content process other than to continue supporting the pref dom.allow_XUL_XBL_for_file.

Comment 18

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/4c8f6b544de1