Skip to content

Allow read_slice to return a Vec#487

Merged
ia0 merged 2 commits intogoogle:developfrom
ia0:read_cow
Jun 3, 2022
Merged

Allow read_slice to return a Vec#487
ia0 merged 2 commits intogoogle:developfrom
ia0:read_cow

Conversation

@ia0
Copy link
Copy Markdown
Member

@ia0 ia0 commented Jun 3, 2022

Similar to #486 but takes the opposite approach:

  • Memory-mapped storage can continue to return the slice directly
  • Storage that require a mutable slice must allocate themselves

This is preferable to #486 because:

  • Nordic has similar performance
  • File-backed storage is not for embedded so allocating is ok

If we ever need to support embedded-storage trait, then we could add an Option<&mut [u8]> argument to be used and returned in the Cow<[u8]>. This would avoid allocation and copy. But since we don't have this need, we don't do it.

@coveralls
Copy link
Copy Markdown

coveralls commented Jun 3, 2022

Coverage Status

Coverage decreased (-0.04%) to 89.907% when pulling 0f368f0 on ia0:read_cow into 85fe9cd on google:develop.

Copy link
Copy Markdown
Collaborator

@kaczmarczyck kaczmarczyck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I like this a bit better than #486. Might be a personal preference, but a quick comment on the non-intuitive use of Cow could be nice (we don't actually care about the writing part).

I find the binary size increases of both PRs surprising (in relation, at least). Would be great if we could check somehow if it's variance, but I don't see how.

@ia0 ia0 requested a review from kaczmarczyck June 3, 2022 15:03
@ia0
Copy link
Copy Markdown
Member Author

ia0 commented Jun 3, 2022

Given that in #488 the binary size didn't increase while adding code, I'm assuming this PR is rather at the bottom of the current slot of added size.

@ia0 ia0 merged commit 12f6ed6 into google:develop Jun 3, 2022
@ia0 ia0 deleted the read_cow branch June 3, 2022 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants