Skip to content

Commit

Permalink
gin: Fix failing DCHECK with memory dumps
Browse files Browse the repository at this point in the history
Before this CL we invoked GetHeapStatistics() first and then got
memory dump information for the individual spaces with
GetHeapSpaceStatistics(). The DCHECK ensured that the total heap size
was at least the sum of the individual spaces.

However with concurrent allocation from background threads that DCHECK
isn't guaranteed to hold anymore since there may be additional
allocations between GetHeapStatistics() and one of the
GetHeapSpaceStatistics().

We can fix the DCHECK by invoking GetHeapStatistics() after all
GetHeapSpaceStatistics().

Bug: chromium:1301865
Change-Id: I4cc732f3ab840a22075468f0576f8591b1e4cbb2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3854009
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Commit-Queue: Dominik Inführ <dinfuehr@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1038793}
  • Loading branch information
Dominik Inführ authored and Chromium LUCI CQ committed Aug 24, 2022
1 parent c0fc437 commit c3d946c
Showing 1 changed file with 6 additions and 6 deletions.
12 changes: 6 additions & 6 deletions gin/v8_isolate_memory_dump_provider.cc
Original file line number Diff line number Diff line change
Expand Up @@ -170,12 +170,6 @@ void V8IsolateMemoryDumpProvider::DumpHeapStatistics(
"isolate_0x%" PRIXPTR,
reinterpret_cast<uintptr_t>(isolate_holder_->isolate()));

// Dump statistics of the heap's spaces.
v8::HeapStatistics heap_statistics;
// The total heap sizes should be sampled before the individual space sizes
// because of concurrent allocation. DCHECKs below rely on this order.
isolate_holder_->isolate()->GetHeapStatistics(&heap_statistics);

IsolateHolder::IsolateType isolate_type = isolate_holder_->isolate_type();
std::string dump_base_name = "v8/" + IsolateTypeString(isolate_type);
std::string dump_name_suffix =
Expand Down Expand Up @@ -217,6 +211,12 @@ void V8IsolateMemoryDumpProvider::DumpHeapStatistics(
space_used_size);
}

// Dump statistics of the heap's spaces.
v8::HeapStatistics heap_statistics;
// The total heap sizes should be sampled before the individual space sizes
// because of concurrent allocation. DCHECKs below rely on this order.
isolate_holder_->isolate()->GetHeapStatistics(&heap_statistics);

// Sanity checks that all spaces are accounted for in GetHeapSpaceStatistics.
// Background threads may be running and allocating concurrently, so the sum
// of space sizes may be exceed the total heap size that was sampled earlier.
Expand Down

0 comments on commit c3d946c

Please sign in to comment.