Closed Bug 1640843 Opened 3 months ago Closed 3 months ago

Consider making invalidation for attribute changes more granular

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla78
Tracking Status
firefox78 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

Details

Attachments

(1 file)

See bug 1633409 comment 20 for a list of attribute selectors uBO injects on all pages.

This list contains stuff like [class] / [id] / [style], etc selectors, though most of them are not those.

Right now we store all attribute selectors in the same bucket, and when any of the relevant attributes changes, we match them to see if they've changed.

Unfortunately that means that every style attribute change is much slower than it needs to with uBO.

We should consider keeping track of the whole list of attributes that actually changed, and keeping the invalidation sets keyed per attribute name. That is a bit worse for mem usage, but should improve things significantly for these cases.

Well, to be more specific, we don't quite use just a single bucket. We use a SelectorMap, but the attribute selectors UBO uses are pretty generic, so in practice most of them end up in the universal bucket.

This should help out quite a bit with uBO, which has lots of very
general attribute selectors. We invalidate per attribute name rather
than using a SelectorMap, which prevents matching for attribute
selectors that can't have changed.

The idea is that this should be generally cheaper, though there are
cases where this would be a slight pesimization. For example, if there's
an attribute selector like:

my-specific-element[my-attribute] { /* ... */ }

And you change my-attribute in an element that isn't a
my-specific-element, before that the SelectorMap would've prevented us
from selector-matching completely. Now we'd still run selector-matching
for that (though the matching would be pretty cheap).

However I think this should speed up things generally, let's see what
the perf tests think before landing this though.

Assignee: nobody → emilio
Status: NEW → ASSIGNED

(In reply to Emilio Cobos Álvarez (:emilio) from comment #0)

See bug 1633409 comment 20 for a list of attribute selectors uBO injects on all pages.

I think you mean bug 1633409 comment 15.

Yes, thanks Xidorn!

And some major regressions as well ;_;

Ok, so I canceled those, because apparently the numbers that were reported as improvements / regressions were not actually Gecko performance numbers, but some sort of mozproxy metric, see bug 1640984.

So my patch is perf neutral and given that, and that it's going to improve stuff with uBO, and that it's a simplification, I think we should take it.

See Also: → 1640984
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/636b70578d73
Finer grained invalidation for attribute changes. r=heycam
Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla78
You need to log in before you can comment on or make changes to this bug.