-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve the efficiency of BufferedBlockDevice #8703
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No documentation changes?
No - no need to. Functionality is just the same, just more efficient. |
@@ -26,7 +26,8 @@ static inline uint32_t align_down(bd_size_t val, bd_size_t size) | |||
} | |||
|
|||
BufferedBlockDevice::BufferedBlockDevice(BlockDevice *bd) | |||
: _bd(bd), _bd_program_size(0), _curr_aligned_addr(0), _flushed(true), _cache(0), _init_ref_count(0), _is_initialized(false) | |||
: _bd(bd), _bd_program_size(0), _bd_read_size(0), _write_cache_addr(0), _write_cache_valid(false), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this constructor change not need new documentation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't look like the constructor params changed. These are member initializers, which are internal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curse that lone :
/morph build |
Build : SUCCESSBuild number : 3609 Triggering tests/morph test |
Exporter Build : SUCCESSBuild number : 3213 |
Test : FAILUREBuild number : 3385 |
4cd7813
to
2629d2d
Compare
Failure in NRF51 fixed (all other failures are not related to this PR). |
/morph export-build |
/morph build |
Build : SUCCESSBuild number : 3618 Triggering tests/morph test |
Exporter Build : SUCCESSBuild number : 3223 |
Exporter Build : SUCCESSBuild number : 3226 |
Test : FAILUREBuild number : 3401 |
Test failures appear to be a whole bunch of sporadic netsocket echo issues, unrelated to CI. Will restart when able. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great 👍
@@ -110,36 +124,52 @@ int BufferedBlockDevice::sync() | |||
|
|||
int BufferedBlockDevice::read(void *b, bd_addr_t addr, bd_size_t size) | |||
{ | |||
MBED_ASSERT(_cache); | |||
MBED_ASSERT(_write_cache); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be?
MBED_ASSERT(_write_cache); | |
MBED_ASSERT(_read_cache); |
Also should there be a similar assert in BlockDevice::program?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@davidsaada ^^^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cmonr, given the dependencies, it's probably better to just move forward with testing. The assert is minor and can be fixed after the release (or if @davidsaada has to make further changes).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@geky Alright, that sounds reasonable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Used those damned netsocket test failures to fix these asserts.
/morph test |
2629d2d
to
50836e7
Compare
@davidsaada Was this PR just rebased? |
Rebased it as part of the fixes (assert fixes). |
Info: This PR has now been bundled into a rollup PR (#8760). The CI failures that this PR saw do not appear to be the fault of the PR. To save time in retesting this PR, along with other PRs that experienced similar issues, it has been bundled into a rollup PR. If any more commits are made in this PR, this PR will remain open and have to go through CI on its own. |
Info: This PR has been re-bundled into a new rollup PR (#8763). The previous rollup found an issue with a bundled PR after new devices were added into master. The PR needing work has been removed. No further work is needed here, as once that PR is merged, this PR will also be closed and marked as merged. |
Description
Previous implementation of BufferedBlockDevice was extremely naive: It had a cache used for both writing and reading, meaning that a read action would always flush the write cache as well. In addition, write operations were split to chunks of program units, meaning that each BufferedBlockDevice
program
call would trigger plenty of the underlying BD's calls of this API. Reads did pretty much the same.This change does the following:
Pull request type