Closed
Bug 1345025
Opened 8 years ago
Closed 7 years ago
stylo: Use Servo for querySelector{,All}, along with stuff like Element::Matches, and Element::Closest
Categories
(Core :: CSS Parsing and Computation, enhancement, P4)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
firefox57 | --- | wontfix |
People
(Reporter: bholley, Unassigned)
References
(Blocks 1 open bug)
Details
I think this doesn't actually block shipping because we can probably continue to use the old style system (it doesn't require any rule processors or style sets). However, we'll need to do it eventually, and we'd certainly get a performance boost from doing those operations in parallel.
Reporter | ||
Updated•8 years ago
|
Summary: stylo: Use Servo for querySelector{,All} → stylo: Use Servo for querySelector{,All}, along with stuff like Element::Matches, Element::Closest and inDOMUtils::SelectorMatchesElement
Reporter | ||
Updated•8 years ago
|
Priority: -- → P4
Comment 2•7 years ago
|
||
It seems some tests may depend on that these functions take the same code path as CSS selector matching, so those test may falsely pass because they go to the Gecko code path.
I saw at least one fullscreen test relies on Element.matches to check the availability of pseudo-classes, and fail to reveal bug 1374901. (I found that bug because there happens to be another test which relies on the real selector matching path. But that test isn't meant to check that at all.)
I suspect we should probably do this to avoid false passes like what I've found above.
Comment 3•7 years ago
|
||
I suspect this is more work than it seems, in that doing it without a performance regression will be nontrivial. Just something to watch out for.
Reporter | ||
Comment 4•7 years ago
|
||
(In reply to Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-) from comment #3)
> I suspect this is more work than it seems, in that doing it without a
> performance regression will be nontrivial. Just something to watch out for.
Yeah, IMO a potential increase in test coverage accuracy isn't enough to justify putting this in-scope for now. We can certainly re-evaluate in July/August if we're in good enough shape to have time to look at this.
Reporter | ||
Comment 5•7 years ago
|
||
Per discussion today, when we do this, we probably want to have a conditional bloom filter, which we maintain only if there's a parent/ancestor combinator in the selector. This would likely reduce reduce our 10x deficit with the competition in the (rather unrealistic) microbenchmark in [1].
[1] https://webkit.org/blog-files/css-jit-introduction/html5-single-page-microbenchmark.html
Comment 6•7 years ago
|
||
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #5)
> Per discussion today, when we do this, we probably want to have a
> conditional bloom filter, which we maintain only if there's a
> parent/ancestor combinator in the selector. This would likely reduce reduce
> our 10x deficit with the competition in the (rather unrealistic)
> microbenchmark in [1].
>
> [1]
> https://webkit.org/blog-files/css-jit-introduction/html5-single-page-
> microbenchmark.html
Or use the left-to-right matching, I guess, which doesn't even need a bloom filter?
Comment 7•7 years ago
|
||
It seems inDOMUtils::SelectorMatchesElement is already using Servo.
Summary: stylo: Use Servo for querySelector{,All}, along with stuff like Element::Matches, Element::Closest and inDOMUtils::SelectorMatchesElement → stylo: Use Servo for querySelector{,All}, along with stuff like Element::Matches, and Element::Closest
Updated•7 years ago
|
Flags: needinfo?(emilio)
Updated•7 years ago
|
Blocks: stylo-everywhere
Comment 9•7 years ago
|
||
status-firefox57=wontfix unless someone thinks this bug should block 57
status-firefox57:
--- → wontfix
Comment 10•7 years ago
|
||
Bug 1410624 landed.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•