From 01f084f5d9b364cc0df92c4ed1071dd74a645f63 Mon Sep 17 00:00:00 2001 From: Minh Thai Date: Thu, 21 Feb 2019 16:36:59 -0800 Subject: [PATCH] Strongly reference indices in DfsPackFile DfsBlockCache.Ref might get cleared out if the JVM is running out of memory. As a result, the index is not persisted for the request and will be reloaded unnecessarily. Change-Id: I3b57ad5e6673f77f2dc66177a649ac412a97fe20 Signed-off-by: Minh Thai --- .../internal/storage/dfs/DfsPackFile.java | 241 ++++++++---------- 1 file changed, 113 insertions(+), 128 deletions(-) diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsPackFile.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsPackFile.java index 5b574e49a..451986e83 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsPackFile.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsPackFile.java @@ -98,13 +98,13 @@ public final class DfsPackFile extends BlockBasedFile { private final Object initLock = new Object(); /** Index mapping {@link ObjectId} to position within the pack stream. */ - private volatile DfsBlockCache.Ref index; + private volatile PackIndex index; /** Reverse version of {@link #index} mapping position to {@link ObjectId}. */ - private volatile DfsBlockCache.Ref reverseIndex; + private volatile PackReverseIndex reverseIndex; /** Index of compressed bitmap mapping entire object graph. */ - private volatile DfsBlockCache.Ref bitmapIndex; + private volatile PackBitmapIndex bitmapIndex; /** * Objects we have tried to read, and discovered to be corrupt. @@ -150,15 +150,15 @@ public final class DfsPackFile extends BlockBasedFile { * @return whether the pack index file is loaded and cached in memory. */ public boolean isIndexLoaded() { - DfsBlockCache.Ref idxref = index; - return idxref != null && idxref.has(); + return index != null; } void setPackIndex(PackIndex idx) { long objCnt = idx.getObjectCount(); int recSize = Constants.OBJECT_ID_LENGTH + 8; long sz = objCnt * recSize; - index = cache.putRef(desc.getStreamKey(INDEX), sz, idx); + cache.putRef(desc.getStreamKey(INDEX), sz, idx); + index = idx; } /** @@ -176,12 +176,8 @@ public final class DfsPackFile extends BlockBasedFile { } private PackIndex idx(DfsReader ctx) throws IOException { - DfsBlockCache.Ref idxref = index; - if (idxref != null) { - PackIndex idx = idxref.get(); - if (idx != null) { - return idx; - } + if (index != null) { + return index; } if (invalid) { @@ -192,59 +188,60 @@ public final class DfsPackFile extends BlockBasedFile { .dispatch(new BeforeDfsPackIndexLoadedEvent(this)); synchronized (initLock) { - idxref = index; - if (idxref != null) { - PackIndex idx = idxref.get(); - if (idx != null) { - return idx; - } + if (index != null) { + return index; } - DfsStreamKey idxKey = desc.getStreamKey(INDEX); try { - idxref = cache.getOrLoadRef(idxKey, () -> { - try { - ctx.stats.readIdx++; - long start = System.nanoTime(); - try (ReadableChannel rc = ctx.db.openFile(desc, - INDEX)) { - InputStream in = Channels.newInputStream(rc); - int wantSize = 8192; - int bs = rc.blockSize(); - if (0 < bs && bs < wantSize) { - bs = (wantSize / bs) * bs; - } else if (bs <= 0) { - bs = wantSize; + DfsStreamKey idxKey = desc.getStreamKey(INDEX); + DfsBlockCache.Ref idxref = cache.getOrLoadRef(idxKey, + () -> { + try { + ctx.stats.readIdx++; + long start = System.nanoTime(); + try (ReadableChannel rc = ctx.db.openFile(desc, + INDEX)) { + InputStream in = Channels + .newInputStream(rc); + int wantSize = 8192; + int bs = rc.blockSize(); + if (0 < bs && bs < wantSize) { + bs = (wantSize / bs) * bs; + } else if (bs <= 0) { + bs = wantSize; + } + PackIndex idx = PackIndex.read( + new BufferedInputStream(in, bs)); + int sz = (int) Math.min( + idx.getObjectCount() * REC_SIZE, + Integer.MAX_VALUE); + ctx.stats.readIdxBytes += rc.position(); + index = idx; + return new DfsBlockCache.Ref<>(idxKey, 0, + sz, idx); + } finally { + ctx.stats.readIdxMicros += elapsedMicros( + start); + } + } catch (EOFException e) { + throw new IOException(MessageFormat.format( + DfsText.get().shortReadOfIndex, + desc.getFileName(INDEX)), e); + } catch (IOException e) { + throw new IOException(MessageFormat.format( + DfsText.get().cannotReadIndex, + desc.getFileName(INDEX)), e); } - PackIndex idx = PackIndex - .read(new BufferedInputStream(in, bs)); - int sz = (int) Math.min( - idx.getObjectCount() * REC_SIZE, - Integer.MAX_VALUE); - ctx.stats.readIdxBytes += rc.position(); - return new DfsBlockCache.Ref<>(idxKey, 0, sz, idx); - } finally { - ctx.stats.readIdxMicros += elapsedMicros(start); - } - } catch (EOFException e) { - throw new IOException(MessageFormat.format( - DfsText.get().shortReadOfIndex, - desc.getFileName(INDEX)), e); - } catch (IOException e) { - throw new IOException(MessageFormat.format( - DfsText.get().cannotReadIndex, - desc.getFileName(INDEX)), e); - } - }); + }); + PackIndex idx = idxref.get(); + if (index == null && idx != null) { + index = idx; + } + return index; } catch (IOException e) { invalid = true; throw e; } - PackIndex idx = idxref.get(); - if (idx != null) { - index = idxref; - } - return idx; } } @@ -257,102 +254,90 @@ public final class DfsPackFile extends BlockBasedFile { return null; } - DfsBlockCache.Ref idxref = bitmapIndex; - if (idxref != null) { - PackBitmapIndex bmidx = idxref.get(); - if (bmidx != null) { - return bmidx; - } + if (bitmapIndex != null) { + return bitmapIndex; } synchronized (initLock) { - idxref = bitmapIndex; - if (idxref != null) { - PackBitmapIndex bmidx = idxref.get(); - if (bmidx != null) { - return bmidx; - } + if (bitmapIndex != null) { + return bitmapIndex; } PackIndex idx = idx(ctx); PackReverseIndex revidx = getReverseIdx(ctx); DfsStreamKey bitmapKey = desc.getStreamKey(BITMAP_INDEX); - idxref = cache.getOrLoadRef(bitmapKey, () -> { - ctx.stats.readBitmap++; - long start = System.nanoTime(); - try (ReadableChannel rc = ctx.db.openFile(desc, BITMAP_INDEX)) { - long size; - PackBitmapIndex bmidx; - try { - InputStream in = Channels.newInputStream(rc); - int wantSize = 8192; - int bs = rc.blockSize(); - if (0 < bs && bs < wantSize) { - bs = (wantSize / bs) * bs; - } else if (bs <= 0) { - bs = wantSize; + DfsBlockCache.Ref idxref = cache + .getOrLoadRef(bitmapKey, () -> { + ctx.stats.readBitmap++; + long start = System.nanoTime(); + try (ReadableChannel rc = ctx.db.openFile(desc, + BITMAP_INDEX)) { + long size; + PackBitmapIndex bmidx; + try { + InputStream in = Channels.newInputStream(rc); + int wantSize = 8192; + int bs = rc.blockSize(); + if (0 < bs && bs < wantSize) { + bs = (wantSize / bs) * bs; + } else if (bs <= 0) { + bs = wantSize; + } + in = new BufferedInputStream(in, bs); + bmidx = PackBitmapIndex.read(in, idx, revidx); + } finally { + size = rc.position(); + ctx.stats.readIdxBytes += size; + ctx.stats.readIdxMicros += elapsedMicros(start); + } + int sz = (int) Math.min(size, Integer.MAX_VALUE); + bitmapIndex = bmidx; + return new DfsBlockCache.Ref<>(bitmapKey, 0, sz, + bmidx); + } catch (EOFException e) { + throw new IOException(MessageFormat.format( + DfsText.get().shortReadOfIndex, + desc.getFileName(BITMAP_INDEX)), e); + } catch (IOException e) { + throw new IOException(MessageFormat.format( + DfsText.get().cannotReadIndex, + desc.getFileName(BITMAP_INDEX)), e); } - in = new BufferedInputStream(in, bs); - bmidx = PackBitmapIndex.read(in, idx, revidx); - } finally { - size = rc.position(); - ctx.stats.readIdxBytes += size; - ctx.stats.readIdxMicros += elapsedMicros(start); - } - int sz = (int) Math.min(size, Integer.MAX_VALUE); - return new DfsBlockCache.Ref<>(bitmapKey, 0, sz, bmidx); - } catch (EOFException e) { - throw new IOException( - MessageFormat.format(DfsText.get().shortReadOfIndex, - desc.getFileName(BITMAP_INDEX)), - e); - } catch (IOException e) { - throw new IOException( - MessageFormat.format(DfsText.get().cannotReadIndex, - desc.getFileName(BITMAP_INDEX)), - e); - } - }); + }); PackBitmapIndex bmidx = idxref.get(); - if (bmidx != null) { - bitmapIndex = idxref; + if (bitmapIndex == null && bmidx != null) { + bitmapIndex = bmidx; } - return bmidx; + return bitmapIndex; } } PackReverseIndex getReverseIdx(DfsReader ctx) throws IOException { - DfsBlockCache.Ref revref = reverseIndex; - if (revref != null) { - PackReverseIndex revidx = revref.get(); - if (revidx != null) { - return revidx; - } + if (reverseIndex != null) { + return reverseIndex; } synchronized (initLock) { - revref = reverseIndex; - if (revref != null) { - PackReverseIndex revidx = revref.get(); - if (revidx != null) { - return revidx; - } + if (reverseIndex != null) { + return reverseIndex; } PackIndex idx = idx(ctx); DfsStreamKey revKey = new DfsStreamKey.ForReverseIndex( desc.getStreamKey(INDEX)); - revref = cache.getOrLoadRef(revKey, () -> { - PackReverseIndex revidx = new PackReverseIndex(idx); - int sz = (int) Math.min(idx.getObjectCount() * 8, - Integer.MAX_VALUE); - return new DfsBlockCache.Ref<>(revKey, 0, sz, revidx); - }); + DfsBlockCache.Ref revref = cache + .getOrLoadRef(revKey, () -> { + PackReverseIndex revidx = new PackReverseIndex(idx); + int sz = (int) Math.min(idx.getObjectCount() * 8, + Integer.MAX_VALUE); + reverseIndex = revidx; + return new DfsBlockCache.Ref<>(revKey, 0, sz, revidx); + }); PackReverseIndex revidx = revref.get(); - if (revidx != null) { - reverseIndex = revref; + if (reverseIndex == null && revidx != null) { + reverseIndex = revidx; } - return revidx; + return reverseIndex; } }