Make Storage::read_slice take a mutable slice#486
Make Storage::read_slice take a mutable slice#486ia0 wants to merge 2 commits intogoogle:developfrom
Conversation
ia0
force-pushed
the
read_mut
branch
2 times, most recently
from
3a26a18 to
6776248
Compare
ia0
changed the title
| // The slice spans the next page. | ||
| let index = pos.next_page(&self.format).index(&self.format); | ||
| result.extend_from_slice(self.storage_read_slice(index, length - max_length)); | ||
| self.storage_read_slice(index, &mut result[max_length as usize..]); |
There was a problem hiding this comment.
Double checking: I was wondering how much more copying we will have to do with this PR. Looks like we have to copy up to 2 pages now, instead of just the final slice that has always been a copy?
There was a problem hiding this comment.
Yes, there will be more allocations and more copying with this PR.
However, for the specific code on which this comment is attached, there's actually no additional allocation or copy. That's probably one of the rare exceptions. For me the most problematic place would be when reading a page just to check that it's erased (this doesn't happen often, at boot and compaction) and when writing a slice to check that the write is valid (this happens at store mutation). When reading, we don't have additional penalty, which is actually ironic since the PR is only changing the reading API. This comes from the fact that the store API is already allocating and copying, so we don't add more. The allocation and copying during store mutation (insert, remove, compaction, etc) is acceptable because it's dominated by the cost of the actual flash write. However the allocation and copying for booting the store is more problematic. But since it happens at boot time, the extra cost is less sensitive.
There was a problem hiding this comment.
Do you have a store benchmark you can quickly run, out of curiousity?
There was a problem hiding this comment.
Good point, I forgot about it. I've updated the numbers. Here's a summary:
- Boot is 35% slower
- Compaction is 5% slower
- Insert is 30% slower
- Remove is 35% slower
There was a problem hiding this comment.
While that may not be problematic, how hard would it be to have an API that is still general over different use cases (files and embedded), but less performance issues on embedded? Just to understand the alternative.
There was a problem hiding this comment.
Indeed, there's an alternative that we may prefer: #487 .
I believe there would be an ideal alternative. I don't think it's hard to implement, so I might just try.
|
Closing in favor of #487 |
3 participants
As mentioned in this comment, the
Storage::read_sliceAPI is too strong because some implementations cannot return a slice. This PR changes the API to take a mutable slice instead. This essentially enables 2 use-cases:embedded_storage::nor_flash::NorFlash