Skip to content

Commit

Permalink
Implement the two ideas in b/290807073 to reduce memory spikes during…
Browse files Browse the repository at this point in the history
… string compression in `FileWriteAction`.

* Presize the `ByteArrayOutputStream` more appropriately. This holds the compressed data, so presizing it to the number of uncompressed bytes can be way more space than needed. A code comment justifies the chosen presizing.
* Use `StringUnsafe` to get a reference to the string's internal byte array instead of calling `getBytes()`, which makes a copy.

PiperOrigin-RevId: 548132680
Change-Id: Id5eb02013e652ae2f0e47a750011491101738df8
  • Loading branch information
justinhorvitz authored and copybara-github committed Jul 14, 2023
1 parent 5eff703 commit 7966079
Show file tree
Hide file tree
Showing 2 changed files with 27 additions and 10 deletions.
1 change: 1 addition & 0 deletions src/main/java/com/google/devtools/build/lib/analysis/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -419,6 +419,7 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/starlarkbuildapi/core",
"//src/main/java/com/google/devtools/build/lib/starlarkbuildapi/platform",
"//src/main/java/com/google/devtools/build/lib/starlarkbuildapi/test",
"//src/main/java/com/google/devtools/build/lib/unsafe:string",
"//src/main/java/com/google/devtools/build/lib/util",
"//src/main/java/com/google/devtools/build/lib/util:detailed_exit_code",
"//src/main/java/com/google/devtools/build/lib/util:filetype",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import com.google.devtools.build.lib.collect.nestedset.Order;
import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
import com.google.devtools.build.lib.events.EventHandler;
import com.google.devtools.build.lib.unsafe.StringUnsafe;
import com.google.devtools.build.lib.util.Fingerprint;
import com.google.devtools.build.lib.util.OnDemandString;
import java.io.ByteArrayInputStream;
Expand Down Expand Up @@ -169,26 +170,36 @@ public static FileWriteAction create(
}

private static final class CompressedString extends OnDemandString {
final byte[] bytes;
final int uncompressedSize;
private final byte[] compressedBytes;
private final int uncompressedSize;
private final byte coder;

CompressedString(String chars) {
byte[] dataToCompress = chars.getBytes(ISO_8859_1);
ByteArrayOutputStream byteStream = new ByteArrayOutputStream(dataToCompress.length);
// Grab the string's internal byte array. Calling getBytes() makes a copy, which can cause
// memory spikes resulting in OOMs (b/290807073). Do not mutate this!
byte[] dataToCompress = StringUnsafe.getInstance().getByteArray(chars);

// Empirically, compressed sizes range from roughly 1/100 to 3/4 of the uncompressed size.
// Presize on the small end to avoid over-allocating memory.
ByteArrayOutputStream byteStream = new ByteArrayOutputStream(dataToCompress.length / 100);

try (GZIPOutputStream zipStream = new GZIPOutputStream(byteStream)) {
zipStream.write(dataToCompress);
} catch (IOException e) {
// This should be impossible since we're writing to a byte array.
throw new RuntimeException(e);
throw new IllegalStateException(e);
}

this.compressedBytes = byteStream.toByteArray();
this.uncompressedSize = dataToCompress.length;
this.bytes = byteStream.toByteArray();
this.coder = StringUnsafe.getInstance().getCoder(chars);
}

@Override
public String toString() {
byte[] uncompressedBytes = new byte[uncompressedSize];
try (GZIPInputStream zipStream = new GZIPInputStream(new ByteArrayInputStream(bytes))) {
try (GZIPInputStream zipStream =
new GZIPInputStream(new ByteArrayInputStream(compressedBytes))) {
int read;
int totalRead = 0;
while (totalRead < uncompressedSize
Expand All @@ -198,13 +209,18 @@ public String toString() {
}
if (totalRead != uncompressedSize) {
// This should be impossible.
throw new RuntimeException("Corrupt byte buffer in FileWriteAction.");
throw new IllegalStateException("Corrupt byte buffer in FileWriteAction.");
}
} catch (IOException e) {
// This should be impossible since we're reading from a byte array.
throw new RuntimeException(e);
throw new IllegalStateException(e);
}

try {
return StringUnsafe.getInstance().newInstance(uncompressedBytes, coder);
} catch (ReflectiveOperationException e) {
throw new IllegalStateException(e);
}
return new String(uncompressedBytes, ISO_8859_1);
}
}

Expand Down

0 comments on commit 7966079

Please sign in to comment.