script: Remove temporary root in fn is_element_in_view - #46678
Conversation
|
Shouldn't it just compare normally without the ptr::eq? |
Actually we can. But that would be slightly slower: servo/components/dom_struct/lib.rs Lines 154 to 156 in fc34008 |
We also have such usage elsewhere: servo/components/script/dom/window/window.rs Line 3720 in 39211dd |
|
Ok then I let somebody else decide if this is acceptable or not. |
|
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. |
sagudev
left a comment
There was a problem hiding this comment.
Needs updated PR title & description then feel free to throw it in MQ.
fn is_element_in_view by comparing raw pointerfn is_element_in_view
…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>
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>
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