stylo: Consider sorting the class list in the style sharing cache.

RESOLVED FIXED

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
8 months ago
8 months ago

People

(Reporter: emilio, Assigned: bholley)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

8 months ago
We are comparing class names in order right now.

This was suggested by Bobby in https://github.com/servo/servo/pull/17063, but I didn't do it there.

I think we should have some numbers before doing it, because I don't think it's common at all, and I don't think sorting is as cheap as Bobby suggests.
(Assignee)

Comment 1

8 months ago
(In reply to Emilio Cobos Álvarez [:emilio] from comment #0)
> We are comparing class names in order right now.
> 
> This was suggested by Bobby in https://github.com/servo/servo/pull/17063,
> but I didn't do it there.
> 
> I think we should have some numbers before doing it, because I don't think
> it's common at all

What numbers, exactly? It seems entirely dependent on whether the page ever uses multiple orderings of the same class list, which is something that can easily occur with dynamic scripts that .toggle() classes, but that would non-trivial to measure for the entire web, especially given the dynamic part.

> I don't think sorting is as cheap as Bobby suggests.

Can you explain why? Sorting small lists happens in-place, and is very cheap as far as I can tell from the stdlib source. I'm totally happy to limit the sort to class lists of <= 5 or so if you're worried about pessimal cases of thousands of classes.
(Assignee)

Updated

8 months ago
Assignee: nobody → bobbyholley
(Assignee)

Comment 2

8 months ago
Created attachment 8872723 [details] [diff] [review]
Sort the cached class list in ValidationInfo. v1

MozReview-Commit-ID: 3vsfP5ECzds
Attachment #8872723 - Flags: review?(emilio+bugs)
(Reporter)

Updated

8 months ago
Attachment #8872723 - Flags: review?(emilio+bugs) → review+
(Assignee)

Updated

8 months ago
Status: NEW → RESOLVED
Last Resolved: 8 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.