Extend smol_str usage #6463
Open
Extend smol_str usage #6463
+607
−513
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Heaptrack showed a lot of memory allocations when the strings
in the NamedReference got copied around. Instead, we now use
SmolStr to benefit from SSO. This already removes ~320000
allocations for the benchmark app I am using here. Runtime
also improves by ca 1.8%.
Before:
```
Benchmark 1: ./target/release/slint-viewer ../slint-perf/app.slint
Time (mean ± σ): 676.6 ms ± 15.6 ms [User: 594.6 ms, System: 77.4 ms]
Range (min … max): 662.9 ms … 703.4 ms 10 runs
allocations: 5202354
temporary allocations: 857511
```
After:
```
Benchmark 1: ./target/release/slint-viewer ../slint-perf/app.slint
Time (mean ± σ): 664.2 ms ± 6.7 ms [User: 589.2 ms, System: 74.0 ms]
Range (min … max): 659.0 ms … 682.4 ms 10 runs
allocations: 4886888
temporary allocations: 857508
```
|
|
|
rebase fail, need to reapply some hunks, give me a moment please |
This removes a lot of allocations and speeds up the compiler step
a bit. Sadly, this patch is very invasive as it touches a lot of
files. That said, each individual hunk is pretty trivial.
For a non-trivial real-world example, the impact is significant,
we get rid of ~29% of all allocations and improve the runtime by
about 4.8% (measured until the viewer loop would start).
Before:
```
Benchmark 1: ./target/release/slint-viewer ../slint-perf/app.slint
Time (mean ± σ): 664.2 ms ± 6.7 ms [User: 589.2 ms, System: 74.0 ms]
Range (min … max): 659.0 ms … 682.4 ms 10 runs
allocations: 4886888
temporary allocations: 857508
```
After:
```
Benchmark 1: ./target/release/slint-viewer ../slint-perf/app.slint
Time (mean ± σ): 632.4 ms ± 21.5 ms [User: 555.1 ms, System: 76.2 ms]
Range (min … max): 609.9 ms … 658.9 ms 10 runs
allocations: 3455770
temporary allocations: 495587
```
|
alright, ready for review now |
ogoffart
reviewed
Oct 7, 2024
| impl Struct { | ||
| /// Get the value for a given struct field | ||
| pub fn get_field(&self, name: &str) -> Option<&Value> { | ||
| self.0.get(&*normalize_identifier(name)) | ||
| } | ||
| /// Set the value of a given struct field | ||
| pub fn set_field(&mut self, name: String, value: Value) { | ||
| pub fn set_field(&mut self, name: SmolStr, value: Value) { |
There was a problem hiding this comment.
the interpreter API is public API and can unfortunately not be changed. So it must continue using String in its public interface. (in retrospect, this should have been a &str perhaps.)
This is valid for other changes in this file.
We can make an issue to change that with Slint 2.0
Before:
```
Benchmark 1: ./target/release/slint-viewer ../slint-perf/app.slint
Time (mean ± σ): 632.4 ms ± 21.5 ms [User: 555.1 ms, System: 76.2 ms]
Range (min … max): 609.9 ms … 658.9 ms 10 runs
allocations: 3455770
temporary allocations: 495587
```
After:
```
Benchmark 1: ./target/release/slint-viewer ../slint-perf/app.slint
Time (mean ± σ): 623.7 ms ± 8.4 ms [User: 549.2 ms, System: 68.4 ms]
Range (min … max): 609.3 ms … 631.5 ms 10 runs
allocations: 3393115
temporary allocations: 466610
```
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Use SmolStr in more places to reduce the number of string allocations. This slightly improves runtime performance too.
Originally, on my laptop, I measured:
After the second patch this goes down to: