Commit 5e7e1b19 authored by Benoît Lizé's avatar Benoît Lizé Committed by Commit Bot

blink/bindings: Use PartitionAlloc for zlib's temporary data.

zlib uses malloc() to allocate temporary data. Unfortunately, on Android,
jemalloc (malloc()'s underlying implementation) caches temporary data after
free() in a thread-local cache. As renderers don't allocate from malloc() often
in background threads, this data is not reclaimed. This increases memory usage
by up to 256kiB per background compression thread, which is the cause for a
sizable malloc() memory regression from foreground string compression (see
linked bug).

The deeper reason for the regression is more complex, see details in the linked
bug, and
https://docs.google.com/document/d/1aRIifaHF5l9relq9vHirTOWt2AZoAkDNqHiYZrWGE9A/edit?usp=sharing

It involves some interaction between:
- jemalloc implementation and memory accounting
- Android's configuration of jemalloc
- Chrome's allocation and threading patterns
- zlib's allocation patterns

Nevertheless, the regression is real, and to mitigate it, use PartitionAlloc to
allocate zlib's temporary data.

This is not necessary for decompression, as the allocation patterns are not the
same, and the main thread doesn't have the same issues. As such, only do it for
compression, to avoid a needlessly complex CL.

On PartitionAlloc vs malloc():
The only path using partition alloc in this CL in from the renderer process,
where it is already widely used. The issue is less likely to appear in the
browser process as malloc() is more widely used there, though further
investigation may reveal issues.

Android vs everywhere:
The issue is present at least with jemalloc(), that is Android. However thread
caches and arena-allocators are used on other platforms as well, and as the
allocation is not performance-sensitive, using PartitionAlloc everwhere makes
the code simpler, and protects against potential issues (as we know that this
specific issue does not arise with PartitionAlloc).

Bug: 931553, 924164
Change-Id: I9e94e9cea5d51fac67b04fdb1681427e57bfbe1f
Reviewed-on: https://chromium-review.googlesource.com/c/1472578
Commit-Queue: Benoit L <lizeb@chromium.org>
Reviewed-by: default avatarAlexei Svitkine <asvitkine@chromium.org>
Reviewed-by: default avatarKentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#632372}
parent 9a13a934
......@@ -498,8 +498,14 @@ void ParkableStringImpl::CompressInBackground(
ok = buffer.data();
size_t compressed_size;
if (ok) {
// Use partition alloc for zlib's temporary data. This is crucial to avoid
// leaking memory on Android, see the details in crbug.com/931553.
auto fast_malloc = [](size_t size) {
return WTF::Partitions::FastMalloc(size, "ZlibTemporaryData");
};
ok = compression::GzipCompress(data, buffer.data(), buffer.size(),
&compressed_size);
&compressed_size, fast_malloc,
WTF::Partitions::FastFree);
}
#if defined(ADDRESS_SANITIZER)
......
......@@ -40,7 +40,9 @@ const int kZlibMemoryLevel = 8;
int GzipCompressHelper(Bytef* dest,
uLongf* dest_length,
const Bytef* source,
uLong source_length) {
uLong source_length,
void* (*malloc_fn)(size_t),
void (*free_fn)(void*)) {
z_stream stream;
stream.next_in = bit_cast<Bytef*>(source);
......@@ -50,9 +52,31 @@ int GzipCompressHelper(Bytef* dest,
if (static_cast<uLong>(stream.avail_out) != *dest_length)
return Z_BUF_ERROR;
stream.zalloc = static_cast<alloc_func>(0);
stream.zfree = static_cast<free_func>(0);
stream.opaque = static_cast<voidpf>(0);
// Cannot convert capturing lambdas to function pointers directly, hence the
// structure.
struct MallocFreeFunctions {
void* (*malloc_fn)(size_t);
void (*free_fn)(void*);
} malloc_free = {malloc_fn, free_fn};
if (malloc_fn) {
DCHECK(free_fn);
auto zalloc = [](void* opaque, uInt items, uInt size) {
return reinterpret_cast<MallocFreeFunctions*>(opaque)->malloc_fn(items *
size);
};
auto zfree = [](void* opaque, void* address) {
return reinterpret_cast<MallocFreeFunctions*>(opaque)->free_fn(address);
};
stream.zalloc = static_cast<alloc_func>(zalloc);
stream.zfree = static_cast<free_func>(zfree);
stream.opaque = static_cast<voidpf>(&malloc_free);
} else {
stream.zalloc = static_cast<alloc_func>(0);
stream.zfree = static_cast<free_func>(0);
stream.opaque = static_cast<voidpf>(0);
}
gz_header gzip_header;
memset(&gzip_header, 0, sizeof(gzip_header));
......@@ -126,14 +150,17 @@ namespace compression {
bool GzipCompress(base::StringPiece input,
char* output_buffer,
size_t output_buffer_size,
size_t* compressed_size) {
size_t* compressed_size,
void* (*malloc_fn)(size_t),
void (*free_fn)(void*)) {
static_assert(sizeof(Bytef) == 1, "");
// uLongf can be larger than size_t.
uLongf compressed_size_long = static_cast<uLongf>(output_buffer_size);
if (GzipCompressHelper(bit_cast<Bytef*>(output_buffer), &compressed_size_long,
bit_cast<const Bytef*>(input.data()),
static_cast<uLongf>(input.size())) != Z_OK) {
static_cast<uLongf>(input.size()), malloc_fn,
free_fn) != Z_OK) {
return false;
}
// No overflow, as compressed_size_long <= output.size() which is a size_t.
......@@ -156,8 +183,8 @@ bool GzipCompress(base::StringPiece input, std::string* output) {
}
if (GzipCompressHelper(compressed_data, &compressed_data_size,
bit_cast<const Bytef*>(input.data()),
input_size) != Z_OK) {
bit_cast<const Bytef*>(input.data()), input_size,
nullptr, nullptr) != Z_OK) {
free(compressed_data);
return false;
}
......
......@@ -15,11 +15,15 @@ namespace compression {
// |output_buffer|, of size |output_buffer_size|. If the buffer is large enough
// and compression succeeds, |compressed_size| points to the compressed data
// size after the call.
// |malloc_fn| and |free_fn| are pointers to malloc() and free()-like functions,
// or nullptr to use the standard ones.
// Returns true for success.
bool GzipCompress(base::StringPiece input,
char* output_buffer,
size_t output_buffer_size,
size_t* compressed_size);
size_t* compressed_size,
void* (*malloc_fn)(size_t),
void (*free_fn)(void*));
// Compresses the data in |input| using gzip, storing the result in |output|.
// |input| and |output| are allowed to point to the same string (in-place
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment