git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[JGIT PATCH 10/23] Make mmap mode more reliable by forcing GC at the correct spot

From
Shawn O. Pearce <spearce@spearce.org>
Date
Dec 25, 2008, 02:11 UTC
Message-ID
<1230171079-17156-11-git-send-email-spearce@spearce.org>
In-Reply-To
<1230171079-17156-10-git-send-email-spearce@spearce.org>

ObjectLoaders can defer the bulk of their data access to the getCachedBytes() method invocation, rather than at the time of their construction. That means there's code paths into the mmap allocator which aren't protected by the "try GC and try again" within openObject, resulting in some random crashes due to mmap failures.

The right place to check for mmap failure and retry is within WindowedFile.loadWindow, where we actually invoke the mmap call. If the call fails, we need can immediately retry it, and if it fails again, we can temporarily degrade to the byte[] variant until the JVM is able to evict and GC enough space.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 .../src/org/spearce/jgit/lib/Repository.java       |   27 +-----------
 .../src/org/spearce/jgit/lib/WindowedFile.java     |   43 ++++++++++++++-----
 2 files changed, 34 insertions(+), 36 deletions(-)
diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/Repository.java b/org.spearce.jgit/src/org/spearce/jgit/lib/Repository.java
index 02f8103..e1c4049 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/lib/Repository.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/Repository.java
@@ -290,30 +290,9 @@ public ObjectLoader openObject(final WindowCursor curs, final AnyObjectId id)
 		int k = packs.length;
 		if (k > 0) {
 			do {
-				try {
-					final ObjectLoader ol = packs[--k].get(curs, id);
-					if (ol != null)
-						return ol;
-				} catch (IOException ioe) {
-					// This shouldn't happen unless the pack was corrupted
-					// after we opened it or the VM runs out of memory. This is
-					// a know problem with memory mapped I/O in java and have
-					// been noticed with JDK < 1.6. Tell the gc that now is a good
-					// time to collect and try once more.
-					try {
-						curs.release();
-						System.gc();
-						final ObjectLoader ol = packs[k].get(curs, id);
-						if (ol != null)
-							return ol;
-					} catch (IOException ioe2) {
-						ioe2.printStackTrace();
-						ioe.printStackTrace();
-						// Still fails.. that's BAD, maybe the pack has
-						// been corrupted after all, or the gc didn't manage
-						// to release enough previously mmaped areas.
-					}
-				}
+				final ObjectLoader ol = packs[--k].get(curs, id);
+				if (ol != null)
+					return ol;
 			} while (k > 0);
 		}
 		try {
diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/WindowedFile.java b/org.spearce.jgit/src/org/spearce/jgit/lib/WindowedFile.java
index f640c42..dd94f24 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/lib/WindowedFile.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/WindowedFile.java
@@ -299,22 +299,41 @@ void cacheClose() {
 	}
 
 	void loadWindow(final WindowCursor curs, final int windowId,
-			final long pos, final int windowSize) throws IOException {
+			final long pos, final int size) throws IOException {
 		if (WindowCache.mmap) {
-			final MappedByteBuffer map = fd.getChannel().map(MapMode.READ_ONLY,
-					pos, windowSize);
-			if (map.hasArray()) {
-				final byte[] b = map.array();
-				curs.window = new ByteArrayWindow(this, pos, windowId, b);
-				curs.handle = b;
-			} else {
-				curs.window = new ByteBufferWindow(this, pos, windowId, map);
-				curs.handle = map;
+			MappedByteBuffer map;
+			try {
+				map = fd.getChannel().map(MapMode.READ_ONLY, pos, size);
+			} catch (IOException e) {
+				// The most likely reason this failed is the JVM has run out
+				// of virtual memory. We need to discard quickly, and try to
+				// force the GC to finalize and release any existing mappings.
+				try {
+					curs.release();
+					System.gc();
+					System.runFinalization();
+					map = fd.getChannel().map(MapMode.READ_ONLY, pos, size);
+				} catch (IOException ioe2) {
+					// Temporarily disable mmap and do buffered disk IO.
+					//
+					map = null;
+					System.err.println("warning: mmap failure: "+ioe2);
+				}
+			}
+			if (map != null) {
+				if (map.hasArray()) {
+					final byte[] b = map.array();
+					curs.window = new ByteArrayWindow(this, pos, windowId, b);
+					curs.handle = b;
+				} else {
+					curs.window = new ByteBufferWindow(this, pos, windowId, map);
+					curs.handle = map;
+				}
+				return;
 			}
-			return;
 		}
 
-		final byte[] b = new byte[windowSize];
+		final byte[] b = new byte[size];
 		synchronized (fd) {
 			fd.seek(pos);
 			fd.readFully(b);

-- 
1.6.1.rc4.301.g5497a
Previous: Shawn O. PearceNext: Shawn O. Pearce
Message 11 of 26 in “Misc. performance tweaks”
  1. 00/18 Misc. performance tweaksShawn O. Pearce, Dec 25, 2008
  2. 01/23 Improve hit performance on the UnpackedObjectCacheShawn O. Pearce, Dec 25, 2008
  3. 02/23 Add MutableObjectId.clear() to set the id to zeroIdShawn O. Pearce, Dec 25, 2008
  4. 03/23 Allow TreeWalk callers to pass a MutableObjectId to get the current idShawn O. Pearce, Dec 25, 2008
  5. 04/23 Switch ObjectWalk to use the new MutableObjectId form in TreeWalkShawn O. Pearce, Dec 25, 2008
  6. 05/23 Change walker based fetch to use TreeWalk's MutableObjectId accessorShawn O. Pearce, Dec 25, 2008
  7. 06/23 Reduce garbage allocation when using TreeWalkShawn O. Pearce, Dec 25, 2008
  8. 07/23 Switch ObjectWalk to use a naked CanonicalTreeParser because its fasterShawn O. Pearce, Dec 25, 2008
  9. 08/23 Remove the unused PackFile.get(ObjectId) formShawn O. Pearce, Dec 25, 2008
  10. 09/23 Remove getId from ObjectLoader API as its unnecessary overheadShawn O. Pearce, Dec 25, 2008
  11. 10/23 Make mmap mode more reliable by forcing GC at the correct spotShawn O. Pearce, Dec 25, 2008
  12. 11/23 Rewrite WindowCache to use a hash tableShawn O. Pearce, Dec 25, 2008
  13. 12/23 Change ByteArrayWindow to read content outside of WindowCache's lockShawn O. Pearce, Dec 25, 2008
  14. 13/23 Dispose of RevCommit buffers when they aren't used in PackWriterShawn O. Pearce, Dec 25, 2008
  15. 14/23 Don't unpack delta chains while writing a pack from a pack v1 indexShawn O. Pearce, Dec 25, 2008
  16. 15/23 Don't unpack delta chains while converting delta to whole objectShawn O. Pearce, Dec 25, 2008
  17. 16/23 Defer parsing of the ObjectId while walking a PackIndex IteratorShawn O. Pearce, Dec 25, 2008
  18. 17/23 Only do one getCachedBytes per whole object writtenShawn O. Pearce, Dec 25, 2008
  19. 18/23 Correctly use a long for the offsets within a generated packShawn O. Pearce, Dec 25, 2008
  20. 19/23 Allow more direct access to determine isWrittenShawn O. Pearce, Dec 25, 2008
  21. 20/23 Move "wantWrite" field of ObjectToPack into the flags fieldShawn O. Pearce, Dec 25, 2008
  22. 21/23 Use an ArrayList for the reuseLoader collection in PackWriterShawn O. Pearce, Dec 25, 2008
  23. 22/23 Don't cut off existing delta chains if we are reusing deltasShawn O. Pearce, Dec 25, 2008
  24. 23/23 Correctly honor the thin parameter to PackWriter.writePackShawn O. Pearce, Dec 25, 2008
  25. Robin RosenbergDec 27, 2008
  26. 11/23 Rewrite WindowCache to use a hash tableShawn O. Pearce, Dec 27, 2008

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.