Oilpan: Don't reset heap counters in a snapshot GC

Source/platform/heap/Heap.cpp

 /*
  * Copyright (C) 2013 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 2001 matching lines @@
 
     // Disallow allocation during garbage collection (but not during the
     // finalization that happens when the gcScope is torn down).
     ThreadState::NoAllocationScope noAllocationScope(state);
 
     preGC();
 
     StackFrameDepthScope stackDepthScope;
 
     size_t totalObjectSize = Heap::allocatedObjectSize() + Heap::markedObjectSize();
-    Heap::resetHeapCounters();
+    if (gcType == ThreadState::TakeSnapshot)

[ssid, 2015/06/18 12:01:07]

Do you mean != ?
With !=,

The allocatedObjectSize seems to be showing sensible values.
The total live_size calculated from each heap page (both normal and large) is
less than Heap::markedObjectSize. Is this expected or I am calculating it wrong?

The markedObjectSize keeps increasing with each trace GC! I think this needs to
be reset.

I am changing the total_allocated_size displayed in tracing to be
allocatedObjectSize + markedObjectSize. Makes sense?

Also estimatedLiveObjectSize > allocatedSpace. I think I am missing some bit
mask ?

Also, the GC with snapshot is failing when the page has large object
allocations, filed a bug (crbug.com/501771)

[haraken, 2015/06/18 18:43:43]

Nice catch!
How are you calculating the "total live_size"?
I think this is expected. Note that the markedObjectSize is counted up in the
lazy sweeping phase. It is not counted up in the marking phase. So, as the lazy
sweeping sweeps more objects, the markedObjectSize increases.

(The reason we don't count up the markedObjectSize in the marking phase is that
we want to reduce the pause time of the marking phase as much as possible.
However, we should reconsider this -- we should investigate if it really affects
the pause time.)
Makes sense.
This is OK, because estimatedLiveObjectSize takes care of the size of objects
allocated in PartitionAlloc. ...Yeah, this is nasty. Now yutak is trying to make
the estimatedLiveObjectSize saner, so let's just wait for his awesome work :)
I'm sorry it's causing your headache.

Just to clarify:

allocatedSpace: The total amount of the memory Oilpan has allocated.
allocatedObjectSize: The size of objects Oilpan has allocated since the last GC.
markedObjectSize: The size of marked objects in the last GC. This value is
finalized when the lazy sweeping completes.
estimatedLiveObjectSize: The estimated live object size in both Oilpan and
PartitionAlloc. Currently the estimation looks broken, and yutak is looking into
the issue.
Sorry again. I'll fix it.

+        Heap::resetHeapCounters();
 
     // 1. Trace persistent roots.
     ThreadState::visitPersistentRoots(gcScope.visitor());
 
     // 2. Trace objects reachable from the stack.  We do this independent of the
     // given stackState since other threads might have a different stack state.
     ThreadState::visitStackRoots(gcScope.visitor());
 
     // 3. Transitive closure to trace objects including ephemerons.
     processMarkingStack(gcScope.visitor());
@@ skipping 289 matching lines @@
 size_t Heap::s_allocatedObjectSize = 0;
 size_t Heap::s_allocatedSpace = 0;
 size_t Heap::s_markedObjectSize = 0;
 // We don't want to use 0 KB for the initial value because it may end up
 // triggering the first GC of some thread too prematurely.
 size_t Heap::s_estimatedLiveObjectSize = 512 * 1024;
 size_t Heap::s_externalObjectSizeAtLastGC = 0;
 double Heap::s_estimatedMarkingTimePerByte = 0.0;
 
 } // namespace blink