MM trigger tests by kpjensen · Pull Request #113 · dnanexus/dxfuse

kpjensen · 2026-01-14T17:53:19Z

No description provided.

Copilot

Pull request overview

This PR introduces a comprehensive memory management system for dxfuse to prevent resource starvation and improve concurrency handling. The changes add a centralized MemoryManager to coordinate memory allocation across read and write operations, along with enhanced prefetch logic and upload worker scaling based on available CPU resources.

Changes:

Implemented a MemoryManager with separate budgets for read/write operations to control memory usage
Refactored prefetch and upload systems to use the memory manager for buffer allocation
Added CPU-aware scaling for HTTP client pools and upload workers

Reviewed changes

Copilot reviewed 16 out of 17 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
memory_manager.go	New memory manager with semaphore-based budgeting for read/write operations
memory_manager_test.go	Comprehensive test coverage for memory manager allocation/release scenarios
util.go	Added CPU detection and worker count calculation utilities
util_test.go	Tests for HTTP client pool and upload worker count bounds
prefetch.go	Integrated memory manager for prefetch buffer allocation and enhanced sequential detection
prefetch_test.go	Tests for prefetch heuristics, cache behavior, and sequential access detection
upload.go	Updated uploader to use memory manager and dynamic part sizing
dxfuse.go	Initialized memory manager with system memory limits and updated component constructors
dx_ops.go	Enhanced error logging for upload failures
cli/main.go	Added `-maxMemoryUsageMiB` flag for memory limit configuration
test_parallel_rw.sh	New test script for parallel read/write operations
test/local/multiple_mounts.sh	Enhanced error handling and log display on test failure
.vscode/launch.json, .vscode/tasks.json	VSCode debug configuration for development
.github/workflows/*.yml	Updated GitHub Actions to use setup-go@v5

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-14T18:32:25Z

+			//if pgs.verboseLevel >= 2 {
+			pfm.log("isDataInCache: wait")
+			//}


Remove commented-out code. If the conditional logging is unnecessary, delete the entire block; otherwise, restore the condition.

Copilot · 2026-01-14T18:32:26Z

+	// data := pgs.allocateIOvecMemory(ioSize)
+	// if data == nil {
+	// 	pgs.log("Memory limit exceeded, dropping IOvec allocation")
+	// 	return nil
+	// }


Remove commented-out dead code related to memory allocation that is no longer in use.

Suggested change

// data := pgs.allocateIOvecMemory(ioSize)

// if data == nil {

// pgs.log("Memory limit exceeded, dropping IOvec allocation")

// return nil

// }

Copilot · 2026-01-14T18:32:26Z

 		iov := pfm.cache.iovecs[i]
 		switch iov.state {
 		case IOV_HOLE:
+			pfm.log("isDataInCache: HOLE")


These debug log statements at lines 1212, 1220, and 1226 should be guarded by verbosity level checks (use pgs.debug() or check pgs.verboseLevel) to avoid excessive logging in production.

Copilot · 2026-01-14T18:32:26Z

 	fsys.opClose(oph)

-	fsys.pgs = NewPrefetchGlobalState(options.VerboseLevel, dxEnv)
+	// Default to max 15% memory usage of system memory


The comment at line 202 says '15%' but the code calculates 10%. Update either the comment or the calculation to match.

Suggested change

// Default to max 15% memory usage of system memory

// Default to max 10% memory usage of system memory

Copilot · 2026-01-14T18:32:27Z

+umount /home/kjensen/MNT || true
+# mount dxfuse
+/home/kjensen/dxfuse --version
+/home/kjensen/dxfuse -limitedWrite /home/kjensen/MNT testing
+rm -rf /home/kjensen/MNT/testing/1GiB*
+rm -rf /home/kjensen/MNT/testing/1kib*


Hard-coded paths like '/home/kjensen/MNT' and '/home/kjensen/dxfuse' should be parameterized or documented as example values, as they are specific to a development environment.

Suggested change

umount /home/kjensen/MNT || true

# mount dxfuse

/home/kjensen/dxfuse --version

/home/kjensen/dxfuse -limitedWrite /home/kjensen/MNT testing

rm -rf /home/kjensen/MNT/testing/1GiB*

rm -rf /home/kjensen/MNT/testing/1kib*

# Allow overriding dxfuse binary and mount directory via environment

DXFUSE_BIN="${DXFUSE_BIN:-/home/kjensen/dxfuse}"

DXFUSE_MNT_DIR="${DXFUSE_MNT_DIR:-/home/kjensen/MNT}"

umount "$DXFUSE_MNT_DIR" || true

# mount dxfuse

"$DXFUSE_BIN" --version

"$DXFUSE_BIN" -limitedWrite "$DXFUSE_MNT_DIR" testing

rm -rf "$DXFUSE_MNT_DIR"/testing/1GiB*

rm -rf "$DXFUSE_MNT_DIR"/testing/1kib*

Copilot

Pull request overview

Copilot reviewed 16 out of 17 changed files in this pull request and generated 9 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 16 out of 17 changed files in this pull request and generated 6 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-20T20:02:58Z

+}
+
+// Analyzes stack traces to find goroutines waiting for mutex locks
+func FindWaitingGoroutines(stackTrace string, mutexName string) string {


The parameter mutexName is declared but never used in the function body. Either remove it if it's not needed, or implement filtering logic based on mutexName if that was the intended functionality.

Copilot · 2026-01-20T20:02:58Z

+	// data := pgs.allocateIOvecMemory(ioSize)
+	// if data == nil {
+	// 	pgs.log("Memory limit exceeded, dropping IOvec allocation")
+	// 	return nil
+	// }


Remove commented-out code. If this allocation logic is no longer needed, delete these lines. If it's intended for future use, document why it's preserved or track it in a TODO/issue.

Suggested change

// data := pgs.allocateIOvecMemory(ioSize)

// if data == nil {

// pgs.log("Memory limit exceeded, dropping IOvec allocation")

// return nil

// }

Copilot · 2026-01-20T20:02:58Z

 			fsys.opClose(oph)
 			fsys.mutex.Unlock()
-			fh.writeBuffer = fsys.uploader.AllocateWriteBuffer(partId, false)
+			fh.writeBuffer = fsys.uploader.AllocateWriteBuffer(partId, true)


The second parameter true is unclear without context. Consider using a named constant or adding a comment explaining what the boolean represents (appears to be a blocking flag based on the function signature).

Suggested change

fh.writeBuffer = fsys.uploader.AllocateWriteBuffer(partId, true)

fh.writeBuffer = fsys.uploader.AllocateWriteBuffer(partId, true /* blocking */)

