gemini-code-assist[bot] commented on code in PR #317:
URL: https://github.com/apache/tvm-ffi/pull/317#discussion_r2594083167
##########
tests/cpp/extra/test_c_env_api.cc:
##########
@@ -89,4 +96,285 @@ TEST(CEnvAPI, TVMFFIEnvTensorAllocError) {
tvm::ffi::Error);
TVMFFIEnvSetDLPackManagedTensorAllocator(old_allocator, 0, nullptr);
}
+
+// Helper functions for TVMFFIHandleInitDeinitOnce test
+static int InitSuccess(void** handle_addr) {
+ *handle_addr = new int(42);
+ return 0;
+}
+
+static int InitShouldNotBeCalled(void** handle_addr) {
+ *handle_addr = new int(999);
+ return 0;
+}
+
+static int DeinitSuccess(void* h) {
+ delete (int*)h;
+ return 0;
+}
+
+static int DeinitShouldNotBeCalled(void* h) {
+ // Should not be called when handle is already null
+ return -1;
+}
+
+static int InitWithError(void** handle_addr) {
+ TVMFFIErrorSetRaisedFromCStr("RuntimeError", "Initialization failed");
+ return -1;
+}
+
+static int InitReturnsNull(void** handle_addr) {
+ *handle_addr = nullptr; // Invalid: must return non-null handle
+ return 0;
+}
+
+static int InitForDeinitError(void** handle_addr) {
+ *handle_addr = new int(100);
+ return 0;
+}
+
+static int DeinitWithError(void* h) {
+ delete (int*)h;
+ TVMFFIErrorSetRaisedFromCStr("RuntimeError", "Deinitialization failed");
+ return -1;
+}
+
+static int InitValue123(void** handle_addr) {
+ *handle_addr = new int(123);
+ return 0;
+}
+
+static int InitValue456(void** handle_addr) {
+ *handle_addr = new int(456);
+ return 0;
+}
+
+TEST(CEnvAPI, TVMFFIHandleInitDeinitOnce) {
+ // Test 1: Successful initialization
+ void* handle = nullptr;
+ int ret = TVMFFIHandleInitOnce(&handle, InitSuccess);
+ EXPECT_EQ(ret, 0);
+ EXPECT_NE(handle, nullptr);
+ EXPECT_EQ(*(int*)handle, 42);
+
+ // Test 2: Multiple init calls should not re-initialize (idempotent)
+ void* original_handle = handle;
+ ret = TVMFFIHandleInitOnce(&handle, InitShouldNotBeCalled);
+ EXPECT_EQ(ret, 0);
+ EXPECT_EQ(handle, original_handle); // Handle should remain unchanged
+ EXPECT_EQ(*(int*)handle, 42); // Value should still be 42
+
+ // Test 3: Successful deinitialization
+ ret = TVMFFIHandleDeinitOnce(&handle, DeinitSuccess);
+ EXPECT_EQ(ret, 0);
+ EXPECT_EQ(handle, nullptr);
+
+ // Test 4: Multiple deinit calls should be safe (idempotent)
+ ret = TVMFFIHandleDeinitOnce(&handle, DeinitShouldNotBeCalled);
+ EXPECT_EQ(ret, 0);
+ EXPECT_EQ(handle, nullptr);
+
+ // Test 5: Init error - init_func returns error code
+ void* handle2 = nullptr;
+ ret = TVMFFIHandleInitOnce(&handle2, InitWithError);
+ EXPECT_NE(ret, 0);
+ EXPECT_EQ(handle2, nullptr);
+
+ // Test 6: Init error - init_func returns nullptr (invalid)
+ void* handle3 = nullptr;
+ ret = TVMFFIHandleInitOnce(&handle3, InitReturnsNull);
+ EXPECT_NE(ret, 0);
+ EXPECT_EQ(handle3, nullptr);
+
+ // Test 7: Deinit error - deinit_func returns error
+ void* handle4 = nullptr;
+ ret = TVMFFIHandleInitOnce(&handle4, InitForDeinitError);
+ EXPECT_EQ(ret, 0);
+ EXPECT_NE(handle4, nullptr);
+
+ ret = TVMFFIHandleDeinitOnce(&handle4, DeinitWithError);
+ EXPECT_NE(ret, 0);
+ EXPECT_EQ(handle4, nullptr); // Handle should still be set to nullptr
+
+ // Test 8: Init-deinit lifecycle
+ void* handle5 = nullptr;
+ ret = TVMFFIHandleInitOnce(&handle5, InitValue123);
+ EXPECT_EQ(ret, 0);
+ EXPECT_NE(handle5, nullptr);
+ EXPECT_EQ(*(int*)handle5, 123);
+
+ ret = TVMFFIHandleDeinitOnce(&handle5, DeinitSuccess);
+ EXPECT_EQ(ret, 0);
+ EXPECT_EQ(handle5, nullptr);
+
+ // Test 9: Ensure subsequent init after deinit works
+ ret = TVMFFIHandleInitOnce(&handle5, InitValue456);
+ EXPECT_EQ(ret, 0);
+ EXPECT_NE(handle5, nullptr);
+ EXPECT_EQ(*(int*)handle5, 456);
+
+ // Clean up
+ ret = TVMFFIHandleDeinitOnce(&handle5, DeinitSuccess);
+ EXPECT_EQ(ret, 0);
+}
+
+// Helper functions and data for multithreaded test
+struct ThreadSafeCounter {
+ int value;
+ std::atomic<int>* init_count_ptr;
+ std::atomic<int>* deinit_count_ptr;
+
+ ThreadSafeCounter(int v, std::atomic<int>* init_ptr, std::atomic<int>*
deinit_ptr)
+ : value(v), init_count_ptr(init_ptr), deinit_count_ptr(deinit_ptr) {}
+};
+
+// Global pointers for the current test counters
+static std::atomic<int>* g_init_count = nullptr;
+static std::atomic<int>* g_deinit_count = nullptr;
Review Comment:
The use of `static` global pointers `g_init_count` and `g_deinit_count` to
share state between the test and the helper functions is fragile. If a test
fails with an exception before these pointers are reset to `nullptr`, they will
become dangling pointers. This could cause subsequent tests to fail in
non-obvious ways if they are run in the same process.
A more robust approach is to use a test fixture (`TEST_F`) to manage this
state. The fixture's `SetUp` and `TearDown` methods can safely manage the
lifecycle of the test context, ensuring that state is properly initialized and
cleaned up for each test, even in the presence of failures.
##########
src/ffi/init_once.cc:
##########
@@ -0,0 +1,94 @@
+
+
+/*
+ * 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.
+ */
+/*
+ * \file src/ffi/init_once.cc
+ * \brief Handle Init Once C API implementation.
+ */
+#include <tvm/ffi/base_details.h>
+#include <tvm/ffi/c_api.h>
+
+#include <mutex>
+
+#ifdef _MSC_VER
+#include <windows.h>
+#endif
+
+namespace {
+
+TVM_FFI_INLINE void* AtomicLoadHandleAcquire(void** src_addr) {
+#ifdef _MSC_VER
+#if defined(_WIN32_WINNT) && (_WIN32_WINNT >= 0x0602)
+ return InterlockedCompareExchangePointerAcquire(reinterpret_cast<PVOID
volatile*>(src_addr), //
+ nullptr, nullptr);
+#else
+ return InterlockedCompareExchangePointer(reinterpret_cast<PVOID
volatile*>(src_addr), //
+ nullptr, nullptr);
+#endif
+#else
+ return __atomic_load_n(src_addr, __ATOMIC_ACQUIRE);
+#endif
+}
+
+TVM_FFI_INLINE void AtomicStoreHandleRelease(void** dst_addr, void* src) {
+#ifdef _MSC_VER
+ _InterlockedExchangePointer(reinterpret_cast<PVOID volatile*>(dst_addr),
src);
+#else
+ __atomic_store_n(dst_addr, src, __ATOMIC_RELEASE);
+#endif
+}
+} // namespace
+
+int TVMFFIHandleInitOnce(void** handle_addr, int (*init_func)(void** result)) {
+ // fast path: handle is already initialized
+ // we still need atomic load acquire to ensure the handle is not initialized
+ if (AtomicLoadHandleAcquire(handle_addr) != nullptr) return 0;
+ // slow path: handle is not initialized, call initialization function
+ // note: slow path is not meant to be called frequently, so we use a simple
mutex
+ static std::mutex mutex;
+ std::scoped_lock<std::mutex> lock(mutex);
Review Comment:
The use of a single `static std::mutex` creates a global lock for all calls
to `TVMFFIHandleInitOnce`. This will serialize all concurrent initializations
of different handles, which could become a performance bottleneck in highly
concurrent scenarios. While the double-checked locking pattern is correctly
implemented, this global contention point is a significant limitation.
Please document this behavior in the function's docstring in
`include/tvm/ffi/c_api.h` so that users are aware of this potential performance
issue.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
