objstorageprovider: small readahead improvements #3607
Conversation
sumeerbhola
commented
May 10, 2024
sumeerbhola
commented
- The limit was not being updated in certain cases, which delayed when readahead would occur. This is fixed, and new test case exercises this.
- The code in recordCacheHit is almost identical to the one in maybeReadahead and the main comprehension challenge of the code is the various predicates (and their code comments with examples), which are repeated (without the examples). These are now merged into a single helper method.
- A small side-effect of the previous change is that numReads is incremented on a cache hit when we are already above the threshold. This has no actual effect on when readahead will happen. And arguably, this new behavior is more principled.
- One of the test cases had a missing - and was not being exercised, and resulting in an incorrect error in a later test case. This is fixed.
- There is additional invariant documentation.
- The limit was not being updated in certain cases, which delayed when readahead would occur. This is fixed, and new test case exercises this. - The code in recordCacheHit is almost identical to the one in maybeReadahead and the main comprehension challenge of the code is the various predicates (and their code comments with examples), which are repeated (without the examples). These are now merged into a single helper method. - A small side-effect of the previous change is that numReads is incremented on a cache hit when we are already above the threshold. This has no actual effect on when readahead will happen. And arguably, this new behavior is more principled. - One of the test cases had a missing - and was not being exercised, and resulting in an incorrect error in a later test case. This is fixed. - There is additional invariant documentation.
|
This change is |
There was a problem hiding this comment.
Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @sumeerbhola)
objstorage/objstorageprovider/readahead.go line 96 at r1 (raw file):
// rs.numReads++ if readahead {
Could we extract these two blocks from this function? This would make the semantics of the function cleaner - it just figures out if the reads matches a readahead pattern and updates the state accordingly. Each caller then does its own thing.
There was a problem hiding this comment.
TFTR!
Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @RaduBerinde)
objstorage/objstorageprovider/readahead.go line 96 at r1 (raw file):
Previously, RaduBerinde wrote…
Could we extract these two blocks from this function? This would make the semantics of the function cleaner - it just figures out if the reads matches a readahead pattern and updates the state accordingly. Each caller then does its own thing.
I am wary of that since there are other paths in this function that modify limit, prevSize, and size. It is confusing that in some cases this function will modify those fields and in other cases the caller has to modify them. And the complexity in this function is from the various nested conditions that result in this action -- the caller would need to do the same or we would need to return some bool. These choices seem harder to grok.
There was a problem hiding this comment.
Reviewable status: 0 of 3 files reviewed, all discussions resolved
objstorage/objstorageprovider/readahead.go line 96 at r1 (raw file):
Previously, sumeerbhola wrote…
I am wary of that since there are other paths in this function that modify
limit,prevSize, andsize. It is confusing that in some cases this function will modify those fields and in other cases the caller has to modify them. And the complexity in this function is from the various nested conditions that result in this action -- the caller would need to do the same or we would need to return some bool. These choices seem harder to grok.
👍