Copilot · 2026-01-20T20:02:58Z

 	} else {
-		pfm.log("(#io=%d) prefetch io error [%d -- %d] %s",
-			ioReq.id, ioReq.startByte, ioReq.endByte, err.Error())
-		pfm.cache.iovecs[iovIdx].state = IOV_ERRORED
+		if errors.Is(err, errPrefetchNoMemory) {
+			// Memory pressure is transient; treat as a cache hole (best-effort prefetch).
+			pfm.cache.iovecs[iovIdx].state = IOV_HOLE
+			pgs.debug("prefetch skipped due to memory pressure: handle=%d startByte=%d endByte=%d",
+				ioReq.hid, ioReq.startByte, ioReq.endByte)
+		} else {
+			pfm.log("prefetch error: handle=%d, startByte=%d, endByte=%d, error=%s", ioReq.hid, ioReq.startByte, ioReq.endByte, err.Error())
+			// Safety: if a buffer somehow comes back with an error, release it.
+			if data != nil {
+				pgs.memoryManager.ReleaseReadBuffer(data)
+			}
+			pfm.log("(#io=%d) prefetch io error [%d -- %d] %s",
+				ioReq.id, ioReq.startByte, ioReq.endByte, err.Error())
+			pfm.cache.iovecs[iovIdx].state = IOV_ERRORED
+			pgs.log("IO request failed: handle=%d, startByte=%d, endByte=%d, error=%s", ioReq.hid, ioReq.startByte, ioReq.endByte, err.Error())
+		}


The nested else-if structure creates unnecessary indentation. Flatten this by handling the errPrefetchNoMemory case first with early returns or by restructuring the conditional logic.

Suggested change

} else {

pfm.log("(#io=%d) prefetch io error [%d -- %d] %s",

ioReq.id, ioReq.startByte, ioReq.endByte, err.Error())

pfm.cache.iovecs[iovIdx].state = IOV_ERRORED

if errors.Is(err, errPrefetchNoMemory) {

// Memory pressure is transient; treat as a cache hole (best-effort prefetch).

pfm.cache.iovecs[iovIdx].state = IOV_HOLE

pgs.debug("prefetch skipped due to memory pressure: handle=%d startByte=%d endByte=%d",

ioReq.hid, ioReq.startByte, ioReq.endByte)

} else {

pfm.log("prefetch error: handle=%d, startByte=%d, endByte=%d, error=%s", ioReq.hid, ioReq.startByte, ioReq.endByte, err.Error())

// Safety: if a buffer somehow comes back with an error, release it.

if data != nil {

pgs.memoryManager.ReleaseReadBuffer(data)

}

pfm.log("(#io=%d) prefetch io error [%d -- %d] %s",

ioReq.id, ioReq.startByte, ioReq.endByte, err.Error())

pfm.cache.iovecs[iovIdx].state = IOV_ERRORED

pgs.log("IO request failed: handle=%d, startByte=%d, endByte=%d, error=%s", ioReq.hid, ioReq.startByte, ioReq.endByte, err.Error())

}

} else if errors.Is(err, errPrefetchNoMemory) {

// Memory pressure is transient; treat as a cache hole (best-effort prefetch).

pfm.cache.iovecs[iovIdx].state = IOV_HOLE

pgs.debug("prefetch skipped due to memory pressure: handle=%d startByte=%d endByte=%d",

ioReq.hid, ioReq.startByte, ioReq.endByte)

} else {

pfm.log("prefetch error: handle=%d, startByte=%d, endByte=%d, error=%s", ioReq.hid, ioReq.startByte, ioReq.endByte, err.Error())

// Safety: if a buffer somehow comes back with an error, release it.

if data != nil {

pgs.memoryManager.ReleaseReadBuffer(data)

}

pfm.log("(#io=%d) prefetch io error [%d -- %d] %s",

ioReq.id, ioReq.startByte, ioReq.endByte, err.Error())

pfm.cache.iovecs[iovIdx].state = IOV_ERRORED

pgs.log("IO request failed: handle=%d, startByte=%d, endByte=%d, error=%s", ioReq.hid, ioReq.startByte, ioReq.endByte, err.Error())

Copilot · 2026-01-20T20:02:59Z

 func (pgs *PrefetchGlobalState) moveCacheWindow(pfm *PrefetchFileMetadata, iovIndex int) {
 	nIovecs := len(pfm.cache.iovecs)
 	nReadAheadChunks := iovIndex + pfm.cache.maxNumIovecs - nIovecs
+	// log the above 2 values


The comment 'log the above 2 values' is vague and doesn't add value. Either remove it or make it descriptive about what values are being logged and why.

Suggested change

// log the above 2 values

// When verbose logging is enabled, record the cache window parameters

// (iovIndex, nIovecs, nReadAheadChunks) to aid debugging of readahead behavior.

Copilot · 2026-01-20T20:02:59Z

    echo "unmounting dxfuse"
    cd $HOME
-    fusermount -u $mountpoint
+    fusermount -u $mountpoint0


Variable naming is inconsistent - the script defines mountpoint (line 7) but uses mountpoint0 (line 19) without defining it. This appears to be a bug introduced by the change from $mountpoint to $mountpoint0.

- Release empty replacement write buffer in FlushFile when last write lands exactly on a part boundary (budget leak) - Release leftover write buffer in ReleaseFileHandle - Handle VirtualMemory() failure with conservative 8 GiB fallback - Use writeBufferOffset instead of len(writeBuffer) for empty file check - Protect fh.writeError write with mutex in uploadWorker - Remove no-op buf=nil in MemoryManager.release() - Remove unused pprof import - Remove commented-out code in newIovec - Remove unused GetDumpableStackTraces/FindWaitingGoroutines - Remove noisy httpClient debug log in upload error path Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

kpjensen added 30 commits April 9, 2025 16:32

WIP

dfc50b9

WIP

0dc7bc4

WIP

0ea2a1b

WIP

4f1d673

Lower to 50%

9c82a02

Merge branch 'master' into mm-wip

f10645c

WIP

3af5751

WIP

7dc2bac

WIP

849e0ba

WIP

3c48881

WIP

ddda542

WIP

60bab07

Fix read allocation error on first read

45b5a25

WIP

148b65a

WIP

8305844

WIP

19686bb

Resize buffer before upload of final part

5d7105b

WIP

9034189

WIP

4e39ee1

WIP

7484172

WIP

d36ba1d

WIP

a861f52

WIP

73293b4

WIP

c05b50e

WIP

464e2cd

Debugging

412344b

WIP

3b9cb92

WIP

024497d

Reset back to 8 chunks

99db3d6

WIP

3b4718c

kpjensen added 9 commits April 29, 2025 12:47

Move fns to util

00cb4f4

MM WIP

fd275b7

Prefetch WIP

50f999c

Prefetch WIP

46e5b34

Modify prefetch heuristics

cd1e6ca

WIP

ed39b03

Move limit calculations to utils

4d25b0f

Merge branch 'master' into mm-wip-take-2

3434878

Bump GHA

dbe9176

kpjensen requested a review from Copilot January 14, 2026 18:31

Copilot AI reviewed Jan 14, 2026

View reviewed changes

kpjensen added 6 commits January 15, 2026 08:32

Set IO vector state to IN_FLIGHT in moveCacheWindow

42a712e

Fix script paths

e6088d3

Version

379bd7c

WIP

f4c26ca

Cleanup

a2420cf

Logging cleanup

d6cdca1

kpjensen requested a review from Copilot January 20, 2026 15:26

Copilot AI reviewed Jan 20, 2026

View reviewed changes

kpjensen added 4 commits January 20, 2026 10:37

Add maxMemoryUsagePercent flag

685844a

Fix paths

79837e3

Logging cleanup

a3ffae7

gofmt

67d8947

kpjensen requested a review from Copilot January 20, 2026 20:01

Copilot AI reviewed Jan 20, 2026

View reviewed changes

kpjensen and others added 5 commits January 26, 2026 15:24

Remove unused fn

6aed219

Use done channel to avoid masking panics

2dd3cb1

Fix up empty file case

3fd1fee

gofmt

337c8e0

	// Default to max 15% memory usage of system memory
	// Default to max 10% memory usage of system memory

-umount /home/kjensen/MNT || true
-# mount dxfuse
-/home/kjensen/dxfuse --version
-/home/kjensen/dxfuse -limitedWrite /home/kjensen/MNT testing
-rm -rf /home/kjensen/MNT/testing/1GiB*
-rm -rf /home/kjensen/MNT/testing/1kib*
+# Allow overriding dxfuse binary and mount directory via environment
+DXFUSE_BIN="${DXFUSE_BIN:-/home/kjensen/dxfuse}"
+DXFUSE_MNT_DIR="${DXFUSE_MNT_DIR:-/home/kjensen/MNT}"
+umount "$DXFUSE_MNT_DIR" || true
+# mount dxfuse
+"$DXFUSE_BIN" --version
+"$DXFUSE_BIN" -limitedWrite "$DXFUSE_MNT_DIR" testing
+rm -rf "$DXFUSE_MNT_DIR"/testing/1GiB*
+rm -rf "$DXFUSE_MNT_DIR"/testing/1kib*

	fh.writeBuffer = fsys.uploader.AllocateWriteBuffer(partId, true)
	fh.writeBuffer = fsys.uploader.AllocateWriteBuffer(partId, true /* blocking */)

	// log the above 2 values
	// When verbose logging is enabled, record the cache window parameters
	// (iovIndex, nIovecs, nReadAheadChunks) to aid debugging of readahead behavior.

Conversation

kpjensen commented Jan 14, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 14, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Jan 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Jan 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 20, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants