You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2022/11/18 14:27:31 UTC
[GitHub] [nifi-minifi-cpp] martinzink opened a new pull request, #1456: MINIFICPP-1965 Add CMAKE flags to select malloc implementation
martinzink opened a new pull request, #1456:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1456
Thank you for submitting a contribution to Apache NiFi - MiNiFi C++.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [x] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [x] Does your PR title start with MINIFICPP-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
- [x] Has your PR been rebased against the latest commit within the target branch (typically main)?
- [x] Is your initial contribution a single, squashed commit?
### For code changes:
- [x] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
- [x] If applicable, have you updated the LICENSE file?
- [x] If applicable, have you updated the NOTICE file?
### For documentation related changes:
- [x] Have you ensured that format looks appropriate for the output in which it is rendered?
### Note:
Please ensure that once the PR is submitted, you check GitHub Actions CI results for build issues and submit an update to your PR as soon as possible.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] martinzink commented on a diff in pull request #1456: MINIFICPP-1965 Add CMAKE flags to select malloc implementation
Posted by GitBox <gi...@apache.org>.
martinzink commented on code in PR #1456:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1456#discussion_r1066960755
##########
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")
Review Comment:
good catch, it does look like its an alias, but I've changed it to FATAL_ERROR to be sure https://github.com/apache/nifi-minifi-cpp/pull/1456/commits/d08d3c78709c3a1042429de7ac18591909f6e4d2#diff-1e7de1ae2d059d21e1dd75d5812d5a34b0222cef273b7c3a2af62eb747f9d20aR211
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] lordgamez commented on a diff in pull request #1456: MINIFICPP-1965 Add CMAKE flags to select malloc implementation
Posted by GitBox <gi...@apache.org>.
lordgamez commented on code in PR #1456:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1456#discussion_r1033754099
##########
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):
Review Comment:
:+1: I was not brave enough yet to introduce type hints
##########
docker/test/integration/minifi/core/DockerTestCluster.py:
##########
@@ -318,3 +318,25 @@ def wait_for_metric_class_on_prometheus(self, metric_class, timeout_seconds):
def wait_for_processor_metric_on_prometheus(self, metric_class, timeout_seconds, processor_name):
return PrometheusChecker().wait_for_processor_metric_on_prometheus(metric_class, timeout_seconds, processor_name)
+
+ def wait_for_peak_memory_usage(self, minimum_peak_memory_usage: int, timeout_seconds: int) -> bool:
+ start_time = time.perf_counter()
+ current_peak_memory_usage = get_peak_memory_usage(get_minifi_pid())
+ while (time.perf_counter() - start_time) < timeout_seconds:
+ if current_peak_memory_usage > minimum_peak_memory_usage:
Review Comment:
Shouldn't we check if `current_peak_memory_usage` is None as the function returns Optional? Same for `wait_for_maximum_memory_usage`
##########
docker/test/integration/minifi/core/DockerTestCluster.py:
##########
@@ -318,3 +318,25 @@ def wait_for_metric_class_on_prometheus(self, metric_class, timeout_seconds):
def wait_for_processor_metric_on_prometheus(self, metric_class, timeout_seconds, processor_name):
return PrometheusChecker().wait_for_processor_metric_on_prometheus(metric_class, timeout_seconds, processor_name)
+
+ def wait_for_peak_memory_usage(self, minimum_peak_memory_usage: int, timeout_seconds: int) -> bool:
+ start_time = time.perf_counter()
+ current_peak_memory_usage = get_peak_memory_usage(get_minifi_pid())
+ while (time.perf_counter() - start_time) < timeout_seconds:
+ if current_peak_memory_usage > minimum_peak_memory_usage:
+ return True
+ current_peak_memory_usage = get_peak_memory_usage(get_minifi_pid())
+ time.sleep(1)
+ print(f"Peak memory usage ({current_peak_memory_usage}) didnt exceed minimum asserted peak memory usage {minimum_peak_memory_usage}")
Review Comment:
I think we shold use the `logging` library instead, same for `wait_for_maximum_memory_usage`
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] martinzink commented on a diff in pull request #1456: MINIFICPP-1965 Add CMAKE flags to select malloc implementation
Posted by GitBox <gi...@apache.org>.
martinzink commented on code in PR #1456:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1456#discussion_r1066962441
##########
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:
:+1: https://github.com/apache/nifi-minifi-cpp/pull/1456/commits/d08d3c78709c3a1042429de7ac18591909f6e4d2#diff-99683856935c1528793ea042b7f6ef4df3b5671a01b981f2c09dc928653cfea4R31
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] martinzink commented on a diff in pull request #1456: MINIFICPP-1965 Add CMAKE flags to select malloc implementation
Posted by GitBox <gi...@apache.org>.
martinzink commented on code in PR #1456:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1456#discussion_r1066962258
##########
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:
good idea, I've changed it in https://github.com/apache/nifi-minifi-cpp/pull/1456/commits/d08d3c78709c3a1042429de7ac18591909f6e4d2#diff-39f29894b0102815d5fbc5a5c2d97cd04845e983c208dd90ad413c3c7e59b5b0R126-R127
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] martinzink commented on a diff in pull request #1456: MINIFICPP-1965 Add CMAKE flags to select malloc implementation
Posted by GitBox <gi...@apache.org>.
martinzink commented on code in PR #1456:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1456#discussion_r1066964528
##########
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:
good catch, I've reworked this part and renamed them as well to be more readable in https://github.com/apache/nifi-minifi-cpp/pull/1456/commits/d08d3c78709c3a1042429de7ac18591909f6e4d2#diff-4487753f519234543da32b7926ab6cfc45c79ae2467eca71c8f6c269c7195eefR322
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] martinzink commented on a diff in pull request #1456: MINIFICPP-1965 Add CMAKE flags to select malloc implementation
Posted by GitBox <gi...@apache.org>.
martinzink commented on code in PR #1456:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1456#discussion_r1066964369
##########
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:
good catch, I've reworked this part and renamed them as well to be more readable in https://github.com/apache/nifi-minifi-cpp/pull/1456/commits/d08d3c78709c3a1042429de7ac18591909f6e4d2#diff-4487753f519234543da32b7926ab6cfc45c79ae2467eca71c8f6c269c7195eefR322
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a diff in pull request #1456: MINIFICPP-1965 Add CMAKE flags to select malloc implementation
Posted by GitBox <gi...@apache.org>.
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] martinzink commented on a diff in pull request #1456: MINIFICPP-1965 Add CMAKE flags to select malloc implementation
Posted by GitBox <gi...@apache.org>.
martinzink commented on code in PR #1456:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1456#discussion_r1066963288
##########
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:
:+1: https://github.com/apache/nifi-minifi-cpp/pull/1456/commits/d08d3c78709c3a1042429de7ac18591909f6e4d2#diff-b8c99bced444e4532cfed3253588140a9cb21b8f94d4fdd0e4cbbdea8a855789R56
##########
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:
:+1: https://github.com/apache/nifi-minifi-cpp/pull/1456/commits/d08d3c78709c3a1042429de7ac18591909f6e4d2#diff-b8c99bced444e4532cfed3253588140a9cb21b8f94d4fdd0e4cbbdea8a855789R56
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a diff in pull request #1456: MINIFICPP-1965 Add CMAKE flags to select malloc implementation
Posted by GitBox <gi...@apache.org>.
szaszm commented on code in PR #1456:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1456#discussion_r1071415382
##########
docker/test/integration/MiNiFi_integration_test_driver.py:
##########
@@ -311,3 +311,15 @@ 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_to_exceed(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_memory_usage_to_drop_below(maximum_memory_usage, timeout_seconds)
Review Comment:
I think the rename would make sense here as well
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] fgerlits closed pull request #1456: MINIFICPP-1965 Add CMAKE flags to select malloc implementation
Posted by "fgerlits (via GitHub)" <gi...@apache.org>.
fgerlits closed pull request #1456: MINIFICPP-1965 Add CMAKE flags to select malloc implementation
URL: https://github.com/apache/nifi-minifi-cpp/pull/1456
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] martinzink commented on a diff in pull request #1456: MINIFICPP-1965 Add CMAKE flags to select malloc implementation
Posted by GitBox <gi...@apache.org>.
martinzink commented on code in PR #1456:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1456#discussion_r1066963448
##########
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:
:+1: https://github.com/apache/nifi-minifi-cpp/pull/1456/commits/d08d3c78709c3a1042429de7ac18591909f6e4d2#diff-b8c99bced444e4532cfed3253588140a9cb21b8f94d4fdd0e4cbbdea8a855789R56
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] martinzink commented on a diff in pull request #1456: MINIFICPP-1965 Add CMAKE flags to select malloc implementation
Posted by "martinzink (via GitHub)" <gi...@apache.org>.
martinzink commented on code in PR #1456:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1456#discussion_r1084251665
##########
docker/test/integration/MiNiFi_integration_test_driver.py:
##########
@@ -311,3 +311,15 @@ 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_to_exceed(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_memory_usage_to_drop_below(maximum_memory_usage, timeout_seconds)
Review Comment:
:+1: i've renamed them aswell https://github.com/apache/nifi-minifi-cpp/pull/1456/commits/801852f548f9965a967dcfa8db6de96b536e32a1
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] martinzink commented on a diff in pull request #1456: MINIFICPP-1965 Add CMAKE flags to select malloc implementation
Posted by GitBox <gi...@apache.org>.
martinzink commented on code in PR #1456:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1456#discussion_r1066965793
##########
docker/test/integration/minifi/core/DockerTestCluster.py:
##########
@@ -318,3 +318,31 @@ def wait_for_metric_class_on_prometheus(self, metric_class, timeout_seconds):
def wait_for_processor_metric_on_prometheus(self, metric_class, timeout_seconds, processor_name):
return PrometheusChecker().wait_for_processor_metric_on_prometheus(metric_class, timeout_seconds, processor_name)
+
+ def wait_for_peak_memory_usage(self, minimum_peak_memory_usage: int, timeout_seconds: int) -> bool:
+ start_time = time.perf_counter()
+ current_peak_memory_usage = get_peak_memory_usage(get_minifi_pid())
+ if current_peak_memory_usage is None:
+ logging.warning("Failed to determine peak memory usage")
+ return False
+ while (time.perf_counter() - start_time) < timeout_seconds:
+ if current_peak_memory_usage > minimum_peak_memory_usage:
+ return True
+ current_peak_memory_usage = get_peak_memory_usage(get_minifi_pid())
+ time.sleep(1)
Review Comment:
good idea, I've changed it in https://github.com/apache/nifi-minifi-cpp/pull/1456/commits/d08d3c78709c3a1042429de7ac18591909f6e4d2#diff-4487753f519234543da32b7926ab6cfc45c79ae2467eca71c8f6c269c7195eefR325-R328
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] fgerlits commented on a diff in pull request #1456: MINIFICPP-1965 Add CMAKE flags to select malloc implementation
Posted by GitBox <gi...@apache.org>.
fgerlits commented on code in PR #1456:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1456#discussion_r1054532014
##########
docker/test/integration/minifi/core/DockerTestCluster.py:
##########
@@ -318,3 +318,31 @@ def wait_for_metric_class_on_prometheus(self, metric_class, timeout_seconds):
def wait_for_processor_metric_on_prometheus(self, metric_class, timeout_seconds, processor_name):
return PrometheusChecker().wait_for_processor_metric_on_prometheus(metric_class, timeout_seconds, processor_name)
+
+ def wait_for_peak_memory_usage(self, minimum_peak_memory_usage: int, timeout_seconds: int) -> bool:
+ start_time = time.perf_counter()
+ current_peak_memory_usage = get_peak_memory_usage(get_minifi_pid())
+ if current_peak_memory_usage is None:
+ logging.warning("Failed to determine peak memory usage")
+ return False
+ while (time.perf_counter() - start_time) < timeout_seconds:
+ if current_peak_memory_usage > minimum_peak_memory_usage:
+ return True
+ current_peak_memory_usage = get_peak_memory_usage(get_minifi_pid())
+ time.sleep(1)
Review Comment:
why do we check `current_peak_memory_usage` twice? would this work?
```suggestion
while (time.perf_counter() - start_time) < timeout_seconds:
current_peak_memory_usage = get_peak_memory_usage(get_minifi_pid())
if current_peak_memory_usage is None:
logging.warning("Failed to determine peak memory usage")
return False
if current_peak_memory_usage > minimum_peak_memory_usage:
return True
time.sleep(1)
```
(similarly in the other function below)
##########
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")
Review Comment:
The docs I have found have only `FATAL_ERROR` and `SEND_ERROR` but no plain `ERROR`: https://cmake.org/cmake/help/latest/command/message.html Is `ERROR` an alias to `FATAL_ERROR`?
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] martinzink commented on a diff in pull request #1456: MINIFICPP-1965 Add CMAKE flags to select malloc implementation
Posted by GitBox <gi...@apache.org>.
martinzink commented on code in PR #1456:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1456#discussion_r1066961080
##########
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:
Good idea, I've changed it in https://github.com/apache/nifi-minifi-cpp/pull/1456/commits/d08d3c78709c3a1042429de7ac18591909f6e4d2#diff-1e7de1ae2d059d21e1dd75d5812d5a34b0222cef273b7c3a2af62eb747f9d20aR214
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] martinzink commented on a diff in pull request #1456: MINIFICPP-1965 Add CMAKE flags to select malloc implementation
Posted by GitBox <gi...@apache.org>.
martinzink commented on code in PR #1456:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1456#discussion_r1066965793
##########
docker/test/integration/minifi/core/DockerTestCluster.py:
##########
@@ -318,3 +318,31 @@ def wait_for_metric_class_on_prometheus(self, metric_class, timeout_seconds):
def wait_for_processor_metric_on_prometheus(self, metric_class, timeout_seconds, processor_name):
return PrometheusChecker().wait_for_processor_metric_on_prometheus(metric_class, timeout_seconds, processor_name)
+
+ def wait_for_peak_memory_usage(self, minimum_peak_memory_usage: int, timeout_seconds: int) -> bool:
+ start_time = time.perf_counter()
+ current_peak_memory_usage = get_peak_memory_usage(get_minifi_pid())
+ if current_peak_memory_usage is None:
+ logging.warning("Failed to determine peak memory usage")
+ return False
+ while (time.perf_counter() - start_time) < timeout_seconds:
+ if current_peak_memory_usage > minimum_peak_memory_usage:
+ return True
+ current_peak_memory_usage = get_peak_memory_usage(get_minifi_pid())
+ time.sleep(1)
Review Comment:
good idea, I've changed it in https://github.com/apache/nifi-minifi-cpp/pull/1456/commits/d08d3c78709c3a1042429de7ac18591909f6e4d2#diff-4487753f519234543da32b7926ab6cfc45c79ae2467eca71c8f6c269c7195eefR325-R328
##########
docker/test/integration/minifi/core/DockerTestCluster.py:
##########
@@ -318,3 +318,31 @@ def wait_for_metric_class_on_prometheus(self, metric_class, timeout_seconds):
def wait_for_processor_metric_on_prometheus(self, metric_class, timeout_seconds, processor_name):
return PrometheusChecker().wait_for_processor_metric_on_prometheus(metric_class, timeout_seconds, processor_name)
+
+ def wait_for_peak_memory_usage(self, minimum_peak_memory_usage: int, timeout_seconds: int) -> bool:
+ start_time = time.perf_counter()
+ current_peak_memory_usage = get_peak_memory_usage(get_minifi_pid())
+ if current_peak_memory_usage is None:
+ logging.warning("Failed to determine peak memory usage")
+ return False
+ while (time.perf_counter() - start_time) < timeout_seconds:
+ if current_peak_memory_usage > minimum_peak_memory_usage:
+ return True
+ current_peak_memory_usage = get_peak_memory_usage(get_minifi_pid())
+ time.sleep(1)
Review Comment:
good idea, I've changed it in https://github.com/apache/nifi-minifi-cpp/pull/1456/commits/d08d3c78709c3a1042429de7ac18591909f6e4d2#diff-4487753f519234543da32b7926ab6cfc45c79ae2467eca71c8f6c269c7195eefR325-R328
##########
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:
good catch, I've reworked this part and renamed them as well to be more readable in https://github.com/apache/nifi-minifi-cpp/pull/1456/commits/d08d3c78709c3a1042429de7ac18591909f6e4d2#diff-4487753f519234543da32b7926ab6cfc45c79ae2467eca71c8f6c269c7195eefR322
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] martinzink commented on a diff in pull request #1456: MINIFICPP-1965 Add CMAKE flags to select malloc implementation
Posted by GitBox <gi...@apache.org>.
martinzink commented on code in PR #1456:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1456#discussion_r1066966901
##########
docker/test/integration/minifi/core/DockerTestCluster.py:
##########
@@ -318,3 +318,31 @@ def wait_for_metric_class_on_prometheus(self, metric_class, timeout_seconds):
def wait_for_processor_metric_on_prometheus(self, metric_class, timeout_seconds, processor_name):
return PrometheusChecker().wait_for_processor_metric_on_prometheus(metric_class, timeout_seconds, processor_name)
+
+ def wait_for_peak_memory_usage(self, minimum_peak_memory_usage: int, timeout_seconds: int) -> bool:
+ start_time = time.perf_counter()
+ current_peak_memory_usage = get_peak_memory_usage(get_minifi_pid())
+ if current_peak_memory_usage is None:
+ logging.warning("Failed to determine peak memory usage")
+ return False
+ while (time.perf_counter() - start_time) < timeout_seconds:
+ if current_peak_memory_usage > minimum_peak_memory_usage:
+ return True
+ current_peak_memory_usage = get_peak_memory_usage(get_minifi_pid())
+ time.sleep(1)
Review Comment:
good idea, I've changed it in https://github.com/apache/nifi-minifi-cpp/pull/1456/commits/d08d3c78709c3a1042429de7ac18591909f6e4d2#diff-4487753f519234543da32b7926ab6cfc45c79ae2467eca71c8f6c269c7195eefR325-R328
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] martinzink commented on a diff in pull request #1456: MINIFICPP-1965 Add CMAKE flags to select malloc implementation
Posted by GitBox <gi...@apache.org>.
martinzink commented on code in PR #1456:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1456#discussion_r1036848859
##########
docker/test/integration/minifi/core/DockerTestCluster.py:
##########
@@ -318,3 +318,25 @@ def wait_for_metric_class_on_prometheus(self, metric_class, timeout_seconds):
def wait_for_processor_metric_on_prometheus(self, metric_class, timeout_seconds, processor_name):
return PrometheusChecker().wait_for_processor_metric_on_prometheus(metric_class, timeout_seconds, processor_name)
+
+ def wait_for_peak_memory_usage(self, minimum_peak_memory_usage: int, timeout_seconds: int) -> bool:
+ start_time = time.perf_counter()
+ current_peak_memory_usage = get_peak_memory_usage(get_minifi_pid())
+ while (time.perf_counter() - start_time) < timeout_seconds:
+ if current_peak_memory_usage > minimum_peak_memory_usage:
+ return True
+ current_peak_memory_usage = get_peak_memory_usage(get_minifi_pid())
+ time.sleep(1)
+ print(f"Peak memory usage ({current_peak_memory_usage}) didnt exceed minimum asserted peak memory usage {minimum_peak_memory_usage}")
Review Comment:
Good idea, I've changed it in https://github.com/apache/nifi-minifi-cpp/commit/c7b837f8f6bd5150c2e1de5c0ecf210df9a98225#diff-4487753f519234543da32b7926ab6cfc45c79ae2467eca71c8f6c269c7195eefR333
##########
docker/test/integration/minifi/core/DockerTestCluster.py:
##########
@@ -318,3 +318,25 @@ def wait_for_metric_class_on_prometheus(self, metric_class, timeout_seconds):
def wait_for_processor_metric_on_prometheus(self, metric_class, timeout_seconds, processor_name):
return PrometheusChecker().wait_for_processor_metric_on_prometheus(metric_class, timeout_seconds, processor_name)
+
+ def wait_for_peak_memory_usage(self, minimum_peak_memory_usage: int, timeout_seconds: int) -> bool:
+ start_time = time.perf_counter()
+ current_peak_memory_usage = get_peak_memory_usage(get_minifi_pid())
+ while (time.perf_counter() - start_time) < timeout_seconds:
+ if current_peak_memory_usage > minimum_peak_memory_usage:
Review Comment:
Good idea, changed it in https://github.com/apache/nifi-minifi-cpp/commit/c7b837f8f6bd5150c2e1de5c0ecf210df9a98225#diff-4487753f519234543da32b7926ab6cfc45c79ae2467eca71c8f6c269c7195eefR325-R327
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org