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

Various improvements to Classes, oriented around reducing allocations #2870

Merged
merged 2 commits into from Sep 21, 2022

Conversation

Lucretiel
Copy link
Contributor

@Lucretiel Lucretiel commented Sep 14, 2022

Description

  • push no longer performs unnecessary allocations if self is empty. - Most constructors (FromIterator, From, Extend) that were already based around Into<Classes> are now oriented around push, to take advantage of this
  • to_string and into_prop_value:
    • No longer allocate an unnecessary Vec; they instead preallocate a string of the correct length and push into it directly.
    • No longer use a length check + unsafe; they instead match over a fallible .next() and proceed from there.
    • Additionally, into_prop_value no longer builds a String or Rc if Classes contains a single &'static str or is empty.
  • From<String> no longer clones the string if it contains a single class.
    • This mainly benefits FromIterator<String> and Iterator<&str>.map(to_owned)
  • impl Eq for Classes

Checklist

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

- `push` no longer performs unnecessary allocations if `self` is empty.
    - Most constructors (FromIterator, From, Extend) are now oriented around push, to take advantage of this
- `to_string` and `into_prop_value`:
  - No longer allocate an unnecessary `Vec`; they instead preallocate a string of the correct length and push into it directly.
  - No longer use a length check + `unsafe`; they instead match over a fallible .next() and proceed from there.
  - Additionally, `into_prop_value` no longer builds a `String` or `Rc` if `Classes` contains a single `&'static str` or is empty.
- `From<String>` no longer clones the string if it contains a single class.
- `impl Eq for Classes`
@Lucretiel
Copy link
Contributor Author

Note: in the future, as a slightly breaking change, we should consider redoing some of the implementation patterns around Classes construction, especially around FromIterator and Extend, which currently perform a lot of unnecessary intermediary allocations (because each item in the iterator is converted into a new Classes, which is then merged into the local one.

@github-actions
Copy link

github-actions bot commented Sep 14, 2022

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

https://yew-rs-api--pr2870-improved-classes-psuc2ffr.web.app

(expires Sat, 24 Sep 2022 01:23:58 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

@github-actions
Copy link

github-actions bot commented Sep 14, 2022

Benchmark - SSR

Yew Master

Benchmark Round Min (ms) Max (ms) Mean (ms) Standard Deviation
Baseline 10 345.542 346.629 346.056 0.377
Hello World 10 598.836 601.203 599.581 0.640
Function Router 10 2256.107 2294.039 2268.111 11.198
Concurrent Task 10 1008.154 1010.020 1008.985 0.706

Pull Request

Benchmark Round Min (ms) Max (ms) Mean (ms) Standard Deviation
Baseline 10 307.253 308.576 307.776 0.427
Hello World 10 602.793 627.080 605.853 7.493
Function Router 10 2224.540 2239.502 2230.902 4.526
Concurrent Task 10 1008.441 1010.044 1009.290 0.635

@github-actions
Copy link

github-actions bot commented Sep 14, 2022

Size Comparison

examples master (KB) pull request (KB) diff (KB) diff (%)
boids 170.231 170.229 -0.002 -0.001%
communication_child_to_parent 90.467 90.468 +0.001 +0.001%
communication_grandchild_with_grandparent 105.416 105.428 +0.012 +0.011%
communication_grandparent_to_grandchild 101.318 101.310 -0.009 -0.009%
communication_parent_to_child 87.606 87.604 -0.002 -0.002%
contexts 107.848 107.852 +0.004 +0.004%
counter 85.480 85.477 -0.004 -0.005%
counter_functional 86.010 86.007 -0.003 -0.003%
dyn_create_destroy_apps 88.380 88.380 0 0.000%
file_upload 100.059 100.059 0 0.000%
function_memory_game 163.776 163.145 -0.632 -0.386%
function_router 348.591 347.832 -0.759 -0.218%
function_todomvc 158.617 157.930 -0.688 -0.433%
futures 221.967 222.308 +0.341 +0.154%
game_of_life 105.718 105.030 -0.688 -0.650%
immutable 181.698 181.695 -0.003 -0.002%
inner_html 82.346 82.344 -0.002 -0.002%
js_callback 111.382 111.366 -0.016 -0.014%
keyed_list 195.565 195.560 -0.006 -0.003%
mount_point 85.099 85.097 -0.002 -0.002%
nested_list 112.600 111.926 -0.674 -0.598%
node_refs 92.984 92.990 +0.006 +0.006%
password_strength 1547.438 1547.438 0 0.000%
portals 96.291 96.291 0 0.000%
router 318.268 317.483 -0.784 -0.246%
simple_ssr 152.193 152.197 +0.004 +0.003%
ssr_router 394.141 393.386 -0.755 -0.192%
suspense 109.107 109.105 -0.002 -0.002%
timer 88.297 88.295 -0.002 -0.002%
todomvc 139.716 139.036 -0.680 -0.486%
two_apps 86.091 86.091 0 0.000%
web_worker_fib 152.230 152.231 +0.001 +0.001%
webgl 84.785 84.785 0 0.000%

✅ None of the examples has changed their size significantly.

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.

Thank you for the pull request.

packages/yew/src/html/classes.rs Show resolved Hide resolved
packages/yew/src/html/classes.rs Outdated Show resolved Hide resolved
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.

Thank you for the pull request. Looks good to me.

Cost of creating 1,000,000 Classes with 5 classes and convert them to a String:

Round: 1 (Warm up)
&'static str (pull request): 439ms
&'static str (master): 404ms
String (pull request): 673ms
String (master): 679ms
Round: 2
&'static str (pull request): 399ms
&'static str (master): 403ms
String (pull request): 673ms
String (master): 678ms
Round: 3
&'static str (pull request): 397ms
&'static str (master): 401ms
String (pull request): 676ms
String (master): 679ms
Round: 4
&'static str (pull request): 399ms
&'static str (master): 404ms
String (pull request): 669ms
String (master): 674ms
Round: 5
&'static str (pull request): 396ms
&'static str (master): 401ms
String (pull request): 668ms
String (master): 677ms

@futursolo futursolo added the A-yew Area: The main yew crate label Sep 18, 2022
@futursolo futursolo merged commit 6751946 into yewstack:master Sep 21, 2022
@hamza1311
Copy link
Member

@futursolo how did you run the benchmarks? Can those be run as part alongside the other benchmarks that we have?

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants