Skip to content

script: Remove temporary root in fn is_element_in_view - #46678

Merged
yezhizhen merged 2 commits into
servo:mainfrom
yezhizhen:root
Jul 22, 2026
Merged

script: Remove temporary root in fn is_element_in_view#46678
yezhizhen merged 2 commits into
servo:mainfrom
yezhizhen:root

Conversation

@yezhizhen

@yezhizhen yezhizhen commented Jul 21, 2026

Copy link
Copy Markdown
Member

We were creating a temporary root for comparison.
We could just compare raw pointers instead.
But we will just rely on PartialEq trait #46678 (comment) so that we may optimize in one place in future.

Testing: It compiles.
Part of #34464

@yezhizhen
yezhizhen requested a review from xiaochengh July 21, 2026 01:08
@servo-highfive servo-highfive added the S-awaiting-review There is new code that needs to be reviewed. label Jul 21, 2026
@Narfinger

Copy link
Copy Markdown
Contributor

Shouldn't it just compare normally without the ptr::eq?

Signed-off-by: Euclid Ye <yezhizhenjiakang@gmail.com>
@yezhizhen

yezhizhen commented Jul 21, 2026

Copy link
Copy Markdown
Member Author

Shouldn't it just compare normally without the ptr::eq?

Actually we can. But that would be slightly slower:

impl PartialEq for DomElement {
fn eq(&self, other: &Self) -> bool {
crate::DomObject::reflector(self) == crate::DomObject::reflector(other)

@yezhizhen

Copy link
Copy Markdown
Member Author

Shouldn't it just compare normally without the ptr::eq?

Actually we can. But that would be slightly slower:

impl PartialEq for DomElement {
fn eq(&self, other: &Self) -> bool {
crate::DomObject::reflector(self) == crate::DomObject::reflector(other)

We also have such usage elsewhere:

if !nodes.iter().any(|n| std::ptr::eq(&*(n.node), &*node)) {

@Narfinger

Copy link
Copy Markdown
Contributor

Ok then I let somebody else decide if this is acceptable or not.

@sagudev

sagudev commented Jul 21, 2026

Copy link
Copy Markdown
Member

I agree with @Narfinger, we should use normal compare here and in other cases too. Then when we optimize it (and we likely should) all places will become faster.

Signed-off-by: Euclid Ye <yezhizhenjiakang@gmail.com>

@sagudev sagudev left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Needs updated PR title & description then feel free to throw it in MQ.

@servo-highfive servo-highfive removed the S-awaiting-review There is new code that needs to be reviewed. label Jul 21, 2026
@yezhizhen yezhizhen changed the title script: Remove temporary root in fn is_element_in_view by comparing raw pointer script: Remove temporary root in fn is_element_in_view Jul 22, 2026
@yezhizhen
yezhizhen added this pull request to the merge queue Jul 22, 2026
@servo-highfive servo-highfive added the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Jul 22, 2026
Merged via the queue into servo:main with commit 65a9621 Jul 22, 2026
34 checks passed
@yezhizhen
yezhizhen deleted the root branch July 22, 2026 04:54
@servo-highfive servo-highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Jul 22, 2026
pull Bot pushed a commit to AKJUS/servo that referenced this pull request Jul 22, 2026
…parison (servo#46718)

As mentioned in
servo#46678 (comment), we
should rely on `PartialEq` for all cases, so that we just need to
optimize a single point.

Testing: This would be slightly slower because of

https://github.com/servo/servo/blob/fc340087b7411d5d736ad8705581baf16b567b1c/components/dom_struct/lib.rs#L154-L156,
which then expands into multiple crates and call chain. But other
behaviours should not change.

Part of servo#34464

Signed-off-by: Euclid Ye <yezhizhenjiakang@gmail.com>
pull Bot pushed a commit to xtqqczze/servo-servo that referenced this pull request Jul 27, 2026
Unlike claimed in
servo#46678 (comment) and
servo#46718, the result from benchmark is
counter-intuitive.
`std::ptr::eq` is actually 15% slower than
https://github.com/servo/servo/blob/fc340087b7411d5d736ad8705581baf16b567b1c/components/dom_struct/lib.rs#L154-L156.

The benchmark mimics the one used in
servo#35323.

Testing: 
`.\mach exec cargo bench -p servo-script-bindings`
```
Running benches.rs (target\release\deps\dom_partial_eq-8ef5af22310fd8f7.exe)
ptr_eq_same             time:   [598.35 ps 602.22 ps 606.18 ps]
Found 4 outliers among 100 measurements (4.00%)
  1 (1.00%) low mild
  1 (1.00%) high mild
  2 (2.00%) high severe

ptr_eq_different        time:   [615.17 ps 619.21 ps 623.09 ps]
Found 3 outliers among 100 measurements (3.00%)
  1 (1.00%) high mild
  2 (2.00%) high severe

dom_eq_same             time:   [514.00 ps 519.76 ps 525.78 ps]
Found 5 outliers among 100 measurements (5.00%)
  3 (3.00%) high mild
  2 (2.00%) high severe

dom_eq_different        time:   [514.45 ps 520.22 ps 526.39 ps]
Found 6 outliers among 100 measurements (6.00%)
  1 (1.00%) low mild
  4 (4.00%) high mild
  1 (1.00%) high severe
```

Also, production profile `.\mach exec cargo bench --profile production
-p servo-script-bindings` is always slower than default.

---------

Signed-off-by: Euclid Ye <yezhizhenjiakang@gmail.com>
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.

4 participants