Joe McDonnell 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: (9 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(..., ...)? Got it working. I needed to add an operator<< function for MallocUtil::HugePageSupport. 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? I wanted to keep the behavior the same for all the daemons for this refactor, and different daemons use different settings right now. Impalad wants to set a large total thread cache and turn on aggressive decommit. Catalogd/statestored/admissiond use the defaults. I think we'll continue to have different defaults for the different daemons in the future, so the rule right now is that the different daemons set up the flags however they want and MallocUtil is responsible for implementing them. 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 Good point, will fix http://gerrit.cloudera.org:8080/#/c/24401/2/be/src/runtime/test-env.cc File be/src/runtime/test-env.cc: http://gerrit.cloudera.org:8080/#/c/24401/2/be/src/runtime/test-env.cc@75 PS2, Line 75: // Always use aggressive decommit for backend tests : FLAGS_tcmalloc_aggressive_memory_decommit = true; : RETURN_IF_ERROR(MallocUtil::GetInstance()->Init()); This needs to move earlier, before RegisterMemoryMetrics() above. http://gerrit.cloudera.org:8080/#/c/24401/2/be/src/util/malloc-util-gperftools.h File be/src/util/malloc-util-gperftools.h: http://gerrit.cloudera.org:8080/#/c/24401/2/be/src/util/malloc-util-gperftools.h@110 PS2, Line 110: int64_t retval; > The old code used zero init. Done 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? I changed this to initialize it during GetInstance(), so there is no need for this function. 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" Done http://gerrit.cloudera.org:8080/#/c/24401/2/be/src/util/malloc-util.cc File be/src/util/malloc-util.cc: http://gerrit.cloudera.org:8080/#/c/24401/2/be/src/util/malloc-util.cc@48 PS2, Line 48: MallocUtil* MallocUtil::GetInstance() { > Is it possible to eliminate Allocate by inlining it into GetInstance? I changed this to initialize it during GetInstance() and avoid the Allocate() call. http://gerrit.cloudera.org:8080/#/c/24401/2/be/src/util/pprof-path-handlers.cc File be/src/util/pprof-path-handlers.cc: http://gerrit.cloudera.org:8080/#/c/24401/2/be/src/util/pprof-path-handlers.cc@75 PS2, Line 75: delete profile; > nit: free? Good point, gpertools uses malloc(), so this should use free(). -- 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-Reviewer: Peter Rozsa <[email protected]> Gerrit-Comment-Date: Tue, 09 Jun 2026 06:50:25 +0000 Gerrit-HasComments: Yes
