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

Improve speed of to_string for Classes #772

Merged
merged 1 commit into from
Dec 4, 2019

Conversation

hgzimmerman
Copy link
Member

The prior code would reallocate for every item in the set that is pushed into the buffer. This should make one bigger allocation for the Vec<&str> (assuming it uses the size_hint? - worst case it makes a log_n number of allocations to accommodate the str references) and another for joining it into the final String with spaces.

@hgzimmerman hgzimmerman changed the title improve speed of to_string for Classes Improve speed of to_string for Classes Dec 4, 2019
self.set
.iter()
.map(String::as_str)
.collect::<Vec<&str>>()
Copy link
Member

@jstarry jstarry Dec 4, 2019

Choose a reason for hiding this comment

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

@hgzimmerman could you please add benchmark results to PRs with perf changes like this?

Copy link
Member Author

Choose a reason for hiding this comment

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

How do you suggest that they be integrated into the project?

This one would be easy enough to copy relevant code into a separate project, run the benchmark and throw the project away when done.
Or instead should a benchmarks directory (analogous to tests or examples) be created?
Or finally, just leave a benchmark as part of a tests or benchmarks module within the existing file?

While I await your response, I'll pursue the first option.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I was suggesting running benchmarks locally and pasting the results (ideally in the PR description). I'll add a formal process for this soon

@hgzimmerman
Copy link
Member Author

hgzimmerman commented Dec 4, 2019

Benchmarks are ran using the nightly test::Bencher.
Tests are ran on Classes constructed from variations on the string: "class1 class2 class3 ...";
old indicates the present master.
new indicates the proposed change.

Tests were ran against sets of 1, 2, 5, and 10 classes as more are unlikely to be used under typical use cases.


Results

test tests::bench_new_1  ... bench:          32 ns/iter (+/- 2)
test tests::bench_new_2  ... bench:          37 ns/iter (+/- 4)
test tests::bench_new_5  ... bench:          50 ns/iter (+/- 5)
test tests::bench_new_10 ... bench:          67 ns/iter (+/- 8)

test tests::bench_old_1  ... bench:          30 ns/iter (+/- 3)
test tests::bench_old_2  ... bench:          46 ns/iter (+/- 4)
test tests::bench_old_5  ... bench:          84 ns/iter (+/- 3)
test tests::bench_old_10 ... bench:         159 ns/iter (+/- 13)

Analysis

The proposed change is within the margin of error in the 1 class case, while meaningfully faster for inputs larger than 1.
In the broader scope of things, this change makes little difference, likely saving only a ten-thousandth of a second per full-app redraw.

Copy link
Member

@jstarry jstarry left a comment

Choose a reason for hiding this comment

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

Thanks for adding the benchmark results!

@jstarry jstarry merged commit 45574e5 into yewstack:master Dec 4, 2019
llebout pushed a commit to llebout/yew that referenced this pull request Jan 20, 2020
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.

2 participants