Optimize EndianReader to use ptr/len #302
Conversation
src/endian_reader.rs
Outdated
| } | ||
| } | ||
| } | ||
|
|
||
| impl<Endian, T> EndianReader<Endian, T> | ||
| where | ||
| Endian: Endianity, | ||
| T: Deref<Target = [u8]> + Clone + Debug, | ||
| T: CloneStableDeref<Target = [u8]> + Clone + Debug, |
There was a problem hiding this comment.
We don't need the Clone trait bound anymore for any of these, because CloneStableDeref has Clone as a super trait.
| @@ -143,59 +149,77 @@ where | |||
| // `self.endian`. Splitting the sub-range out from the endian lets us work | |||
| // around this, making it so that only the `self.range` borrow is held active, | |||
| // not all of `self`. | |||
| // | |||
| // This also serves to encapsulate the unsafe code concerning `CloneStableDeref`. | |||
There was a problem hiding this comment.
Maybe also comment about how we must keep a handle to bytes around since the ptr is only valid while we still hold the bytes (eg because holding the bytes holds a refcount for us).
| r.range.end = r.range.start + idx.end; | ||
| r.range.start += idx.start; | ||
| assert!(r.range.start <= r.range.end); | ||
| assert!(r.range.end <= self.range.end); |
There was a problem hiding this comment.
It is nice to have all this encapsulated.
Interesting that our benches didn't show any speed up but the larger program did. I wonder if our benches are small enough that the caches are hiding some of the indirection costs for us here. |
This requires `stable_deref_trait::CloneStableDeref` for the bytes. If needed in future, we could relax that to `stable_deref_trait::StableDeref` by implementing a `Clone` for `SubRange` that recalculates the `ptr`. This doesn't show any significant change in the gimli benchmarks, but I have modified `addr2line` to use `Rc<Cow<[u8]>>`, and it shows about 15% improvement for all operations in that (both context creation and lookup).
philipc
force-pushed
the
endian-reader-stablederef
branch
from
8fc936e to
f4a43c2
Compare
|
perf reports that a large chunk of the time in the benches is spent in a move and drop_in_place, and this didn't change due to this PR. This PR did make the code smaller and remove some slice bounds checks, but they weren't being reported as hot. That move and drop aren't present in the |
This requires
stable_deref_trait::CloneStableDereffor the bytes. If needed in future, we could relax that tostable_deref_trait::StableDerefby implementing aCloneforSubRangethat recalculates theptr.This doesn't show any significant change in the gimli benchmarks, but I have modified
addr2lineto useRc<Cow<[u8]>>, and it shows about 15% improvement for all operations in that (both context creation and lookup).