1485511 - Consider sharing declaration blocks for SVG mapped attributes.

Status: RESOLVED WORKSFORME

(Core :: SVG, enhancement, P3)

Description

[Emilio Cobos Álvarez [:emilio]]

See bug 1485402 comment 4+.

Comment 1

[Emilio Cobos Álvarez [:emilio]]

It'd be a similar setup to nsHTMLCSSStyleSheet, though slightly more complicated.

Comment 2

[Daniel Holbert [:dholbert]]

Just for easy reference:
 - The testcase here is https://bugzilla.mozilla.org/attachment.cgi?id=9003185 (the attachment on bug 1485402).
 - The profile that we're hoping to optimize is https://perfht.ml/2PJE9fo (notice the long restyle there)
 - I'll quote the comments from Emilio that motivated this bug to be filed:

(Quoting Emilio Cobos Álvarez (:emilio) from bug 1485402 comment 4)
> Ah, I think I know what's going on. We create a unique declaration block for
> each SVG element with mapped attributes.
> 
> Since these elements have no other rules applied, the rule tree ends up with
> a single node for each tag with a lot of leaves, which is pretty much the
> worse case for it.
> 
> I think we could share them, the same way we share style="" attribute
> declaration blocks, and this should make this test-case basically instant.
[...]
> I looked into this, and turns out to be quite annoying because the key
> cannot be just text (unless we create a declaration block per attribute,
> which might or might be ok?), and also because of cross-document <svg:use>
> (where you have a different base URI).
> 
> We could cache them if the base uri, doc uri and principal are the same, or
> if we don't have potentially url-dependent values. Still it's probably a
> bunch of work. I filed bug 1485511 for that, but I don't think I could get
> to it really soon.
(This bug here is the bug mentioned in the last paragraph there.)

Comment 3

[violet.bugreport]

After looking into some of the details, I'm not sure if we should do this optimization.

Some of the observations:

1. If an element (regardless of SVG or not) has presentation hint, it has an associated DeclarationBlock declaring all its hints. Gecko_GetHTMLPresentationAttrDeclarationBlock https://searchfox.org/mozilla-central/source/servo/components/style/gecko/wrapper.rs#1788
2. nsHTMLCSSStyleSheet is for inline style, it essentially shares a common DeclarationBlock for elements if they have the same string in style="...". Some glue code: Gecko_GetStyleAttrDeclarationBlock https://searchfox.org/mozilla-central/source/servo/components/style/gecko/wrapper.rs#1272

So if we want to do the same for SVG presentation hint, we should share a DeclarationBlock for every individual property when they have the same value. (in that case, probably we will be caching a Declaration? since it only has 1 property, not a block...).

The concerns are:

1. It will cause many SVG elements have multiple DeclarationBlock, one for each presentation hint, will this cause perf problem?

2. The ultimate goal is to avoid constructing new DeclarationBlock for every SVG element. But the above method doesn't fit well with geometry property, since x, y, etc. values can rarely be shared. If many elements have different geometry properties, we'll be constructing more blocks than before.

I think the second case is more concerning, since in the testcase, we actually take advantage of x, y in <use> not being geometry property per current spec. If they were geometry property, then the optimization would just become useless. Actually Chrome has implemented x, y as geometry property for <use>, I think that might be a big part of the reason that Chrome is slower than us.

Emilio, what do you think?

Comment 4

[violet.bugreport]

Also, the workaround in current implementation is trivial: replacing repeating attributes with inline CSS style. e.g. overflow="visible" => style="overflow:visible"

Comment 5

[Emilio Cobos Álvarez [:emilio]]

(In reply to violet.bugreport from comment #3)

1. It will cause many SVG elements have multiple DeclarationBlock, one for each presentation hint, will this cause perf problem?

Yeah, that wouldn't be great.

2. The ultimate goal is to avoid constructing new DeclarationBlock for every SVG element. But the above method doesn't fit well with geometry property, since x, y, etc. values can rarely be shared. If many elements have different geometry properties, we'll be constructing more blocks than before.

Right, that's only useful for massive copies of shadow trees or such.

I think the second case is more concerning, since in the testcase, we actually take advantage of x, y in <use> not being geometry property per current spec. If they were geometry property, then the optimization would just become useless. Actually Chrome has implemented x, y as geometry property for <use>, I think that might be a big part of the reason that Chrome is slower than us.

I don't follow this? This will surely make the optimization not apply to the <use> elements, but that's fine, the optimization would still apply to all the linked elements, wouldn't it?

Emilio, what do you think?

Would have to implement it and measure, but I suspect this may not be worth the effort.

Comment 6

[violet.bugreport]

I don't follow this? This will surely make the optimization not apply to the <use> elements, but that's fine, the optimization would still apply to all the linked elements, wouldn't it?

Yeah, I shouldn't have described it as "useless", I actually meant "not-that-impressive". Because in that case, if N <use> elements are used, we at least needs N blocks. For large N, this hard limit is already slow enough to provide a smooth user experience as in the current testcase.

Actually, I was impressed yesterday by the drastic improvement after using s/overflow="visible"/style="overflow:visible"/. But now it seems to be only applicable to some very specific and localized situations. Unassigned... Presumably it's a WONTFIX...

Comment 7

[violet.bugreport]

See Bug 1555520 Comment 2.