[oilpan]: Change marking to do precise roots first and conservative second.

[oilpan]: Change marking to do precise roots first and conservative second.

This way we avoid conservatively scanning objects that were reachable through precise pointers.

R=ager@chromium.org, erik.corry@gmail.com, haraken@chromium.org, oilpan-reviews@chromium.org, sigbjornf@opera.com, tkent@chromium.org, zerny@chromium.org
BUG=

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=178664

[haraken, 2014-07-22 11:32:43]

LGTM. Nice change.

https://codereview.chromium.org/405403003/diff/1/Source/platform/heap/Heap.cpp
File Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/405403003/diff/1/Source/platform/heap/Heap.cp...
Source/platform/heap/Heap.cpp:2085: // 2. trace object reachable from the
persistent roots including ephemerons.

object => objects

https://codereview.chromium.org/405403003/diff/1/Source/platform/heap/Heap.cp...
Source/platform/heap/Heap.cpp:2138: // only cases where this could happen is if
a subsequent conservative

cases => case

[wibling-chromium, 2014-07-22 11:56:56]

Thanks for the review!

https://codereview.chromium.org/405403003/diff/1/Source/platform/heap/Heap.cpp
File Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/405403003/diff/1/Source/platform/heap/Heap.cp...
Source/platform/heap/Heap.cpp:2085: // 2. trace object reachable from the
persistent roots including ephemerons.
On 2014/07/22 11:32:43, haraken wrote:
> 
> object => objects

Done.

https://codereview.chromium.org/405403003/diff/1/Source/platform/heap/Heap.cp...
Source/platform/heap/Heap.cpp:2138: // only cases where this could happen is if
a subsequent conservative
On 2014/07/22 11:32:43, haraken wrote:
> 
> cases => case

I think it should be "cases" since I mention two scenarios below, ie. global
conservative GC or programming error.

[Erik Corry, 2014-07-22 12:39:27]

lgtm

https://codereview.chromium.org/405403003/diff/1/Source/platform/heap/Heap.cpp
File Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/405403003/diff/1/Source/platform/heap/Heap.cp...
Source/platform/heap/Heap.cpp:2092: // 4. trace objects reachable from the stack
"roots" including ephemerons.
This will always do at least one run through the ephemerons even if there is
nothing on the marking stack. That's correct, but it's only needed if at least
one conservative pointer was found. Since we already have a flag for that, we
could use it here to not bother calling processMarkingStack a second time.

https://codereview.chromium.org/405403003/diff/20001/Source/platform/heap/Thr...
File Source/platform/heap/ThreadState.cpp (left):

https://codereview.chromium.org/405403003/diff/20001/Source/platform/heap/Thr...
Source/platform/heap/ThreadState.cpp:467: // handles on other threads or
CrossThreadPersistents. The only cases where
This comment was just deleted, but it looks helpful to me. Can it find a new
home?

[wibling-chromium, 2014-07-22 12:46:33]

Thanks for the review!

https://codereview.chromium.org/405403003/diff/1/Source/platform/heap/Heap.cpp
File Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/405403003/diff/1/Source/platform/heap/Heap.cp...
Source/platform/heap/Heap.cpp:2092: // 4. trace objects reachable from the stack
"roots" including ephemerons.
On 2014/07/22 12:39:27, Erik Corry wrote:
> This will always do at least one run through the ephemerons even if there is
> nothing on the marking stack. That's correct, but it's only needed if at least
> one conservative pointer was found. Since we already have a flag for that, we
> could use it here to not bother calling processMarkingStack a second time.

Good idea. I guarded it by lastGCWasConservative().

https://codereview.chromium.org/405403003/diff/20001/Source/platform/heap/Thr...
File Source/platform/heap/ThreadState.cpp (left):

https://codereview.chromium.org/405403003/diff/20001/Source/platform/heap/Thr...
Source/platform/heap/ThreadState.cpp:467: // handles on other threads or
CrossThreadPersistents. The only cases where
On 2014/07/22 12:39:27, Erik Corry wrote:
> This comment was just deleted, but it looks helpful to me. Can it find a new
> home?

I too found it useful:) I did move it to CollectGarbageForTerminatingThread just
before calling the visitPersistents method (step 1).

[wibling-chromium, 2014-07-22 12:52:23]

The CQ bit was checked by wibling@chromium.org

[commit-bot: I haz the power, 2014-07-22 12:53:26]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/wibling@chromium.org/405403003/40001

[commit-bot: I haz the power, 2014-07-22 14:00:10]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/16535)
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[commit-bot: I haz the power, 2014-07-22 15:07:35]

Change committed as 178664

[johnme, 2014-07-22 16:25:52]

A revert of this CL has been created in
https://codereview.chromium.org/409183002/ by johnme@chromium.org.

The reason for reverting is: Broke blink_heap_unittests on
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Android%20%28Ne...

Specifically, HeapTest.CrossThreadPointerToOrphanedPage fails with:

../../third_party/WebKit/Source/platform/heap/HeapTest.cpp:4685: Failure
Value of: CrossThreadObject::s_destructorCalls
  Actual: 1
Expected: 0.