perf(profiling): shrink the size of ZendFrame #2709
Conversation
BenchmarksBenchmark execution time: 2024-06-12 23:26:42 Comparing candidate commit a8fc7ff in PR branch Found 5 performance improvements and 0 performance regressions! Performance is the same for 24 metrics, 7 unstable metrics. scenario:walk_stack/1
scenario:walk_stack/99
scenario:walk_stack_instructions/1
scenario:walk_stack_instructions/50
scenario:walk_stack_instructions/99
|
morrisonlevi
force-pushed
the
levi/smaller-frames
branch
4 times, most recently
from
43f7779 to
d06f75e
Compare
| Some(str.to_string()) | ||
| let thin_str: ThinStr<'a> = unsafe { core::mem::transmute(non_null) }; | ||
| let str: &'a str = thin_str.into(); | ||
| Some(Cow::Borrowed(str)) |
There was a problem hiding this comment.
This reworked section allows us to lend the string from the string cache to the caller. The caller then allocates and copies it into a ThinString, which is equivalent to the str.to_string() it did before. This means the number of string copies is a wash from the previous version to this version (for this branch).
| } | ||
| None => { | ||
| let string = f()?; | ||
| let string = f(self.frame)?; |
There was a problem hiding this comment.
This now returns a Cow<str>. For file names, if the files are already utf-8, then the copy is avoided but we'll later make a copy into a ThinString anyway. So for usual cases, the number of copies compared to the previous version is the same (for this branch).
We have an extra copy compared to before for two cases:
- If the filename is not valid utf-8.
- The function name is built for the first time (not from cache).
The benchmarks have high hit-rates in the cache (perfect hit rates), so neither of these show up in the benchmark.
morrisonlevi
force-pushed
the
levi/smaller-frames
branch
from
d06f75e to
df75a9b
Compare
Previously, each frame was 7 * 64 bits. They are now 3 * 64 bits, at the cost that every `String` gets copied to a `ThinString`.
morrisonlevi
force-pushed
the
levi/smaller-frames
branch
from
df75a9b to
a8fc7ff
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2709 +/- ##
============================================
+ Coverage 79.12% 79.40% +0.28%
Complexity 2212 2212
============================================
Files 201 201
Lines 22451 22544 +93
============================================
+ Hits 17764 17901 +137
+ Misses 4687 4643 -44
Flags with carried forward coverage won't be shown. Click here to find out more. see 12 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
PROF-9925
Description
Previously, each frame was 7 * 64 bits. They are now 3 * 64 bits, at the cost that there are slightly more
Stringcopies than before. This PR is an attempt to see if this trade-off is worth it.The current status is that when things are fetched from cache, or if the inserting thing returns a borrowed
Cow<str>such as for filenames, then we have the same number of copies as before. Function names are going to return an ownedCow<str>, though, and in this case we have an extra copy compared to before. The benchmarks that the PR bot posts do not capture this case.Reviewer checklist