Skip to content

Commit 70ff956

Browse files
sunchaoSteNicholas
authored andcommitted
[CELEBORN-2321] Avoid locking disk writers during memory split checks
### Why are the changes needed? `needHardSplitForMemoryShuffleStorage()` runs on the push path. Disk-backed writers can never require this memory-only split check, but the method currently acquires the writer lock before returning `false`. For the common disk-backed case, that adds avoidable contention with writes and evictions on a hot path. ### What changes were proposed in this PR? This PR adds an unlocked fast path for non-memory writers so they return immediately without taking the `PartitionDataWriter` monitor. For memory-backed writers, it rechecks `currentTierWriter` after entering the synchronized block before evaluating the existing hard-split conditions, which preserves the original behavior if the writer tier changes concurrently. ### Does this PR resolve a correctness bug? No. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? - Attempted `build/mvn -pl worker -am -DskipTests compile` on current `main`. - The Maven reactor fails before reaching `worker` because `celeborn-master_2.12` cannot resolve snapshot test-jar artifacts for `celeborn-common_2.12` and `celeborn-service_2.12`; that failure is unrelated to this change. Closes #3680 from sunchao/dev/chao/codex/celeborn-fast-memory-split-check-oss-main. Authored-by: Chao Sun <chao@openai.com> Signed-off-by: SteNicholas <programgeek@163.com>
1 parent 8d473c5 commit 70ff956

1 file changed

Lines changed: 15 additions & 5 deletions

File tree

worker/src/main/java/org/apache/celeborn/service/deploy/worker/storage/PartitionDataWriter.java

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -122,13 +122,23 @@ public synchronized void flush() {
122122
currentTierWriter.flush(false, false);
123123
}
124124

125-
public synchronized boolean needHardSplitForMemoryShuffleStorage() {
126-
if (!(currentTierWriter instanceof MemoryTierWriter)) {
125+
public boolean needHardSplitForMemoryShuffleStorage() {
126+
// Disk-backed writers never need this memory-only split check. Avoid contending with writes and
127+
// evictions on the hot push path for the common case.
128+
TierWriterBase tierWriter = currentTierWriter;
129+
if (!(tierWriter instanceof MemoryTierWriter)) {
127130
return false;
128131
}
129-
return !storageManager.localOrDfsStorageAvailable()
130-
&& (currentTierWriter.fileInfo().getFileLength() > memoryFileStorageMaxFileSize
131-
|| !MemoryManager.instance().memoryFileStorageAvailable());
132+
133+
synchronized (this) {
134+
tierWriter = currentTierWriter;
135+
if (!(tierWriter instanceof MemoryTierWriter)) {
136+
return false;
137+
}
138+
return !storageManager.localOrDfsStorageAvailable()
139+
&& (tierWriter.fileInfo().getFileLength() > memoryFileStorageMaxFileSize
140+
|| !MemoryManager.instance().memoryFileStorageAvailable());
141+
}
132142
}
133143

134144
public synchronized void write(ByteBuf data) throws IOException {

0 commit comments

Comments
 (0)