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

Make UseFutureHandle Clone #3529

Merged
merged 3 commits into from
Oct 12, 2024

Conversation

AdamSteinberg1
Copy link
Contributor

Description

This change makes UseFutureHandle implement Clone. All other UseXHandle structs have a cheap clone that calls Rc::clone internally, so I think UseFutureHandle should also have this functionality.

Checklist

  • I have reviewed my own code
  • I have added tests

Copy link

github-actions bot commented Nov 15, 2023

Benchmark - core

Yew Master

vnode           fastest       │ slowest       │ median        │ mean          │ samples │ iters
╰─ vnode_clone  2.471 ns      │ 3.143 ns      │ 2.477 ns      │ 2.497 ns      │ 100     │ 1000000000

Pull Request

vnode           fastest       │ slowest       │ median        │ mean          │ samples │ iters
╰─ vnode_clone  2.47 ns       │ 2.72 ns       │ 2.475 ns      │ 2.481 ns      │ 100     │ 1000000000

Copy link

github-actions bot commented Nov 15, 2023

Visit the preview URL for this PR (updated for commit d4f8b6d):

https://yew-rs-api--pr3529-clone-usefuturehandl-gaf7a4nd.web.app

(expires Fri, 24 Nov 2023 13:47:33 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Copy link

github-actions bot commented Nov 15, 2023

Size Comparison

examples master (KB) pull request (KB) diff (KB) diff (%)
async_clock 100.292 100.292 0 0.000%
boids 173.715 173.715 0 0.000%
communication_child_to_parent 92.741 92.741 0 0.000%
communication_grandchild_with_grandparent 105.765 105.765 0 0.000%
communication_grandparent_to_grandchild 101.014 101.014 0 0.000%
communication_parent_to_child 89.081 89.081 0 0.000%
contexts 106.004 106.004 0 0.000%
counter 86.122 86.122 0 0.000%
counter_functional 86.515 86.515 0 0.000%
dyn_create_destroy_apps 88.933 88.933 0 0.000%
file_upload 99.989 99.989 0 0.000%
function_memory_game 172.336 172.336 0 0.000%
function_router 349.847 349.847 0 0.000%
function_todomvc 161.175 161.175 0 0.000%
futures 229.061 229.061 0 0.000%
game_of_life 110.121 110.121 0 0.000%
immutable 185.438 185.438 0 0.000%
inner_html 79.897 79.897 0 0.000%
js_callback 109.510 109.510 0 0.000%
keyed_list 199.568 199.568 0 0.000%
mount_point 82.788 82.788 0 0.000%
nested_list 113.883 113.883 0 0.000%
node_refs 90.322 90.322 0 0.000%
password_strength 1750.379 1750.379 0 0.000%
portals 93.460 93.460 0 0.000%
router 318.744 318.744 0 0.000%
simple_ssr 140.312 140.312 0 0.000%
ssr_router 387.090 387.090 0 0.000%
suspense 115.709 115.709 0 0.000%
timer 88.556 88.556 0 0.000%
timer_functional 97.915 97.915 0 0.000%
todomvc 141.336 141.336 0 0.000%
two_apps 85.826 85.826 0 0.000%
web_worker_fib 134.762 134.762 0 0.000%
web_worker_prime 184.994 184.994 0 0.000%
webgl 82.504 82.504 0 0.000%

✅ None of the examples has changed their size significantly.

Copy link

github-actions bot commented Nov 15, 2023

Benchmark - SSR

Yew Master

Benchmark Round Min (ms) Max (ms) Mean (ms) Standard Deviation
Baseline 10 289.309 291.921 289.752 0.795
Hello World 10 482.650 508.189 488.353 8.537
Function Router 10 1619.175 1633.713 1626.074 4.732
Concurrent Task 10 1005.433 1007.091 1006.227 0.510
Many Providers 10 1147.812 1180.075 1158.336 10.303

Pull Request

Benchmark Round Min (ms) Max (ms) Mean (ms) Standard Deviation
Baseline 10 289.289 291.050 289.678 0.530
Hello World 10 483.102 511.514 491.705 9.721
Function Router 10 1596.404 1617.222 1604.978 6.699
Concurrent Task 10 1005.835 1006.592 1006.231 0.268
Many Providers 10 1137.354 1194.003 1153.236 16.604

@its-the-shrimp
Copy link
Contributor

its-the-shrimp commented Nov 17, 2023

Adding derive(Clone) expands to an impl with a O: Clone constraint, which is redundant in the case of UseFutureHandle, consider adding a manual Clone impl instead

@AdamSteinberg1
Copy link
Contributor Author

Ok I fixed it. TIL that #[derive(Clone)] will generate:

impl<O: Clone> Clone for UseFutureHandle<O> {
    fn clone(&self) -> Self {
        Self {
            inner: self.inner.clone(),
        }
    }
}

instead of what I intended:

impl<O> Clone for UseFutureHandle<O> {
    fn clone(&self) -> Self {
        Self {
            inner: self.inner.clone(),
        }
    }
}

Copy link
Member

@ranile ranile left a comment

Choose a reason for hiding this comment

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

Looks good to me

@tguichaoua
Copy link

Is there a reason it hasn't been merged yet?

@ranile ranile merged commit 197e2d5 into yewstack:master Oct 12, 2024
21 checks passed
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