kernel: use MapCell for Process::stored_state #1826
Merged
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
4 tasks
alistair23
approved these changes
May 5, 2020
bradjc
approved these changes
May 5, 2020
2 tasks
hudson-ayers
approved these changes
May 6, 2020
There was a problem hiding this comment.
This looks good! This also enables us to remove the Copy bound from the StoredState associated type, and the concrete implementations of it (RiscvimacStoredState, CortexMStoredState) as we no longer copy StoredState objects anywhere -- that might be a good change to add to this PR?
Good point! Created #1833, happy to take care of it, please feel free to assign to me. |
|
bors r+ |
|
Build succeeded: |
2 tasks
bors bot
added a commit
that referenced
this pull request
May 11, 2020
1842: remove clone/copy from StoredState r=ppannuto a=hudson-ayers ### Pull Request Overview This pull request removes the Copy trait requirement for the `StoredState` trait, possible thanks to #1826 . This should help prevent performance regressions from somewhat expensive copying (at least on risc-v) of this object now that it is stored in a MapCell. Closes #1833 . ### Testing Strategy This pull request was tested by compiling. I think this is fine as we no longer seem to be using copies of `StoredState` objects anywhere. ### TODO or Help Wanted N/A ### Documentation Updated - [x] No updates are required. ### Formatting - [x] Ran `make formatall`. Co-authored-by: Hudson Ayers <hayers@stanford.edu>
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.
Pull Request Overview
Currently,
Process::stored_stateis defined as aCell. This changeaims to improve syscall latency by using
MapCellinstead.See also: #1730, #178.
Testing Strategy
I ran the same
libtock-rsapp used to test #1822 with and without thischange on opentitan on verilator. This change saves ~1200 cycles for
a
commandsyscall.TODO or Help Wanted
The main goal of
MapCellis to avoid panics at runtime, but I'm not sureif it is OK to silently continue in cases where
Process::stored_statecanpossibly be empty, which should never happen. The current
implementation exclusively uses
Cell::get, but atakecould alwaysleave a
defaultStoredStatebehind so this change doesn't seem to beworse in that regard. I tried to preserve the current behavior but please let
me know if I missed anything.
Documentation Updated
/docs, or no updates are required.Formatting
make formatall.