Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13188 )
Change subject: replace nvml with memkind in block cache ...................................................................... Patch Set 1: (9 comments) http://gerrit.cloudera.org:8080/#/c/13188/1/CMakeLists.txt File CMakeLists.txt: http://gerrit.cloudera.org:8080/#/c/13188/1/CMakeLists.txt@162 PS1, Line 162: add_definitions(-DJE_PREFIX=jemk_) Why is this necessary? Please add a comment. http://gerrit.cloudera.org:8080/#/c/13188/1/CMakeLists.txt@452 PS1, Line 452: # disable ld.gold due some compilation issue for numa Given that NOT NUMA_LIB_PATH on L1167 yields a fatal error, this effectively disables gold universally. That's not acceptable; gold provides a significant link time speedup for Kudu's large C++ binaries. Can you please get to the bottom of this issue and fix it? http://gerrit.cloudera.org:8080/#/c/13188/1/CMakeLists.txt@1166 PS1, Line 1166: find_library(NUMA_LIB_PATH numa) Like Todd asked, why is libnuma required in the system vs. added as a thirdparty dependency? How did you reach that decision? http://gerrit.cloudera.org:8080/#/c/13188/1/CMakeLists.txt@1171 PS1, Line 1171: SHARED_LIB "${NUMA_LIB_PATH}") With only a shared library, does this mean the exported C++ client library (i.e. lib/exported/libkudu_client.so) now depends on libnuma.so in the target system? That's probably not OK, unless it's a dependency that we expect to find universally. http://gerrit.cloudera.org:8080/#/c/13188/1/cmake_modules/FindMemkind.cmake File cmake_modules/FindMemkind.cmake: http://gerrit.cloudera.org:8080/#/c/13188/1/cmake_modules/FindMemkind.cmake@21 PS1, Line 21: # XXX_STATIC_LIBS, path to *.a : # XXX_SHARED_LIBS, path to *.so shared library Nit: replace the XXX with MEMKIND. Also, the generated variables end with _LIB, not _LIBS. http://gerrit.cloudera.org:8080/#/c/13188/1/src/kudu/util/nvm_cache.cc File src/kudu/util/nvm_cache.cc: http://gerrit.cloudera.org:8080/#/c/13188/1/src/kudu/util/nvm_cache.cc@a229 PS1, Line 229: This comment is still correct, except that it wraps memkind_malloc now. http://gerrit.cloudera.org:8080/#/c/13188/1/thirdparty/build-definitions.sh File thirdparty/build-definitions.sh: http://gerrit.cloudera.org:8080/#/c/13188/1/thirdparty/build-definitions.sh@753 PS1, Line 753: mkdir -p $PREFIX/include/jemalloc : mkdir -p $PREFIX/include/memkind/internal Not necessary if you use rsync in the installation steps, which may be preferable as it'll mean that installation does the right thing vis a vis old files if the list of files shrinks. http://gerrit.cloudera.org:8080/#/c/13188/1/thirdparty/build-definitions.sh@757 PS1, Line 757: # It doesn't appear possible to isolate source and build directories, so just : # prepopulate the latter using the former. Is this still true for memkind? http://gerrit.cloudera.org:8080/#/c/13188/1/thirdparty/build-definitions.sh@763 PS1, Line 763: # manually install the built artifacts. The memkind build script doesn't offer a "make install" equivalent? -- To view, visit http://gerrit.cloudera.org:8080/13188 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I08855c62bc42b9e6f7a4b3bfa27a708ed2c133f5 Gerrit-Change-Number: 13188 Gerrit-PatchSet: 1 Gerrit-Owner: ye yuqiang <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Tue, 30 Apr 2019 22:48:03 +0000 Gerrit-HasComments: Yes
