Remove uses of Arc::make_mut to avoid clones #2934
Closed
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
lu-zero
reviewed
May 9, 2022
shssoichiro
force-pushed
the
remove-make--mut
branch
3 times, most recently
from
13dc931 to
73c1d1a
Compare
shssoichiro
force-pushed
the
remove-make--mut
branch
2 times, most recently
from
5b3bf7d to
7b2cdb0
Compare
shssoichiro
force-pushed
the
remove-make--mut
branch
from
7b2cdb0 to
24ec704
Compare
lu-zero
reviewed
May 31, 2022
| // SAFETY: We're not inside a tile, there's nothing else writing to the encoder config here. | ||
| unsafe { | ||
| // Increment the film grain seed for the next frame | ||
| if let Some(params) = arc_get_mut_unsafe(&mut fi.config) |
There was a problem hiding this comment.
Wouldn't be better to move the film grain information to the FrameInvariants? Also who is accessing the config here?
lu-zero
reviewed
May 31, 2022
| fi, | ||
| ); | ||
| // SAFETY: We know no other threads are accessing this because we are done with tiles. | ||
| // The only references would be in ref frames, which are not being touched at the moment. |
There was a problem hiding this comment.
What is the gain from doing this? Maybe it should be moved to a different patch.
lu-zero
requested changes
May 31, 2022
shssoichiro
added a commit
to shssoichiro/rav1e
that referenced
this pull request
Jun 2, 2022
This partially addresses the comments in xiph#2934 by removing the unnecessary field `coded_data.lookahead_me_stats`. This data is never modified between the time it is created in `compute_lookahead_motion_vectors` and the time it is used in `compute_block_importances`, so there is no need to keep a separate copy of it. Instead, we can use the main copy of the data in `fs.frame_me_stats`. Results in ~1% peak memory usage reduction.
shssoichiro
added a commit
to shssoichiro/rav1e
that referenced
this pull request
Jun 2, 2022
This partially addresses the comments in xiph#2934 by removing the unnecessary field `coded_data.lookahead_me_stats`. This data is never modified between the time it is created in `compute_lookahead_motion_vectors` and the time it is used in `compute_block_importances`, so there is no need to keep a separate copy of it. Instead, we can use the main copy of the data in `fs.frame_me_stats`. Results in ~1% peak memory usage reduction.
shssoichiro
added a commit
that referenced
this pull request
Jun 2, 2022
This partially addresses the comments in #2934 by removing the unnecessary field `coded_data.lookahead_me_stats`. This data is never modified between the time it is created in `compute_lookahead_motion_vectors` and the time it is used in `compute_block_importances`, so there is no need to keep a separate copy of it. Instead, we can use the main copy of the data in `fs.frame_me_stats`. Results in ~1% peak memory usage reduction.
|
https://gist.github.com/lu-zero/ffc7152ab0aabc651f0fde3da42b4559 this fixes most of the problems |
|
All the tests pass for me when I apply these changes against master. However, I still see the significant increase in memory usage, because this is basically the same as not having an |
|
You need 1 clone for the lookahead and 1 clone for the normal encoding in theory. |
|
Closed for #2959 |
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.
Profiling revealed there to be a measurable number of clones coming out of
TileStateMut::new, which was traceable to usage ofArc::make_mut. A characteristic ofArc::make_mutis that it will clone the inner data when a thread attempts to write to it. In theory, I have concerns that this could cause data inconsistencies, although that does not seem to have happened in practice in rav1e. However, the clones are avoidable, because each place where we useArc::make_mutcan be safely assumed to be independent--i.e. there is never more than one thread accessing the same portion of the data.640x360 with 1 tile: 1.5% memory usage reduction
640x360 with 4 tiles: 4% memory usage reduction
1920x1080 with 4 tiles: 6% memory usage reduction
No measurable runtime improvement with 1 tile.
~1% runtime improvement with 4 tiles.