Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/22164 )
Change subject: KUDU-613: Vector optimizations ...................................................................... Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/22164/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/22164/1//COMMIT_MSG@16 PS1, Line 16: The method 'InsertAndReturnEvicted' returns a : vector with the evicted entries. Rather than : returning a copy of the vector, the vector of : evicted entries is now a parameter to the function : so it's passed by pointer. > I think this doesn't make sense when working with C++11 and newer, unless I Good to know. This does seem unnecessary since in C++17 and later NRVO is guaranteed. http://gerrit.cloudera.org:8080/#/c/22164/1/src/kudu/util/slru_cache.cc File src/kudu/util/slru_cache.cc: http://gerrit.cloudera.org:8080/#/c/22164/1/src/kudu/util/slru_cache.cc@249 PS1, Line 249: EvictionsCount > This seems to be quite an expensive function to call from the computational This function is only called when the usage of the shard exceeds the capacity btw, so it won't be doing this extra computation that often. After some testing, it's clear that the evictions vector's size is rarely greater than 0 or 1. Usually, an upgrade is usually matched with one eviction unless the upgraded entry's size is much greater than the evicted entries, and at least through testing that didn't happen. Btw, the vector is a vector of SLRUHandles, not integers. All that being said, it doesn't look like this function makes much of a difference. When this is called, most of the time it will return 1. Instead of this function, maybe it's better to just pre-allocate the vector with a size of 1. As for the alternate idea, I was thinking about this and this would be doable if we had the average entry's charge in the protected segment store. I don't think adding a field for that would justify the benefits of rarely reserving capacity past 1. All that being said, I'm ok with removing this function and just reserving a capacity of 1. Given changing the return vector to an output parameter and this, it seems like this patch can be abandoned :). I didn't bother addressing the other minor feedback btw since the core of this patch is likely to be scrapped. -- To view, visit http://gerrit.cloudera.org:8080/22164 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0cb24b05724fdf5c1e975ac83c0cdc23c51f9f36 Gerrit-Change-Number: 22164 Gerrit-PatchSet: 1 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 23:59:39 +0000 Gerrit-HasComments: Yes
