Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/22132 )
Change subject: KUDU-613: Don't modify refs when moving entries ...................................................................... Patch Set 5: (4 comments) http://gerrit.cloudera.org:8080/#/c/22132/4/src/kudu/util/slru_cache.cc File src/kudu/util/slru_cache.cc: http://gerrit.cloudera.org:8080/#/c/22132/4/src/kudu/util/slru_cache.cc@311 PS4, Line 311: vector<SLRUHandle*> evicted_entries; > This will be taken up in a subsequent patch. It's possible to reserve size for this vector outside the lock under Lookup() which is on the ShardPair() level. The Lookup() function contains the only callsite for InsertAndReturnEvicted(). The mutex is only explicitly held on the ShardPair level btw. As for the second point, if the intention is to avoid copying the return value, as of C++17, NRVO is guaranteed so the return vector wouldn't be copied. There shouldn't be any difference between having a the vector to be returned like this vs using an output parameter. http://gerrit.cloudera.org:8080/#/c/22132/4/src/kudu/util/slru_cache.cc@321 PS4, Line 321: // Two refs for the handle: one from SLRUCacheSha > Ah, indeed. While you are here, could you add a comment for this line, at Done http://gerrit.cloudera.org:8080/#/c/22132/4/src/kudu/util/slru_cache.cc@346 PS4, Line 346: > This is not exactly about this patch, but so far at the call sites I see Re This is an interesting idea. Instead of having the loop on the ShardPair level, we would essentially just be moving it to within this function. 1 of the 3 metrics here would still have to be updated per iteration of the loop, the other two can just be updated once outside the loop. IIUC, 'RemoveEntriesPastCapacity' would only have to be called once at the end of the loop of this vector of handles as well. As mentioned, this is outside of the scope of the patch so I'll explore putting out a follow-up patch regarding this. Not sure I follow the point of adding complexity in regards to getting rid of input entries. Do you mind expanding on that point? Thanks! http://gerrit.cloudera.org:8080/#/c/22132/4/src/kudu/util/slru_cache.cc@482 PS4, Line 482: void SLRUCacheShardPair::Release(Handle* handle) { : SLRUHandle* e = reinterpret_cast<SLRUHandle*>(handle); : : // Release from either the probationary or the protected shard. : if (!e->in_protected_segment.load(std::memory_order_relaxed)) { : > This isn't related to this exact patch, but looking at this code brings up IIUC, your question has more to do with which segment the handle is being released from, rather than if it will be released at all. You're right regarding the fact that consistency cannot be guaranteed here. I'm not sure how to address this without completely nuking the performance of the cache by adding the mutex here. That being said, the only segment-specific thing being updated is two metrics. As mentioned, this can also be addressed in a separate patch btw. Do you have any ideas on how to guarantee consistency without nuking the performance? Would changing the in_memory_order to a different type help? Thanks for bringing this up! -- To view, visit http://gerrit.cloudera.org:8080/22132 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I643907612d43eba2c5f8dbc19d2f74f88bbca869 Gerrit-Change-Number: 22132 Gerrit-PatchSet: 5 Gerrit-Owner: Mahesh Reddy <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Kurt Deschler <[email protected]> Gerrit-Reviewer: Mahesh Reddy <[email protected]> Gerrit-Comment-Date: Fri, 06 Dec 2024 01:53:56 +0000 Gerrit-HasComments: Yes
