Skip to content

Commit 890fad4

Browse files
committed
perf(common): gate IOHook shared_mutex behind armed flag
Try() is invoked by CHECK_HOOK on every local-file IO, so taking a std::shared_mutex shared lock per call regresses the previously lock-free IO path. Writers (Reset()/Clear()) only exist in tests and the production default is the disabled state, so gate the synchronized path behind an atomic armed flag: Try() is now a single acquire load when disabled, and only takes the mutex once armed. Reset() publishes the complete configuration under the mutex and then release-stores armed; armed readers acquire the mutex before reading mode/pos, so an observed armed state always implies a complete configuration. Clear() disarms first so IO threads drop off the lock as soon as possible. One semantic change: the disabled fast path no longer increments the IO counter, so IOCount() now counts only while armed; all IOCount() consumers are tests and the header documents the contract. Add IOHookTest.TestDisabledFastPath to pin the disabled behavior, and arm the hook in TestReadAheadCache.TestPreBufferWindowLimit (#209), which relied on the previous count-while-disabled behavior.
1 parent 6e18c75 commit 890fad4

4 files changed

Lines changed: 70 additions & 24 deletions

File tree

src/paimon/common/factories/io_hook.cpp

Lines changed: 38 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -24,13 +24,50 @@
2424
#include <stdexcept>
2525

2626
#include "fmt/format.h"
27+
#include "paimon/macros.h"
2728
#include "paimon/status.h"
2829

2930
namespace paimon {
3031

3132
class IOHook::Impl {
3233
public:
3334
Status Try(const std::string& path) {
35+
// Fast path: the hook is disabled, which is always the case in production;
36+
// writers (Reset()/Clear()) only exist in tests. This keeps Try() a single
37+
// atomic load on the IO path instead of a shared_mutex acquisition per IO.
38+
if (PAIMON_UNLIKELY(armed_.load(std::memory_order_acquire))) {
39+
return TryArmed(path);
40+
}
41+
return Status::OK();
42+
}
43+
44+
inline void Reset(int64_t pos, IOHook::Mode mode) {
45+
std::unique_lock<std::shared_mutex> lock(mutex_);
46+
mode_ = mode;
47+
pos_ = pos;
48+
io_count_ = 0;
49+
// Arm only after the configuration is complete: TryArmed() reads mode_/pos_
50+
// under mutex_, which synchronizes with this store, so an observed armed state
51+
// always implies a complete configuration.
52+
armed_.store(true, std::memory_order_release);
53+
}
54+
55+
int64_t IOCount() const {
56+
std::shared_lock<std::shared_mutex> lock(mutex_);
57+
return io_count_.load();
58+
}
59+
60+
void Clear() {
61+
std::unique_lock<std::shared_mutex> lock(mutex_);
62+
// Disarm first so IO threads stop taking the lock as soon as possible.
63+
armed_.store(false, std::memory_order_release);
64+
mode_ = IOHook::Mode::SILENT;
65+
pos_ = -1;
66+
io_count_ = 0;
67+
}
68+
69+
private:
70+
Status TryArmed(const std::string& path) {
3471
std::shared_lock<std::shared_mutex> lock(mutex_);
3572
if (io_count_.fetch_add(1) < pos_) {
3673
return Status::OK();
@@ -51,24 +88,8 @@ class IOHook::Impl {
5188
}
5289
}
5390

54-
inline void Reset(int64_t pos, IOHook::Mode mode) {
55-
std::unique_lock<std::shared_mutex> lock(mutex_);
56-
mode_ = mode;
57-
pos_ = pos;
58-
io_count_ = 0;
59-
}
60-
61-
int64_t IOCount() const {
62-
std::shared_lock<std::shared_mutex> lock(mutex_);
63-
return io_count_.load();
64-
}
65-
66-
void Clear() {
67-
Reset(-1, IOHook::Mode::SILENT);
68-
}
69-
70-
private:
7191
mutable std::shared_mutex mutex_;
92+
std::atomic<bool> armed_ = {false};
7293
std::atomic<int64_t> io_count_ = {0};
7394
int64_t pos_ = -1;
7495
IOHook::Mode mode_ = IOHook::Mode::SILENT;

src/paimon/common/factories/io_hook.h

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,8 @@ class PAIMON_EXPORT IOHook : public Singleton<IOHook> {
4545
};
4646

4747
/// Reset the IO exception position and behavior mode to handle the exception.
48-
/// IOCount will be reset to 0.
48+
/// IOCount will be reset to 0. Arms the hook: Try() switches from its lock-free
49+
/// disabled fast path to the synchronized armed path.
4950
///
5051
/// @params pos The position where the IO exception occurs.
5152
/// @params mode The mode of behavior for handling the exception.
@@ -56,12 +57,14 @@ class PAIMON_EXPORT IOHook : public Singleton<IOHook> {
5657
Status Try(const std::string& path);
5758

5859
/// Get the count of IO operations that have already occurred.
60+
/// IOs are only counted while the hook is armed (after Reset(), before Clear());
61+
/// the disabled fast path does not count.
5962
///
6063
/// @return The number of IO operations executed.
6164
int64_t IOCount() const;
6265

6366
/// Clear the state of the IOHook, including resetting IO count and
64-
/// any stored exception state.
67+
/// any stored exception state. Disarms the hook back to the lock-free fast path.
6568
void Clear();
6669

6770
private:

src/paimon/common/factories/io_hook_test.cpp

Lines changed: 24 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -68,12 +68,33 @@ TEST(IOHookTest, TestThrowExceptionMode) {
6868
hook->Clear();
6969
}
7070

71+
// The disabled state is the production default: Try() must take the lock-free fast
72+
// path, always return OK, and not count IOs (see IOCount()'s contract). Clear() first
73+
// so the test does not depend on execution order.
74+
TEST(IOHookTest, TestDisabledFastPath) {
75+
auto hook = IOHook::GetInstance();
76+
hook->Clear();
77+
ASSERT_OK(hook->Try("path"));
78+
ASSERT_OK(hook->Try("path"));
79+
ASSERT_EQ(0, hook->IOCount());
80+
81+
// Re-arming and disarming must restore the exact disabled behavior.
82+
hook->Reset(0, IOHook::Mode::RETURN_ERROR);
83+
ASSERT_NOK(hook->Try("path"));
84+
ASSERT_EQ(1, hook->IOCount());
85+
hook->Clear();
86+
ASSERT_OK(hook->Try("path"));
87+
ASSERT_OK(hook->Try("path"));
88+
ASSERT_EQ(0, hook->IOCount());
89+
}
90+
7191
// Regression test for torn IOHook configurations: Reset()/Clear() run on one thread
7292
// while other threads call Try() concurrently. A shared start barrier releases all
7393
// threads together, and the reset thread keeps hammering until every worker has
74-
// finished, so overlap is structural rather than timing-dependent. Under a
75-
// ThreadSanitizer build this deterministically reports any unsynchronized access;
76-
// functionally every Try() must return OK.
94+
// finished, so overlap is structural rather than timing-dependent. The continuous
95+
// arm/disarm cycling also keeps workers switching between the disabled fast path and
96+
// the synchronized armed path. Under a ThreadSanitizer build this deterministically
97+
// reports any unsynchronized access; functionally every Try() must return OK.
7798
TEST(IOHookTest, TestConcurrentResetAndTry) {
7899
auto hook = IOHook::GetInstance();
79100

src/paimon/common/utils/read_ahead_cache_test.cpp

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -464,7 +464,8 @@ TEST(TestReadAheadCache, TestPreBufferWindowLimit) {
464464

465465
auto io_hook = paimon::IOHook::GetInstance();
466466
paimon::ScopeGuard guard([&io_hook]() { io_hook->Clear(); });
467-
io_hook->Clear();
467+
// IOCount() only counts while armed; INT64_MAX never triggers the error mode.
468+
io_hook->Reset(INT64_MAX, paimon::IOHook::Mode::RETURN_ERROR);
468469

469470
AssertReadEquals({0, 10}, "abcdefghij", &cache);
470471
// The second range did not fit into the window: only one prefetch IO.
@@ -475,7 +476,7 @@ TEST(TestReadAheadCache, TestPreBufferWindowLimit) {
475476
ASSERT_EQ(io_hook->IOCount(), 2);
476477

477478
// The range is cached now: re-reading it issues no IO at all.
478-
io_hook->Clear();
479+
io_hook->Reset(INT64_MAX, paimon::IOHook::Mode::RETURN_ERROR);
479480
AssertReadEquals({16, 10}, "qrstuvwxyz", &cache);
480481
ASSERT_EQ(io_hook->IOCount(), 0);
481482
}

0 commit comments

Comments
 (0)