-
Notifications
You must be signed in to change notification settings - Fork 95
H-3330: Optimize allocation behavior during report construction #5161
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
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5161 +/- ##
=======================================
Coverage ? 19.42%
=======================================
Files ? 508
Lines ? 17126
Branches ? 2546
=======================================
Hits ? 3327
Misses ? 13787
Partials ? 12
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Benchmark results
|
Function | Value | Mean | Flame graphs |
---|---|---|---|
entity_by_id | 10 entities | Flame Graph | |
entity_by_id | 50 entities | Flame Graph | |
entity_by_id | 1 entities | Flame Graph | |
entity_by_id | 25 entities | Flame Graph | |
entity_by_id | 5 entities | Flame Graph |
scaling_read_entity_complete_zero_depth
Function | Value | Mean | Flame graphs |
---|---|---|---|
entity_by_id | 10 entities | Flame Graph | |
entity_by_id | 50 entities | Flame Graph | |
entity_by_id | 1 entities | Flame Graph | |
entity_by_id | 25 entities | Flame Graph | |
entity_by_id | 5 entities | Flame Graph |
scaling_read_entity_linkless
Function | Value | Mean | Flame graphs |
---|---|---|---|
entity_by_id | 10 entities | Flame Graph | |
entity_by_id | 100 entities | Flame Graph | |
entity_by_id | 1000 entities | Flame Graph | |
entity_by_id | 1 entities | Flame Graph | |
entity_by_id | 10000 entities | Flame Graph |
representative_read_entity_type
Function | Value | Mean | Flame graphs |
---|---|---|---|
get_entity_type_by_id | Account ID: d4e16033-c281-4cde-aa35-9085bf2e7579
|
Flame Graph |
representative_read_entity
Function | Value | Mean | Flame graphs |
---|---|---|---|
entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/song/v/1
|
Flame Graph | |
entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/block/v/1
|
Flame Graph | |
entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/book/v/1
|
Flame Graph | |
entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/page/v/2
|
Flame Graph | |
entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/person/v/1
|
Flame Graph | |
entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/uk-address/v/1
|
Flame Graph | |
entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/playlist/v/1
|
Flame Graph | |
entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/building/v/1
|
Flame Graph | |
entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/organization/v/1
|
Flame Graph |
representative_read_multiple_entities
Function | Value | Mean | Flame graphs |
---|---|---|---|
link_by_source_by_property | depths: DT=2, PT=2, ET=2, E=2 | Flame Graph | |
link_by_source_by_property | depths: DT=0, PT=0, ET=0, E=2 | Flame Graph | |
link_by_source_by_property | depths: DT=0, PT=0, ET=0, E=0 | Flame Graph | |
link_by_source_by_property | depths: DT=0, PT=0, ET=2, E=2 | Flame Graph | |
link_by_source_by_property | depths: DT=0, PT=2, ET=2, E=2 | Flame Graph | |
link_by_source_by_property | depths: DT=255, PT=255, ET=255, E=255 | Flame Graph | |
entity_by_property | depths: DT=2, PT=2, ET=2, E=2 | Flame Graph | |
entity_by_property | depths: DT=0, PT=0, ET=0, E=2 | Flame Graph | |
entity_by_property | depths: DT=0, PT=0, ET=0, E=0 | Flame Graph | |
entity_by_property | depths: DT=0, PT=0, ET=2, E=2 | Flame Graph | |
entity_by_property | depths: DT=0, PT=2, ET=2, E=2 | Flame Graph | |
entity_by_property | depths: DT=255, PT=255, ET=255, E=255 | Flame Graph |
putting in draft for now as a consideration for additional future changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR introduces significant optimizations to error-stack's allocation behavior by implementing a backing vector approach. The changes look well-thought-out and should lead to improved performance when creating large error reports.
The main improvements include:
-
Reduced allocations: By using a fragment-based approach with a backing vector, the implementation can avoid frequent reallocations when building up error reports.
-
Smart memory management: The use of
RawSlice
and the fragment capacity doubling strategy should lead to more efficient memory usage patterns. -
Unsized frames: Making
Frame
unsized and only accessible through references provides stronger guarantees about how frames are allocated and managed.
While the implementation is generally sound, I have some concerns about the safety of mutable slices and how errors are handled in some parts of the code. Additionally, some of the comments could be clearer to help future maintainers understand the design decisions.
Overall, this is a valuable optimization that should improve performance, but a few details could be refined to make the code more robust and maintainable.
// - `Frame`s and their sources coexist within the same report structure. | ||
unsafe { core::slice::from_raw_parts(self.ptr.as_ptr(), self.len) } | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The as_slice_mut
method is returning a mutable reference to memory that could potentially be referenced elsewhere. Consider adding documentation that explains why this is safe, specifically about the ownership model that prevents aliasing.
fn append<T>(&mut self, frames: T) -> Result<RawSlice, T> | ||
where | ||
T: Iterator<Item = Box<Frame>> + ExactSizeIterator, | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The append
function could be improved with better error handling. Instead of returning T
on error, consider returning a Result<RawSlice, InsertionError>
where InsertionError
contains the original frames. This makes the error path more explicit and follows Rust's error handling patterns.
Ok(RawSlice { ptr, len }) | ||
} | ||
|
||
fn capacity(&self) -> usize { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The INITIAL_FRAGMENT_CAPACITY
constant is a good start for tuning, but it might be worth making this configurable via a feature flag or build-time configuration, especially if this is an experimental optimization that might be benchmarked with different values.
pub(crate) fn union(&mut self, mut other: Self) { | ||
// pretty simple and won't lead to any data being reallocated that is referenced to by a | ||
// `RawSlice` | ||
if self.last_fragment_capacity() > other.last_fragment_capacity() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment about "front loading" fragments is a bit confusing. Consider clarifying this with more detailed explanation about the memory layout implications of this approach and why it improves performance.
@@ -578,40 +581,37 @@ impl<C: ?Sized> Report<C> { | |||
where | |||
T: Context, | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two calls to layer
should be combined into a single layer operation for clarity, or at least have a comment explaining why they're separated. Typically when we add a context and a location, they should be bundled together as a single unit.
🌟 What is the purpose of this PR?
This optimizes the allocation behavior of a report by introducing a backing vector.
This is an experimental change and might not be merged into error-stack!
Pre-Merge Checklist 🚀
🚢 Has this modified a publishable library?
This PR:
📜 Does this require a change to the docs?
The changes in this PR:
🕸️ Does this require a change to the Turbo Graph?
The changes in this PR: