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

use_future_with: simplify code a bit by using read-only use_memo rather than use_state #3610

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Ekleog
Copy link

@Ekleog Ekleog commented Feb 21, 2024

Description

Slight code simplification: here latest_id is never used as mutable, so might as well make it non-mutable by using use_memo_base

Checklist

  • I have reviewed my own code

There was no relevant test to add for this code simplification

Copy link

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

https://yew-rs-api--pr3610-simplify-use-future-9wkk8znq.web.app

(expires Wed, 28 Feb 2024 23:02:04 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Copy link

Benchmark - core

Yew Master

vnode           fastest       │ slowest       │ median        │ mean          │ samples │ iters
╰─ vnode_clone  2.783 ns      │ 4.739 ns      │ 4.016 ns      │ 3.989 ns      │ 100     │ 1000000000

Pull Request

vnode           fastest       │ slowest       │ median        │ mean          │ samples │ iters
╰─ vnode_clone  2.779 ns      │ 5.936 ns      │ 4.016 ns      │ 3.835 ns      │ 100     │ 1000000000

Copy link

github-actions bot commented Feb 21, 2024

Size Comparison

examples master (KB) pull request (KB) diff (KB) diff (%)
async_clock 100.530 100.530 0 0.000%
boids 173.547 173.547 0 0.000%
communication_child_to_parent 93.072 93.072 0 0.000%
communication_grandchild_with_grandparent 105.655 105.655 0 0.000%
communication_grandparent_to_grandchild 101.043 101.043 0 0.000%
communication_parent_to_child 89.413 89.413 0 0.000%
contexts 105.821 105.821 0 0.000%
counter 86.281 86.281 0 0.000%
counter_functional 86.451 86.451 0 0.000%
dyn_create_destroy_apps 89.275 89.275 0 0.000%
file_upload 100.440 100.440 0 0.000%
function_memory_game 172.253 172.253 0 0.000%
function_router 349.531 349.531 0 0.000%
function_todomvc 162.304 162.304 0 0.000%
futures 229.101 229.101 0 0.000%
game_of_life 109.982 109.982 0 0.000%
immutable 189.679 189.679 0 0.000%
inner_html 80.040 80.040 0 0.000%
js_callback 109.576 109.153 -0.423 -0.386%
keyed_list 198.589 198.589 0 0.000%
mount_point 82.877 82.877 0 0.000%
nested_list 114.631 114.631 0 0.000%
node_refs 90.462 90.462 0 0.000%
password_strength 1726.314 1726.314 0 0.000%
portals 93.754 93.754 0 0.000%
router 318.227 318.227 0 0.000%
simple_ssr 141.650 141.650 0 0.000%
ssr_router 389.408 389.408 0 0.000%
suspense 116.107 116.107 0 0.000%
timer 88.976 88.976 0 0.000%
timer_functional 97.995 97.995 0 0.000%
todomvc 142.318 142.318 0 0.000%
two_apps 85.590 85.590 0 0.000%
web_worker_fib 136.037 136.037 0 0.000%
web_worker_prime 186.757 186.757 0 0.000%
webgl 82.662 82.662 0 0.000%

✅ None of the examples has changed their size significantly.

Copy link

Benchmark - SSR

Yew Master

Benchmark Round Min (ms) Max (ms) Mean (ms) Standard Deviation
Baseline 10 289.504 290.573 289.859 0.398
Hello World 10 485.538 496.308 488.704 3.024
Function Router 10 1663.645 1681.469 1674.381 6.332
Concurrent Task 10 1005.563 1006.888 1006.230 0.461
Many Providers 10 1107.094 1149.811 1127.156 11.126

Pull Request

Benchmark Round Min (ms) Max (ms) Mean (ms) Standard Deviation
Baseline 10 289.421 290.134 289.712 0.250
Hello World 10 484.111 492.304 486.684 3.354
Function Router 10 1649.225 1666.995 1657.593 5.900
Concurrent Task 10 1005.321 1006.563 1005.971 0.427
Many Providers 10 1096.474 1142.509 1111.645 15.706

@Ekleog
Copy link
Author

Ekleog commented Feb 21, 2024

It looks like CI is broken in ways most likely unrelated to this PR?

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.

Simple yet effective.

The feature linting failures come from know extra lints that got introduced at some point that we haven't gotten around to fix yet.

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.

With #3548 this could actually be even simpler: use_raw_ref(|| Cell::new(0u32))

@Ekleog
Copy link
Author

Ekleog commented Feb 22, 2024

Yup, sounds like it could get even simpler! But I'll probably have completely forgotten about this once #3548 lands, so… 😅

@Ekleog
Copy link
Author

Ekleog commented May 7, 2024

@WorldSEnder Should we just land this, or do you want to wait for #3548 to land before?

@WorldSEnder
Copy link
Member

@Ekleog depending on #3548 was the idea lest we rewrite something soon after. I've finished up what I think was missing in that PR before we can merge, so any day now.

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.

None yet

2 participants