FDB-427 Limit memory usage on remotefdb.read #96
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #96 +/- ##
===========================================
- Coverage 64.25% 63.92% -0.33%
===========================================
Files 278 280 +2
Lines 15713 15806 +93
Branches 1632 1644 +12
===========================================
+ Hits 10096 10104 +8
- Misses 5617 5702 +85 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| @@ -93,7 +93,8 @@ eckit::DataHandle* RemoteFieldLocation::dataHandle() const { | |||
| remote.query("internalHost", ""); | |||
| } | |||
| remote.query("internalScheme", ""); | |||
| FieldLocation* loc = FieldLocationFactory::instance().build(scheme, remote, offset_, length_, remapKey_); | |||
| std::unique_ptr<FieldLocation> loc( | |||
There was a problem hiding this comment.
NB: this was a fly-by fix of a memory-leak.
| // When a RemoteStore is destroyed, it must evict any unconsumed requests. | ||
| // If all went well, there will be no requests to evict, but we must check (consumer may have abandoned the | ||
| // request). | ||
| /// @todo: This is somewhat pointless right now because the RemoteStores appear to be infinitely long lived... |
There was a problem hiding this comment.
This seems surprising. Are these not owned by something with a finite lifetime?
There was a problem hiding this comment.
Not as far as I can tell:
RemoteStore& RemoteStore::get(const eckit::URI& uri) {
static std::mutex storeMutex_;
// we memoise one read store for each endpoint. Do not need to have one for each key
static std::map<std::string, std::unique_ptr<RemoteStore>> readStores_;
std::lock_guard<std::mutex> lock(storeMutex_);
const std::string& endpoint = uri.hostport();
auto it = readStores_.find(endpoint);
if (it != readStores_.end()) {
return *(it->second);
}
return *(readStores_[endpoint] = std::make_unique<RemoteStore>(uri, Config()));
}Where RemoteStore::get is a static function
|
|
||
| // Attempt to send the next request in the queue. Returns true if a request was sent. | ||
| // If not enough memory is available, or there is no next request, returns false. | ||
| bool tryNextRequest(); |
There was a problem hiding this comment.
It is a bit odd that the return value for this method doesn't seem to ever be used
There was a problem hiding this comment.
I figured there could one day be contexts where it was useful to know whether the request was actually sent. But can make void if this is premature.
ChrisspyB
force-pushed
the
fix/FDB-427-memory-limit
branch
from
9e7ff15 to
c612266
Compare
|
We would like to have more fine grained control of queue sizes. We have decided this will be implemented in a followup PR, for the sake of expediency. I will just make some small changes here first before we merge. |
ChrisspyB
force-pushed
the
fix/FDB-427-memory-limit
branch
from
58f1e43 to
34d522f
Compare
(Only relevant to RemoteFDB)
Introduces the ReadLimiter class, which tracks the total size of fields already requested from the store servers and prevents requesting more fields until the exisitng ones have been consumed. The FDBRemoteDataHandle is responsible for signalling to the ReadLimiter when its field has been consumed.
RemoteStores no longer directly send their request to a server, they now first send it to the ReadLimiter class, which holds onto the request until there is sufficient memory to process it.