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

Add use_raw_ref #3548

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ColonelThirtyTwo
Copy link

Description

Works like use_mut_ref but doesn't forcibly wrap your type in RefCell, so that users can handle more complex or specialized cases of interior mutability.

Checklist

  • I have reviewed my own code
  • I have added tests (doctest, also altered use_mut_ref to use the new hook)

Works like use_mut_ref but doesn't forcibly wrap your type in `RefCell`,
so that users can handle more complex or specialized cases of interior
mutability.
Copy link

github-actions bot commented Nov 30, 2023

Benchmark - core

Yew Master

vnode           fastest       │ slowest       │ median        │ mean          │ samples │ iters
╰─ vnode_clone  2.549 ns      │ 2.776 ns      │ 2.564 ns      │ 2.581 ns      │ 100     │ 1000000000

Pull Request

vnode           fastest       │ slowest       │ median        │ mean          │ samples │ iters
╰─ vnode_clone  2.558 ns      │ 3.597 ns      │ 2.59 ns       │ 2.613 ns      │ 100     │ 1000000000

Copy link

github-actions bot commented Nov 30, 2023

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

https://yew-rs--pr3548-use-raw-ref-g8odw0nk.web.app

(expires Fri, 17 May 2024 02:15:11 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Copy link

github-actions bot commented Nov 30, 2023

Benchmark - SSR

Yew Master

Benchmark Round Min (ms) Max (ms) Mean (ms) Standard Deviation
Baseline 10 308.818 310.588 309.272 0.572
Hello World 10 491.347 507.062 500.281 5.426
Function Router 10 1747.931 1795.857 1761.451 13.416
Concurrent Task 10 1005.368 1007.311 1006.334 0.567
Many Providers 10 1219.474 1269.123 1237.898 18.455

Pull Request

Benchmark Round Min (ms) Max (ms) Mean (ms) Standard Deviation
Baseline 10 308.865 309.195 308.975 0.122
Hello World 10 476.586 502.374 485.912 6.880
Function Router 10 1638.579 1655.422 1647.661 5.853
Concurrent Task 10 1005.785 1007.328 1006.469 0.589
Many Providers 10 1122.360 1162.456 1136.927 12.661

Copy link

github-actions bot commented Nov 30, 2023

Size Comparison

examples master (KB) pull request (KB) diff (KB) diff (%)
async_clock 97.465 97.465 0 0.000%
boids 167.688 167.688 0 0.000%
communication_child_to_parent 90.043 90.043 0 0.000%
communication_grandchild_with_grandparent 102.174 102.174 0 0.000%
communication_grandparent_to_grandchild 97.606 97.606 0 0.000%
communication_parent_to_child 86.406 86.406 0 0.000%
contexts 101.857 101.857 0 0.000%
counter 83.033 83.033 0 0.000%
counter_functional 83.466 83.466 0 0.000%
dyn_create_destroy_apps 86.407 86.407 0 0.000%
file_upload 97.464 97.464 0 0.000%
function_memory_game 163.701 163.701 0 0.000%
function_router 337.467 337.467 0 0.000%
function_todomvc 157.695 157.695 0 0.000%
futures 225.437 225.437 0 0.000%
game_of_life 105.017 105.017 0 0.000%
immutable 185.969 185.969 0 0.000%
inner_html 77.385 77.385 0 0.000%
js_callback 105.585 105.585 0 0.000%
keyed_list 193.227 193.227 0 0.000%
mount_point 80.375 80.375 0 0.000%
nested_list 110.996 110.996 0 0.000%
node_refs 87.721 87.721 0 0.000%
password_strength 1698.061 1698.061 0 0.000%
portals 90.646 90.646 0 0.000%
router 308.397 308.397 0 0.000%
simple_ssr 137.556 137.556 0 0.000%
ssr_router 374.715 374.715 0 0.000%
suspense 111.937 111.937 0 0.000%
timer 85.942 85.942 0 0.000%
timer_functional 94.722 94.722 0 0.000%
todomvc 138.840 138.840 0 0.000%
two_apps 83.090 83.090 0 0.000%
web_worker_fib 131.289 131.289 0 0.000%
web_worker_prime 181.447 181.447 0 0.000%
webgl 80.059 80.059 0 0.000%

✅ None of the examples has changed their size significantly.

@ranile ranile added breaking change A-yew Area: The main yew crate labels Nov 30, 2023
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.

I'm not sold on the use_raw_ref name but I also can't think of anything better. Going by the name alone, what is the meaning of "raw"? Perhaps we add (back) use_ref that just returns Rc<T> and keep use_mut_ref to avoid a breaking change and provide a convenience hook

Other than that, it looks good.

Copy link
Member

@futursolo futursolo left a comment

Choose a reason for hiding this comment

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

You are looking for use_memo((), |_| AtomicUsize:::new(0))?

@ColonelThirtyTwo
Copy link
Author

I'm not sold on the use_raw_ref name but I also can't think of anything better. Going by the name alone, what is the meaning of "raw"? Perhaps we add (back) use_ref that just returns Rc<T> and keep use_mut_ref to avoid a breaking change and provide a convenience hook

Other than that, it looks good.

I was stuck on 0.20 and didn't realize use_ref was renamed in the latest version.

@ColonelThirtyTwo
Copy link
Author

You are looking for use_memo((), |_| AtomicUsize:::new(0))?

I'm not really a fan of how Yew currently hides the basic hook functionality and forces you to half-use its convenience hooks for low-level things. Yes, use_memo((), ...) works, but internally it involves an RC-in-an-RC which is redundant and wasteful.

Copy link
Member

@WorldSEnder WorldSEnder left a comment

Choose a reason for hiding this comment

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

I like the iead of reintroducing this hook too. It used to be part of the base set of hooks until #2401 removed it. The reasoning there was to replace this with use_memo((), |_| initializer()) which technically does the same thing. Though as you note use_memo has two allocations although I'm not sure how much the compiler is able to optimize.

In any case, I think chosing the old name use_ref would be better because I do not know what "raw" would refer to here - I would have expected some pointer or an internal type, but there are none in the interface.

@WorldSEnder
Copy link
Member

WorldSEnder commented May 10, 2024

I have taken the liberty to do the required renaming myself. The failing clippy issue is unrelated (clippy getting smarter every day and catching previously overlooked issues). Not a cause for concern as long as the other tests pass.

You will of course still be credited with the PR ;)

@WorldSEnder WorldSEnder enabled auto-merge (squash) May 10, 2024 02:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-yew Area: The main yew crate breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants