szaszm commented on code in PR #1456:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1456#discussion_r1061432434
##########
CMakeLists.txt:
##########
@@ -196,14 +196,26 @@ set(PASSTHROUGH_CMAKE_ARGS -DANDROID_ABI=${ANDROID_ABI}
-G${CMAKE_GENERATOR}
)
-# jemalloc
+if(CUSTOM_MALLOC)
+ if (CUSTOM_MALLOC STREQUAL jemalloc)
+ include(BundledJemalloc)
+ use_bundled_jemalloc(${CMAKE_CURRENT_SOURCE_DIR}
${CMAKE_CURRENT_BINARY_DIR})
+ set(CUSTOM_MALLOC_LIB JeMalloc::JeMalloc)
+ elseif (CUSTOM_MALLOC STREQUAL mimalloc)
+ include(MiMalloc)
+ set(CUSTOM_MALLOC_LIB mimalloc)
+ elseif (CUSTOM_MALLOC STREQUAL rpmalloc)
+ include(RpMalloc)
+ set(CUSTOM_MALLOC_LIB rpmalloc)
+ else()
+ message(ERROR "Invalid CUSTOM_MALLOC")
+ endif()
+else()
+ message("No custom malloc implementation")
Review Comment:
I'd lower the level of this message to `STATUS` or `VERBOSE`
##########
cmake/MiNiFiOptions.cmake:
##########
@@ -117,6 +123,9 @@ add_minifi_option(ENABLE_TEST_PROCESSORS "Enables test
processors" OFF)
add_minifi_option(ENABLE_PROMETHEUS "Enables Prometheus support." OFF)
add_minifi_option(DISABLE_JEMALLOC "Disables jemalloc." OFF)
+set_minifi_cache_variable(CUSTOM_MALLOC "" "Overwrite malloc implementation.")
+set_property(CACHE CUSTOM_MALLOC PROPERTY STRINGS "jemalloc" "mimalloc"
"rpmalloc")
Review Comment:
Please include a disabled option ("OFF") to be able to disable this on the
GUI. I think it would also be better to use it as the default, instead of the
empty string.
```suggestion
set_minifi_cache_variable(CUSTOM_MALLOC OFF "Overwrite malloc
implementation.")
set_property(CACHE CUSTOM_MALLOC PROPERTY STRINGS "jemalloc" "mimalloc"
"rpmalloc" OFF)
```
##########
docker/test/integration/MiNiFi_integration_test_driver.py:
##########
@@ -311,3 +311,9 @@ def check_metric_class_on_prometheus(self, metric_class,
timeout_seconds):
def check_processor_metric_on_prometheus(self, metric_class,
timeout_seconds, processor_name):
assert
self.cluster.wait_for_processor_metric_on_prometheus(metric_class,
timeout_seconds, processor_name)
+
+ def check_minimum_peak_memory_usage(self, minimum_peak_memory_usage: int,
timeout_seconds: int) -> None:
+ assert
self.cluster.wait_for_peak_memory_usage(minimum_peak_memory_usage,
timeout_seconds)
+
+ def check_maximum_memory_usage(self, maximum_memory_usage: int,
timeout_seconds: int) -> None:
+ assert self.cluster.wait_for_peak_memory_usage(maximum_memory_usage,
timeout_seconds)
Review Comment:
I believe there is a copy paste error here
```suggestion
def check_minimum_peak_memory_usage(self, minimum_peak_memory_usage:
int, timeout_seconds: int) -> None:
assert
self.cluster.wait_for_peak_memory_usage(minimum_peak_memory_usage,
timeout_seconds)
def check_maximum_memory_usage(self, maximum_memory_usage: int,
timeout_seconds: int) -> None:
assert
self.cluster.wait_for_maximum_memory_usage(maximum_memory_usage,
timeout_seconds)
```
By the way, what's the difference between the peak and the maximum memory
usage? They sound the same to me.
##########
cmake/RpMalloc.cmake:
##########
@@ -0,0 +1,32 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+#
+include(FetchContent)
+FetchContent_Declare(
+ rpmalloc
+ URL
https://github.com/mjansson/rpmalloc/archive/refs/tags/1.4.4.tar.gz
+ URL_HASH
SHA256=3859620c03e6473f0b3f16a4e965e7c049594253f70e8370fb9caa0e4118accb
+)
+FetchContent_GetProperties(rpmalloc)
+
+if(NOT rpmalloc_POPULATED)
+ FetchContent_Populate(rpmalloc)
+ add_library(rpmalloc ${rpmalloc_SOURCE_DIR}/rpmalloc/rpmalloc.c)
+ target_include_directories(rpmalloc PUBLIC ${rpmalloc_SOURCE_DIR}/rpmalloc)
+ target_compile_definitions("rpmalloc" PRIVATE ENABLE_OVERRIDE=1
ENABLE_PRELOAD=1)
Review Comment:
The quotes are unnecessary
```suggestion
target_compile_definitions(rpmalloc PRIVATE ENABLE_OVERRIDE=1
ENABLE_PRELOAD=1)
```
##########
minifi_main/CMakeLists.txt:
##########
@@ -52,8 +52,9 @@ endif(NOT USE_SHARED_LIBS)
target_link_libraries(minifiexe Threads::Threads)
target_link_libraries(minifiexe yaml-cpp)
-if(NOT WIN32 AND ENABLE_JNI AND NOT DISABLE_JEMALLOC)
- target_link_libraries(minifiexe JeMalloc::JeMalloc)
+if(CUSTOM_MALLOC_LIB)
+ message("Using custom malloc lib ${CUSTOM_MALLOC_LIB} for minifiexe")
Review Comment:
I'd lower the log level here as well, we don't need to print setting values
during normal usage.
```suggestion
message(VERBOSE "Using custom malloc lib ${CUSTOM_MALLOC_LIB} for
minifiexe")
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
