Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/22164 )
Change subject: KUDU-613: Vector optimizations ...................................................................... Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/22164/1/src/kudu/util/slru_cache.h File src/kudu/util/slru_cache.h: http://gerrit.cloudera.org:8080/#/c/22164/1/src/kudu/util/slru_cache.h@162 PS1, Line 162: size_t EvictionsCount(); Should this be a const method? Also, add a comment as it's done for all other methods around. 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 perspective. Consequently, it might increase contention in the cache when spending extra time iterating over the elements just to gauge how many elements are going to be evicted. I'm not sure this might justify an extra re-allocation of a vector of integers, especially if the number of evicted entries is most times isn't a big number (which I'm guessing is almost always the case) and vector itself have power-of-two style re-allocation strategy with non-zero initial capacity. Did you do any performance testing to validate performance improvement with this patch? Alternatively, you could find some cheap way of the upper bound estimate for the number of elements to be evicted and use it for pre-allocation of the vector of evicted elements. http://gerrit.cloudera.org:8080/#/c/22164/1/src/kudu/util/slru_cache.cc@252 PS1, Line 252: SLRUHandle* Could this be const SLRUHandle* ? http://gerrit.cloudera.org:8080/#/c/22164/1/src/kudu/util/slru_cache.cc@257 PS1, Line 257: temp_usage -= current->charge; If temp_usage is unsigned, might this underflow? Consider using ssize_t or int64_t for temp_usage instead. -- 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: Wed, 04 Dec 2024 19:51:18 +0000 Gerrit-HasComments: Yes
