Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/21076 )
Change subject: KUDU-3557 Simple way to change maximum thread cache size of tcmalloc. ...................................................................... Patch Set 3: (8 comments) http://gerrit.cloudera.org:8080/#/c/21076/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21076/3//COMMIT_MSG@7 PS3, Line 7: Simple way to change maximum thread cache size What if somebody sets the TCMALLOC_MAX_TOTAL_THREAD_CACHE_BYTES environment variable to control the behavior of the tcmalloc library linked in? What setting will prevail: the setting via the new flag or the setting via the environment variable? Should we warn about possible conflicts in such a case? http://gerrit.cloudera.org:8080/#/c/21076/3//COMMIT_MSG@10 PS3, Line 10: Making the total thread cache size larger : could highly decrease the lock contention. Is there a way to add a small test to validate the results? We do already have a few tests in src/kudu/integration-tests/memory_gc-itest.cc, adding a couple more tests to focus on lock contention metrics for different settings of the new flag would be very useful. Thank you! http://gerrit.cloudera.org:8080/#/c/21076/3/src/kudu/server/server_base.cc File src/kudu/server/server_base.cc: http://gerrit.cloudera.org:8080/#/c/21076/3/src/kudu/server/server_base.cc@309 PS3, Line 309: 33554432 nit: replacing it with 32 * 1024 * 1024 would be more descriptive to human readers http://gerrit.cloudera.org:8080/#/c/21076/3/src/kudu/server/server_base.cc@310 PS3, Line 310: across all in tcmalloc Some words are missing? http://gerrit.cloudera.org:8080/#/c/21076/3/src/kudu/server/server_base.cc@311 PS3, Line 311: if thread cache request memory from central cache frequently. Well, those requests are always frequent even in non-stressful workloads and no quantification is provided to make an assessment what is the threshold. Maybe, instead mention that increasing this value helps in reducing lock contention in tcmalloc for memory-intensive workloads. Providing information on the upper boundary for the value could be helpful as well. http://gerrit.cloudera.org:8080/#/c/21076/3/src/kudu/server/server_base.cc@311 PS3, Line 311: larger a greater value http://gerrit.cloudera.org:8080/#/c/21076/3/src/kudu/server/server_base.cc@312 PS3, Line 312: TAG_FLAG(tcmalloc_max_total_thread_cache_bytes, advanced); Please also add the 'experimental' tag as well. http://gerrit.cloudera.org:8080/#/c/21076/3/src/kudu/server/server_base.cc@313 PS3, Line 313: Does it make sense to add a validator for the flag? It seems in the gperftools's implementation of the tcmalloc library there is 1GByte cap on the setting for this flag. -- To view, visit http://gerrit.cloudera.org:8080/21076 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3cb8c6ed6a8f24c63258ae65f8b841fe74b75308 Gerrit-Change-Number: 21076 Gerrit-PatchSet: 3 Gerrit-Owner: Song Jiacheng <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 27 Feb 2024 19:13:50 +0000 Gerrit-HasComments: Yes
