Rkyv 0.8 migration #575
Merged
Rkyv 0.8 migration #575
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Turns out, this is the wrong approach. It's still useful code because it's a direct method for passing strings that doesn't rely on rkyv (so it could potentially be more efficient for very efficiency-oriented applications). But I think in the end, after writing this and seeing the errors that come from it, I think we should just abandon String<N> altogether because it doesn't get us anything.
this triggers a whole lotta changes....
minor cleanup because the warning is distracting from chasing down all the other warnings
split into a couple commits for ease of committing
split into multiple commits for easier committing
split into multiple commits for easier committing
split into multiple commits for easier committing
core xous services cluster
more regex results
codegenerator fixes
includes some light touch-up to the API to clean up a consequence of String not being copy-able anymore
some idioms are improved with String, this cleans up a bit of code in addition to the search/replace
Rust can't tell how to cast some u32_le into u32/usize automatically due to confusion with the num_derive trait, so we have to explicitly specify a chain of typecasts to clean up the conflict with the new rkyv
xous_ipc::<String> had a clear-volatile() API for zeroizing sensitive strings. This is now missing in String, so it has been manually implemented here by casting the String to a slice-u8 and then iterating through it using raw pointers and volatile writes with a compiler barrier to hopefully avoid all of the landmines I know of for zeroizing a string... commit is called out separately because this one is one of the more "dangerous" security-related changes in the API migration.
There isn't a simple regex to deal with the loss of Copy when deprecating xous_ipc, so this has to be a manual pass. Losing the copy trait means (maybe) some loss of efficiency in the API because we have to clone some structures instead of copying them, although, I think actually what might have been happening under the hood anyways is the copy was implemented as a clone-under-the-hood for the xous-ipc. Either way, this happens in the high level management structures for modals, so it's not the end of the world to incur a clone on the description of the structures -- we'd have to pay more attention if this happened e.g. at the pixel shuffling level, but a few UI strings at human-interaction rates isn't going to kill us.
The original loop takes advantage of the copy-trait that is no longer available. I think the intent of the loop was to mutate a field within the copy through each iteration so you can update the prompt to handle field validations that failed without re-allocating the ManagedPromtWithTextResponse structure. This has been rewritten to alloc that each iteration to avoid the borrow error, and instead the `prompt` itself is a string that's initialized with the initial prompt message and is updated on each iteration if the form validation fails. @gsora git seems to say you wrote this code, maybe I could use your help to review this patch.
prior use of `rkyv` reserves two bytes to record `pos` pre-pended to the rkyv record the new rkyv works without having to do that because it indexes the rkyv data off the end of the stored records. Thus, all you need to know is the total length of the byte slice that stores the serialized data. This is available as the return argument when you read a PDDB key, so we don't have to explicitly store it anymore.
Before xous-ipc's fixed-length string guaranteed this thing fit into a single page. Now, we can send potentially unbounded amounts of text between servers. I don't think it makes sense to send more than a few k of text to render on the screen at once, given our screen sizes, so I've added a patch that truncates the text to the previous length that things would truncate to. The behavior is the same as before, except that you will over-allocate memory on the heap in the sending process as you shove more string data into a TextView than can be rendered. Previously, the TextView would just truncate the data and discard the data. The truncation happens at the point where you try to hand the record off to another process, so, one could in theory try to make this recoverable by adding a layer that splits the textview into more manageable chunks or pages the data, but that is an exercise for another day.
we don't have to pass a "size" field anymore; since we're end-offset from pos, just pass pos and use that to chain records.
this allows us to handle the error. probably should propagate on to xous-ipc
|
now running on real hardware with hard test cases. some performance data rkyv-0.4.3, 500 elements in vault read out: 1390ms Basically within margin of error, possibly a slight performance improvement. Binary size is improved by about 81k but need to run the build on CI for an apples-to-apples comparison because that is also within margin of error due to e.g. debug string lengths. |
this allows for more graceful error handling in case of serialization failures
|
Alright, going to merge this so our main CI runners can get started on it. Will open new PRs for all the cleanup from here. In the end, this was about as annoying as routing a full DDR4 memory bus for an embedded CPU. Not the sort of thing you want to do every weekend, but also not so bad that it's not worth doing it if the pay-off is there. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is the WIP branch for rkyv 0.8 migration. The main consequence of the migration is the removal of xous-ipc's String type.
Merge is pending @xobs also merging flat-ipc into the kernel, there are a couple of places where it would be better to move directly to flat-ipc rather than bodge in an rkyv shim. For example, in the trng crate...