From beaf591dfb62a279c61867cfd4fab1584b3aa6fb Mon Sep 17 00:00:00 2001 From: "Maxim.Mossienko" Date: Thu, 18 Jan 2018 10:59:41 +0100 Subject: [PATCH] Fix of the bug in PersistentHashMap's backward compact When compacting fragmented PersistentHashMap in some cases records got content not in the write order. In particular, append calls can produce fragments A,B,C and after compact record read loads C,B,A --- .../intellij/util/io/PersistentMapTest.java | 68 +++++++++++++++++-- .../io/PersistentHashMapValueStorage.java | 16 +++-- 2 files changed, 74 insertions(+), 10 deletions(-) diff --git a/platform/platform-tests/testSrc/com/intellij/util/io/PersistentMapTest.java b/platform/platform-tests/testSrc/com/intellij/util/io/PersistentMapTest.java index 73cb83731476..ad42155d16ba 100644 --- a/platform/platform-tests/testSrc/com/intellij/util/io/PersistentMapTest.java +++ b/platform/platform-tests/testSrc/com/intellij/util/io/PersistentMapTest.java @@ -25,16 +25,72 @@ import com.intellij.util.io.storage.AbstractStorage; import junit.framework.AssertionFailedError; import org.jetbrains.annotations.NotNull; -import java.io.DataInput; -import java.io.DataOutput; -import java.io.File; -import java.io.IOException; +import java.io.*; import java.util.*; /** * @author Eugene Zhuravlev */ public class PersistentMapTest extends PersistentMapTestBase { + public void testRetainWriteOrderWhenCompactingBackward() throws IOException { + clearMap(myFile, myMap); + myMap = null; + + String longString = StringUtil.repeat("1234567890", 120); + assertTrue(longString.length() > PersistentHashMapValueStorage.BLOCK_SIZE_TO_WRITE_WHEN_SOFT_MAX_RETAINED_LIMIT_IS_HIT); + String removalMarker = "\uFFFF"; + PersistentMapPerformanceTest.MapConstructor> mapConstructor = + (file) -> new PersistentHashMap<>( + file, + EnumeratorIntegerDescriptor.INSTANCE, + new DataExternalizer>() { + @Override + public void save(@NotNull DataOutput out, Collection value) throws IOException { + for(String str:value) { + IOUtil.writeUTF(out, str); + } + } + + @Override + public Collection read(@NotNull DataInput in) throws IOException { + List result = new ArrayList<>(); + while(((InputStream)in).available() > 0) { + String string = IOUtil.readUTF(in); + if (string.equals(removalMarker)) { + result.remove(result.size() - 1); + } else { + result.add(string); + } + } + return result; + } + } + ); + PersistentHashMap> map = mapConstructor.createMap(myFile); + try { + int keys = 10_000; + for(int iteration = 0; iteration < 5; ++iteration) { + String toAppend = iteration % 2 == 0 ? longString : removalMarker; + for (int i = 0; i < keys; ++i) { + map.appendData(i, out -> IOUtil.writeUTF(out, toAppend)); + } + } + + map.close(); + assertTrue(map.getValueStorage().getSize() > 2 * PersistentHashMapValueStorage.SOFT_MAX_RETAINED_LIMIT); + map = mapConstructor.createMap(myFile); + map.compact(); + + for (int i = 0; i < keys; ++i) { + Collection strings = map.get(i); + assertTrue(strings != null && strings.size() == 1); + assertEquals(longString, strings.iterator().next()); + } + } finally { + clearMap(myFile, map); + } + } + public void testMap() throws IOException { myMap.put("AAA", "AAA_VALUE"); @@ -308,7 +364,7 @@ public class PersistentMapTest extends PersistentMapTestBase { public void testOpeningWithCompact2() throws IOException { File file = FileUtil.createTempFile("persistent", "map"); - PersistentHashMap map = new PersistentHashMap<>(file, new IntInlineKeyDescriptor(), EnumeratorStringDescriptor.INSTANCE); + PersistentHashMap map = new PersistentHashMap<>(file, EnumeratorIntegerDescriptor.INSTANCE, EnumeratorStringDescriptor.INSTANCE); try { final int stringsCount = 5/*1000000*/; Map testMapping = new LinkedHashMap<>(stringsCount); @@ -319,7 +375,7 @@ public class PersistentMapTest extends PersistentMapTestBase { map.put(i, value); } map.close(); - map = new PersistentHashMap<>(file, new IntInlineKeyDescriptor(), EnumeratorStringDescriptor.INSTANCE); + map = new PersistentHashMap<>(file, EnumeratorIntegerDescriptor.INSTANCE, EnumeratorStringDescriptor.INSTANCE); { // before compact final Collection allKeys = new HashSet<>(map.getAllKeysWithExistingMapping()); diff --git a/platform/util/src/com/intellij/util/io/PersistentHashMapValueStorage.java b/platform/util/src/com/intellij/util/io/PersistentHashMapValueStorage.java index 49fe193d1147..7e0776a17fa0 100644 --- a/platform/util/src/com/intellij/util/io/PersistentHashMapValueStorage.java +++ b/platform/util/src/com/intellij/util/io/PersistentHashMapValueStorage.java @@ -47,6 +47,9 @@ public class PersistentHashMapValueStorage { private static final int CACHE_PROTECTED_QUEUE_SIZE = 10; private static final int CACHE_PROBATIONAL_QUEUE_SIZE = 20; private static final long MAX_RETAINED_LIMIT_WHEN_COMPACTING = 100 * 1024 * 1024; + + static final long SOFT_MAX_RETAINED_LIMIT = 10 * 1024 * 1024; + static final int BLOCK_SIZE_TO_WRITE_WHEN_SOFT_MAX_RETAINED_LIMIT_IS_HIT = 1024; public static class CreationTimeOptions { public static final ThreadLocal EXCEPTIONAL_IO_CANCELLATION = new ThreadLocal(); @@ -361,12 +364,10 @@ public class PersistentHashMapValueStorage { int fragments = 0; int newFragments = 0; int allRecordsLength = 0; + byte[] stuffFromPreviousRecord = null; int bytesRead = (int)(mySize - (mySize / fileBufferLength) * fileBufferLength); long retained = 0; - final long softMaxRetainedLimit = 10 * 1024* 1024; - final int blockSizeToWriteWhenSoftMaxRetainedLimitIsHit = 1024; - final long maxRetainedLimit = 100 * 1024* 1024; while(lastReadOffset != 0) { final long readStartOffset = lastReadOffset - bytesRead; @@ -457,12 +458,19 @@ public class PersistentHashMapValueStorage { info.newValueAddress = storage.appendBytes(b, 0, chunkSize, info.newValueAddress); ++newFragments; } else { - if (retained > softMaxRetainedLimit && b.length > blockSizeToWriteWhenSoftMaxRetainedLimitIsHit || + if (retained > SOFT_MAX_RETAINED_LIMIT && b.length > BLOCK_SIZE_TO_WRITE_WHEN_SOFT_MAX_RETAINED_LIMIT_IS_HIT || retained > MAX_RETAINED_LIMIT_WHEN_COMPACTING) { + // to avoid OOME we need to save 'b' accumulated from chunks + // to preserve write order prev data is loaded in usual backward chunk reads + ReadResult result = readBytes(prevChunkAddress); + info.newValueAddress = storage.appendBytes(result.buffer, 0, result.buffer.length, info.newValueAddress); ++newFragments; info.newValueAddress = storage.appendBytes(b, 0, chunkSize, info.newValueAddress); + ++newFragments; info.value = null; + info.valueAddress = 0; retained -= b.length; + continue; } else { info.value = b; }