Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/24401 )
Change subject: IMPALA-14702 (prep): Add an abstraction around malloc implementation details ...................................................................... Patch Set 2: (5 comments) http://gerrit.cloudera.org:8080/#/c/24401/2/be/src/runtime/bufferpool/system-allocator.cc File be/src/runtime/bufferpool/system-allocator.cc: http://gerrit.cloudera.org:8080/#/c/24401/2/be/src/runtime/bufferpool/system-allocator.cc@51 PS2, Line 51: CHECK(support == MallocUtil::HugePageSupport::MADVISE_COMPATIBLE); Why switch to CHECK(... == ...) from CHECK_EQ(..., ...)? http://gerrit.cloudera.org:8080/#/c/24401/2/be/src/runtime/exec-env.cc File be/src/runtime/exec-env.cc: http://gerrit.cloudera.org:8080/#/c/24401/2/be/src/runtime/exec-env.cc@426 PS2, Line 426: if (FLAGS_tcmalloc_max_total_thread_cache_bytes == 0) { Should updating flags be part of MallocUtil::Init? http://gerrit.cloudera.org:8080/#/c/24401/2/be/src/runtime/query-exec-mgr.cc File be/src/runtime/query-exec-mgr.cc: http://gerrit.cloudera.org:8080/#/c/24401/2/be/src/runtime/query-exec-mgr.cc@176 PS2, Line 176: string s = MallocUtil::GetInstance()->GetTextDescription(); nit: 2 lines doesn't seem to make this any more readable; the variable name is not descriptive. http://gerrit.cloudera.org:8080/#/c/24401/2/be/src/util/malloc-util.h File be/src/util/malloc-util.h: http://gerrit.cloudera.org:8080/#/c/24401/2/be/src/util/malloc-util.h@32 PS2, Line 32: static Status Allocate(); The name Allocate seems overloaded here. What about Create? http://gerrit.cloudera.org:8080/#/c/24401/2/be/src/util/malloc-util.h@37 PS2, Line 37: // Initialize the settings for the malloc implemenation. This should be called once typo, should be "implementation" -- To view, visit http://gerrit.cloudera.org:8080/24401 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3ac7841cf279ac40141147d4be9192894d6dc908 Gerrit-Change-Number: 24401 Gerrit-PatchSet: 2 Gerrit-Owner: Joe McDonnell <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Comment-Date: Thu, 04 Jun 2026 23:16:56 +0000 Gerrit-HasComments: Yes
