Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update selectors crate to 0.25, and enables :has and :is selectors. #65

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

Lymia
Copy link

@Lymia Lymia commented Jul 17, 2024

This is a (very minor) breaking change to the API because it adds a new field to the Select iterator in order to add caching for :nth-child.

If this is not wanted, don't pull the last commit da1d495407d5e8b5801172e1061db528fe9e0ffa.

@Lymia Lymia requested a review from a team as a code owner July 17, 2024 18:39
@rillian
Copy link
Contributor

rillian commented Jul 17, 2024

Thanks! I don't see any benchmarks. Do you have numbers for how much the selector cache helps?

@Lymia
Copy link
Author

Lymia commented Jul 18, 2024

I don't and it might take some thought as to how to construct one. From what I understand, it only matters for selectors that contain :nth-child(x) anyways, which... I'm not sure is the most common selector to care about being fast. It'd be completely legitimate to just leave out, tbh.

@Lymia
Copy link
Author

Lymia commented Jul 18, 2024

Running on an AMD Framework 13 with a Ryzen 7 7840U:

parse: test_data/real_world/vec_rustdoc.html
                        time:   [16.768 ms 16.864 ms 16.971 ms]

select: test_data/real_world/vec_rustdoc.html / p
                        time:   [446.73 µs 453.60 µs 461.04 µs]

select uncached: test_data/real_world/vec_rustdoc.html / p
                        time:   [480.22 µs 483.68 µs 487.34 µs]

select: test_data/real_world/vec_rustdoc.html / p:has(a)
                        time:   [495.69 µs 499.97 µs 504.66 µs]

select uncached: test_data/real_world/vec_rustdoc.html / p:has(a)
                        time:   [547.53 µs 553.19 µs 559.00 µs]

select: test_data/real_world/vec_rustdoc.html / p + p
                        time:   [452.18 µs 457.92 µs 466.39 µs]

select uncached: test_data/real_world/vec_rustdoc.html / p + p
                        time:   [498.54 µs 516.75 µs 542.44 µs]

select: test_data/real_world/vec_rustdoc.html / p:nth-child(4n+1)
                        time:   [501.90 µs 507.47 µs 513.52 µs]

select uncached: test_data/real_world/vec_rustdoc.html / p:nth-child(4n+1)
                        time:   [563.07 µs 568.15 µs 573.56 µs]

select: test_data/real_world/vec_rustdoc.html / p:nth-of-type(4n+1)
                        time:   [494.85 µs 518.08 µs 546.73 µs]

select uncached: test_data/real_world/vec_rustdoc.html / p:nth-of-type(4n+1)
                        time:   [551.41 µs 582.28 µs 631.73 µs]

parse: test_data/real_world/rust_wikipedia.html
                        time:   [9.9531 ms 9.9872 ms 10.024 ms]

select: test_data/real_world/rust_wikipedia.html / p
                        time:   [247.07 µs 250.44 µs 254.54 µs]

select uncached: test_data/real_world/rust_wikipedia.html / p
                        time:   [271.24 µs 274.19 µs 277.87 µs]

select: test_data/real_world/rust_wikipedia.html / p:has(a)
                        time:   [251.25 µs 252.65 µs 254.23 µs]

select uncached: test_data/real_world/rust_wikipedia.html / p:has(a)
                        time:   [284.42 µs 291.99 µs 302.83 µs]

select: test_data/real_world/rust_wikipedia.html / p + p
                        time:   [246.15 µs 247.80 µs 249.60 µs]

select uncached: test_data/real_world/rust_wikipedia.html / p + p
                        time:   [271.21 µs 273.11 µs 275.26 µs]

select: test_data/real_world/rust_wikipedia.html / p:nth-child(4n+1)
                        time:   [251.70 µs 253.96 µs 256.77 µs]

select uncached: test_data/real_world/rust_wikipedia.html / p:nth-child(4n+1)
                        time:   [396.74 µs 399.24 µs 402.05 µs]

select: test_data/real_world/rust_wikipedia.html / p:nth-of-type(4n+1)
                        time:   [251.21 µs 254.23 µs 258.04 µs]

select uncached: test_data/real_world/rust_wikipedia.html / p:nth-of-type(4n+1)
                        time:   [403.99 µs 408.67 µs 413.81 µs]

Strangely, I'm observing that the cache increases performance for even selectors that shouldn't need to use the cache. I tested with LTO turned on, and that didn't help, so it's not some inlining-related issue. I'm not sure why.

@rillian
Copy link
Contributor

rillian commented Jul 18, 2024

Thanks for adding benchmarks! Seems like a modest speed up for the expected cases? I'm just trying to balance this against the scary warning about crossing the streams with SelectorCache instances. I'm glad you added it, but it would be nice to somehow tie them to the document so they're harder to interchange in the public api.

Cargo.toml Outdated
[dependencies]
cssparser = "0.31"
html5ever = "0.27.0"
selectors = "0.25"
indexmap = "2.2.6"

[dev-dependencies]
tempfile = "3"
criterion = { version = "0.4", features = ["html_reports"] }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

version 0.5 please, to avoid cargo audit warnings.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gah. My IDE betrayed me.

@Lymia
Copy link
Author

Lymia commented Jul 18, 2024

Thanks for adding benchmarks! Seems like a modest speed up for the expected cases? I'm just trying to balance this against the scary warning about crossing the streams with SelectorCache instances. I'm glad you added it, but it would be nice to somehow tie them to the document so they're harder to interchange in the public api.

Now that I think about it and reading through the code, I don't think it's accurate. The only time it'll cause errors if you mutate a node such that it is still the same OpaqueElement, but, e.g. has different siblings or a different parent. Since, then it may be a different nth child index in two different queries, but the result is cached anyways.

I'll edit the documentation to be more specific.

@Lymia
Copy link
Author

Lymia commented Jul 19, 2024

After looking at the generated assembly, I think I do have a satisfying explanation for why the cache helps even when :nth-child() isn't used, anyways. Allocating the cache means zeroing the stack space for 6 HashMaps, and also running the Drop (since it can't statically prove nothing is allocated), and that adds up over all the elements in a document. Unfortunately, the API no longer lets you pass None in place of the nth index cache, which presumably would remove that difference.

@Lymia
Copy link
Author

Lymia commented Jul 19, 2024

There is also an active fork of selectors that is more updated: https://lib.rs/crates/parcel_selectors

I'm investigating it right now too. This would go all the way to cssparser 0.33

Edit: It'd be a LOT more work, because Selector as best I can tell in parcel_selectors takes a borrowed str instead of an owned String.
Edit 2: And it only implements parsing :has and not matching it. nvm, that fork would be a maintenance nightmare, devs only care about whether it parses not whether it works.

@Lymia
Copy link
Author

Lymia commented Jul 27, 2024

Any update on this PR? The current version should have all the reviews addressed.

@rillian
Copy link
Contributor

rillian commented Jul 29, 2024

Thanks for you patience. I'm away this week, but will follow up when I get back.

@rillian
Copy link
Contributor

rillian commented Aug 16, 2024

@Lymia Unfortunately I'm not longer with Brave, so I won't be able to merge this. Hopefully someone else will take over maintenance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants