Alexey Serbin 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 4: Code-Review+1 (3 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@321 PS4, Line 321: handle->refs.store(2, std::memory_order_relaxed); > Yep this is still necessary. The use case of ProtectedInsert is for the upd Ah, indeed. While you are here, could you add a comment for this line, at least to let the reader know that's the insertion path for a new entry (something similar to what's in the Insert() method at line 270 in PS4), even if the key is the same? Without having enough context on this code, it's not trivial to see this right away. Thanks! http://gerrit.cloudera.org:8080/#/c/22132/4/src/kudu/util/slru_cache.cc@346 PS4, Line 346: ReInsert(SLRUHandle* handle) This is not exactly about this patch, but so far at the call sites I see ReInsert() called in a cycle for each element of a bunch of soft-removed entries. Should this method change its signature to accept a vector of handles, so all the metrics updates and especially 'RemoveEntriesPastCapacity()' would be called only once for a bunch of entries being re-inserted? That could add a bit of complexity to even get rid of a part of the input entries (according to the LRU policy), but that might be a positive change since the we aren't going to perform the add-and-then-remove dance with them. What do you think? http://gerrit.cloudera.org:8080/#/c/22132/4/src/kudu/util/slru_cache.cc@482 PS4, Line 482: // Release from either the probationary or the protected shard. : if (!e->in_protected_segment.load(std::memory_order_relaxed)) { : probationary_shard_.Release(handle); : } else { : protected_shard_.Release(handle); : } This isn't related to this exact patch, but looking at this code brings up a question: what if the handle is migrating right at this moment from the protected to the probationary segment because of concurrent insertion of some other entry into the cache? How do we guarantee the consistency here in such a case -- the 'mutex_' primitive isn't acquired here, and 'in_protected_segment' atomic field is being evaluated with std::memory_order_relaxed sequencing. IIUC, there isn't a guarantee on whether the entry is going to be released in such a case, right? What do you think? -- 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: 4 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: Thu, 05 Dec 2024 01:24:12 +0000 Gerrit-HasComments: Yes
