1427681 - stylo: There is unreasonable number of nsStyleDisplay instances are kept for SVG images

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P2)

Description

[Xidorn Quan [:xidorn] UTC+10]

See the analysis in bug 1426295 comment 5, the document of tracking-protection-16.svg#enabled contains only 28 elements to style, but stylo generates 42 nsStyleDisplay instances, while Gecko only produces 24.

We should investigate why and try to reduce it at least to match the number of elements.

(I'll try to investigate it tomorrow.)

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

So, the analysis from bug 1426295 isn't really useful for stylo-chrome memory usage.

There are 15 extra instances of nsStyleDisplay when the SVG file is opened as a document rather than an image, which comes from the scrollbar. Reducing those, an SVG-as-image tracking-protection-16.svg#enabled only has 27 distinct nsStyleDisplay instances, which is much closer to the estimation, although the actual result doesn't match exactly what I described originally.

The style display instances my local log catches are:
<svg>
<svg>::-moz-text
<style>
<defs>
<rect>
<use>
<path>
<use>
<path>
<use>
<path>
<rect>
<use>
<path>
<use>
<path>
<use>
<path>
<line>
<line>
<use>
<path>
<g>
<div>
Viewport
HTMLScroll
Canvas

The element part of the log is listed in pre-order. There are something weird from the log, though.

Firstly, none of the <mask> elements in the file has style display, but their children have. Similarly, <path> elements directly inside <defs> don't have style display, but their clone under <use> elements has. However, the <line> under <defs> seems to have one. I have no idea why this happens.

Also, if Gecko also has the 15 extra instances of nsStyleDisplay when SVG is opened as document, it's going to have only 9 distinct nsStyleDisplay in image, which is extremely low. I'm going to check how many there actually are.

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

So, yes, in SVG-as-image, Gecko only produces 8 nsStyleDisplay instances in total for the given image... I cannot even tell how can it be achieved...

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

So... I did some frame tree dumping with additional code to show the display struct each frame gets, it seems that every SVG element in tracking-protection-16.svg#enabled except the root <svg>, but including all the anonymous content inside <use>, share the same display struct, and all the anonymous boxes outside the root <svg> element shares the same display struct, so it has very small number of unique display style.

It sounds like Stylo's rule cache should be able to do this kind of sharing as well. I'll have a look at why it isn't effective.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Servo shares reset structs in an all-or-nothing way, so that could / should explain it.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

(That is, either it shares all the structs, or it shares none)

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

I've figured out the reason, which is that the mapped attributes on the SVG elements form a level of rule node, which stops any sharing from the rule cache. Removing those attributes significantly improves sharing in that file.

Mapped attributes on inherited properties should be quite common in SVG, so I'm trying to skip that kind of rule nodes before querying the rule cache, which should help.

Another problem here is that, we cannot share the same style struct between different anonymous content tree under each <use> element, because we style the new subtree immediately each time we construct them. I'm wondering whether we can defer styling them so that they can also reuse styles from the rule cache.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #6)
> Another problem here is that, we cannot share the same style struct between
> different anonymous content tree under each <use> element, because we style
> the new subtree immediately each time we construct them. I'm wondering
> whether we can defer styling them so that they can also reuse styles from
> the rule cache.

Yeah, we should move <use> to use Shadow DOM instead of NAC, and then this would be fixed. We need to do that anyway to support display: contents in <use>, but I don't think it's trivial.

Comment 8

[Chris Peterson [:cpeterson]]

P2 because it sounds like we should fix this to reduce memory usage before we ship Stylo-chrome.

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Servo PR for improving sharing on normal SVG elements

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

After the PR in comment 9, we should be sharing more on SVG elements, but <use> subtree would still not share styles with anything else, so Stylo would still be using more than Gecko, but that can probably be in a different bug, and maybe switching that to shadow dom from nac is the way to fix it as mentioned by emilio in comment 7.

Given this, I'm going to mark this as FIXED.