Oilpan: Implement a GC to take a heap snapshot

Oilpan: Implement a GC to take a heap snapshot

This CL implements Heap::collectGarbage(gcType = TakeSnapshot), which is intended
to be used to take a heap snapshot when onMemoryDump is requested etc.

The GC is expected to be as side-effect-free as possible. Thus the GC behaves as follows:

1) Run a marking task.
2) Take a heap snapshot (The actual implementation will come in a follow-up CL).
3) Clear all marks on marked objects. No sweeping task runs.

More contexts:
https://codereview.chromium.org/1149673002/
https://groups.google.com/a/chromium.org/d/topic/oilpan-reviews/6LhByzmVt6Q/discussion

BUG=490087
TEST=HeapTest.ThreadedWeakness

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

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

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

[haraken, 2015-05-27 08:15:06]

haraken@chromium.org changed reviewers:
+ keishi@chromium.org, oilpan-reviews@chromium.org, primiano@chromium.org,
ssid@chromium.org

[haraken, 2015-05-27 08:15:06]

PTAL

[keishi, 2015-05-27 08:45:55]

LGTM

[Primiano Tucci (use gerrit), 2015-05-27 08:56:41]

On 2015/05/27 08:15:06, haraken wrote:
> PTAL

So, let me see if I get the overall picture. With this change this will look as
follows:

- The MemoryInfra MemoryDumpManger will post a task to ssid's
BlinkGCMemoryDumpProvider::onMemoryDump, saying "dump now" on the main thread.
- BlinkGCMemoryDumpProvider will call Heap::collectGarbage(TakeSnapshot). This
call will take a while and will synchronously invoke the code that ssid will put
into ThreadState::takeSnapshot() and friends.
- ThreadState::takeSnapshot() will freeze all the other threads on safepoints,
and collect all the data for all the threads and their heaps.
- When ThreadState::takeSnapshot() returns we are guaranteed that ssid's code
will have be executed and populated whatever internal data structure, so the
BlinkGCMemoryDumpProvider can return that to the MemoryInfra manager.

Does all this make sense?

[haraken, 2015-05-27 09:21:28]

Thanks for review.

In the patch set 1, I implemented the snapshot GC so that it doesn't mark
unmarked objects dead but I begin to suspect if it is a correct optimization (it
may be correct but it's not clear). So I reverted the optimization in the patch
set 2. If the snapshot GC marks unmarked objects dead, it will have a side
effect on the program execution (because the snapshot GC produces dead objects)
but it is clearer that the snapshot GC doesn't violate the correctness. So I'm
planning to land the patch set 2, for safety.

> So, let me see if I get the overall picture. With this change this will look
as
> follows:
> 
> - The MemoryInfra MemoryDumpManger will post a task to ssid's
> BlinkGCMemoryDumpProvider::onMemoryDump, saying "dump now" on the main thread.
> - BlinkGCMemoryDumpProvider will call Heap::collectGarbage(TakeSnapshot). This
> call will take a while and will synchronously invoke the code that ssid will
put
> into ThreadState::takeSnapshot() and friends.
> - ThreadState::takeSnapshot() will freeze all the other threads on safepoints,
> and collect all the data for all the threads and their heaps.
> - When ThreadState::takeSnapshot() returns we are guaranteed that ssid's code
> will have be executed and populated whatever internal data structure, so the
> BlinkGCMemoryDumpProvider can return that to the MemoryInfra manager.
> 
> Does all this make sense?

Mostly. Heap::collectGarbage(TakeSnapshot) (not ThreadState::takeSnapshot()) is
the guy that stops all other threads on safe points. ThreadState::takeSnapshot()
for all ThreadStates are called on the GCing thread. When
Heap::collectGarbage(TakeSnapshot) returns, it is guaranteed that all the
ThreadState::takeSnapshot() has run (on the GCing thread).

[Primiano Tucci (use gerrit), 2015-05-27 09:44:38]

> Mostly. Heap::collectGarbage(TakeSnapshot) (not ThreadState::takeSnapshot())
Yeah sorry I meant Heap...

> the guy that stops all other threads on safe points.
ThreadState::takeSnapshot()
> for all ThreadStates are called on the GCing thread. When
> Heap::collectGarbage(TakeSnapshot) returns, it is guaranteed that all the
> ThreadState::takeSnapshot() has run (on the GCing thread).

Where "the GCing thread" is the one that invoked
Heap::collectGarbage(TakeSnapshot) in the first place, right?
If so LGTM from the MemoryInfra dumping perspective, I think I see all this
working in my mind :)

[haraken, 2015-05-27 10:28:21]

On 2015/05/27 09:44:38, Primiano Tucci wrote:
> > Mostly. Heap::collectGarbage(TakeSnapshot) (not ThreadState::takeSnapshot())
> Yeah sorry I meant Heap...
> 
> > the guy that stops all other threads on safe points.
> ThreadState::takeSnapshot()
> > for all ThreadStates are called on the GCing thread. When
> > Heap::collectGarbage(TakeSnapshot) returns, it is guaranteed that all the
> > ThreadState::takeSnapshot() has run (on the GCing thread).
> 
> Where "the GCing thread" is the one that invoked
> Heap::collectGarbage(TakeSnapshot) in the first place, right?

Exactly.

[haraken, 2015-05-27 10:28:25]

The CQ bit was checked by haraken@chromium.org

[haraken, 2015-05-27 10:28:26]

The patchset sent to the CQ was uploaded after l-g-t-m from keishi@chromium.org
Link to the patchset: https://codereview.chromium.org/1159773004/#ps20001
(title: " ")

[commit-bot: I haz the power, 2015-05-27 10:28:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1159773004/20001

[ssid, 2015-05-27 12:43:47]

Sorry couldn't get to this earlier.

https://codereview.chromium.org/1159773004/diff/20001/Source/platform/heap/Th...
File Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/1159773004/diff/20001/Source/platform/heap/Th...
Source/platform/heap/ThreadState.cpp:889: takeSnapshot();
Sorry, I just realized. This takeSnapshot() should be called before
prepareForSweep().
Also, should prepareForSweep() be called for a GC without sweeping? I see this
method moves pages to other list. Will this be undone?
In any case, i think this should be before the call.

[haraken, 2015-05-27 13:48:11]

https://codereview.chromium.org/1159773004/diff/20001/Source/platform/heap/Th...
File Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/1159773004/diff/20001/Source/platform/heap/Th...
Source/platform/heap/ThreadState.cpp:889: takeSnapshot();
On 2015/05/27 12:43:47, ssid wrote:
> Sorry, I just realized. This takeSnapshot() should be called before
> prepareForSweep().
> Also, should prepareForSweep() be called for a GC without sweeping? I see this
> method moves pages to other list. Will this be undone?
> In any case, i think this should be before the call.

prepareForSweep() needs to be called, because the following
makeConsistentForSweeping() works only on the list of unswept pages. If we don't
call prepareForSweep(), all pages are in the list of swept pages and thus we'll
fail at unmarking the marked objects etc.

It is possible to move prepareForSweep() to after takeSnapshot(), but it won't
matter much. If we don't move it, takeSnapshot() needs to scan the list of
unswept pages. If we move it, takeSnapshot() needs to scan the list of swept
pages. That's the only difference, I guess.

[ssid, 2015-05-27 13:56:05]

On 2015/05/27 13:48:11, haraken wrote:
>
https://codereview.chromium.org/1159773004/diff/20001/Source/platform/heap/Th...
> File Source/platform/heap/ThreadState.cpp (right):
> 
>
https://codereview.chromium.org/1159773004/diff/20001/Source/platform/heap/Th...
> Source/platform/heap/ThreadState.cpp:889: takeSnapshot();
> On 2015/05/27 12:43:47, ssid wrote:
> > Sorry, I just realized. This takeSnapshot() should be called before
> > prepareForSweep().
> > Also, should prepareForSweep() be called for a GC without sweeping? I see
this
> > method moves pages to other list. Will this be undone?
> > In any case, i think this should be before the call.
> 
> prepareForSweep() needs to be called, because the following
> makeConsistentForSweeping() works only on the list of unswept pages. If we
don't
> call prepareForSweep(), all pages are in the list of swept pages and thus
we'll
> fail at unmarking the marked objects etc.
> 
> It is possible to move prepareForSweep() to after takeSnapshot(), but it won't
> matter much. If we don't move it, takeSnapshot() needs to scan the list of
> unswept pages. If we move it, takeSnapshot() needs to scan the list of swept
> pages. That's the only difference, I guess.

I am trying to run a trace using the new gc(takeSnapshot). If the takeSnapshot()
is called after prepareForSweep(), the trace shows all heaps to have 0 pages.
But if i take snapshot before prepareForSweep() then I get pages and objects in
each page. Am I missing something?

[haraken, 2015-05-27 14:14:23]

On 2015/05/27 13:56:05, ssid wrote:
> On 2015/05/27 13:48:11, haraken wrote:
> >
>
https://codereview.chromium.org/1159773004/diff/20001/Source/platform/heap/Th...
> > File Source/platform/heap/ThreadState.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1159773004/diff/20001/Source/platform/heap/Th...
> > Source/platform/heap/ThreadState.cpp:889: takeSnapshot();
> > On 2015/05/27 12:43:47, ssid wrote:
> > > Sorry, I just realized. This takeSnapshot() should be called before
> > > prepareForSweep().
> > > Also, should prepareForSweep() be called for a GC without sweeping? I see
> this
> > > method moves pages to other list. Will this be undone?
> > > In any case, i think this should be before the call.
> > 
> > prepareForSweep() needs to be called, because the following
> > makeConsistentForSweeping() works only on the list of unswept pages. If we
> don't
> > call prepareForSweep(), all pages are in the list of swept pages and thus
> we'll
> > fail at unmarking the marked objects etc.
> > 
> > It is possible to move prepareForSweep() to after takeSnapshot(), but it
won't
> > matter much. If we don't move it, takeSnapshot() needs to scan the list of
> > unswept pages. If we move it, takeSnapshot() needs to scan the list of swept
> > pages. That's the only difference, I guess.
> 
> I am trying to run a trace using the new gc(takeSnapshot). If the
takeSnapshot()
> is called after prepareForSweep(), the trace shows all heaps to have 0 pages.
> But if i take snapshot before prepareForSweep() then I get pages and objects
in
> each page. Am I missing something?

Are you scanning pages in BaseHeap::m_unsweptFirstPage? After calling
prepareForSweep(), pages will be linked on BaseHeap::m_unsweptFirstPage, not
BaseHeap::m_firstPage.

[ssid, 2015-05-27 14:28:13]

On 2015/05/27 14:14:23, haraken wrote:
> On 2015/05/27 13:56:05, ssid wrote:
> > On 2015/05/27 13:48:11, haraken wrote:
> > >
> >
>
https://codereview.chromium.org/1159773004/diff/20001/Source/platform/heap/Th...
> > > File Source/platform/heap/ThreadState.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1159773004/diff/20001/Source/platform/heap/Th...
> > > Source/platform/heap/ThreadState.cpp:889: takeSnapshot();
> > > On 2015/05/27 12:43:47, ssid wrote:
> > > > Sorry, I just realized. This takeSnapshot() should be called before
> > > > prepareForSweep().
> > > > Also, should prepareForSweep() be called for a GC without sweeping? I
see
> > this
> > > > method moves pages to other list. Will this be undone?
> > > > In any case, i think this should be before the call.
> > > 
> > > prepareForSweep() needs to be called, because the following
> > > makeConsistentForSweeping() works only on the list of unswept pages. If we
> > don't
> > > call prepareForSweep(), all pages are in the list of swept pages and thus
> > we'll
> > > fail at unmarking the marked objects etc.
> > > 
> > > It is possible to move prepareForSweep() to after takeSnapshot(), but it
> won't
> > > matter much. If we don't move it, takeSnapshot() needs to scan the list of
> > > unswept pages. If we move it, takeSnapshot() needs to scan the list of
swept
> > > pages. That's the only difference, I guess.
> > 
> > I am trying to run a trace using the new gc(takeSnapshot). If the
> takeSnapshot()
> > is called after prepareForSweep(), the trace shows all heaps to have 0
pages.
> > But if i take snapshot before prepareForSweep() then I get pages and objects
> in
> > each page. Am I missing something?
> 
> Are you scanning pages in BaseHeap::m_unsweptFirstPage? After calling
> prepareForSweep(), pages will be linked on BaseHeap::m_unsweptFirstPage, not
> BaseHeap::m_firstPage.

Yes. Works now, thanks.

[commit-bot: I haz the power, 2015-05-27 15:07:47]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=195982

[dmurph, 2015-05-27 19:10:17]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/1159003003/ by dmurph@chromium.org.

The reason for reverting is: Breaking Nexus 4 tests,
https://code.google.com/p/chromium/issues/detail?id=492776.

[haraken, 2015-05-28 03:33:30]

On 2015/05/27 19:10:17, dmurph wrote:
> A revert of this CL (patchset #2 id:20001) has been created in
> https://codereview.chromium.org/1159003003/ by mailto:dmurph@chromium.org.
> 
> The reason for reverting is: Breaking Nexus 4 tests,
> https://code.google.com/p/chromium/issues/detail?id=492776.

I noticed that the test is timing out on Nexus 4. I'll reduce the number of
allocations in the test and reland.

[haraken, 2015-05-28 03:36:40]

The CQ bit was checked by haraken@chromium.org

[haraken, 2015-05-28 03:36:41]

The patchset sent to the CQ was uploaded after l-g-t-m from keishi@chromium.org,
primiano@chromium.org
Link to the patchset: https://codereview.chromium.org/1159773004/#ps40001
(title: " ")

[commit-bot: I haz the power, 2015-05-28 03:37:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1159773004/40001

[haraken, 2015-05-28 03:40:37]

The CQ bit was unchecked by haraken@chromium.org

[haraken, 2015-05-28 03:51:44]

...and I noticed one bug of our GC infrastructure. Let me fix it before landing
this CL.

[haraken, 2015-05-28 05:41:02]

On 2015/05/28 03:51:44, haraken wrote:
> ...and I noticed one bug of our GC infrastructure. Let me fix it before
landing
> this CL.

Sorry about a lot of confusions. I think the patch set 6 fixes all the issues we
observed around the snapshot GC. To make the patch size smaller, I'll land
https://codereview.chromium.org/1162533003/ first.

[haraken, 2015-05-28 11:45:07]

I think this is now ready for review.

keishi-san: PTAL again?

[keishi, 2015-05-28 11:47:08]

LGTM

[haraken, 2015-05-28 11:47:20]

The CQ bit was checked by haraken@chromium.org

[haraken, 2015-05-28 11:47:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org
Link to the patchset: https://codereview.chromium.org/1159773004/#ps120001
(title: " ")

[commit-bot: I haz the power, 2015-05-28 11:47:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1159773004/120001

[sof, 2015-05-28 12:15:11]

sigbjornf@opera.com changed reviewers:
+ sigbjornf@opera.com

[sof, 2015-05-28 12:15:13]

https://codereview.chromium.org/1159773004/diff/120001/Source/platform/heap/T...
File Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/1159773004/diff/120001/Source/platform/heap/T...
Source/platform/heap/ThreadState.cpp:907: m_gcState = NoGCScheduled;
Why are you doing this (way)?

[haraken, 2015-05-28 12:33:37]

https://codereview.chromium.org/1159773004/diff/120001/Source/platform/heap/T...
File Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/1159773004/diff/120001/Source/platform/heap/T...
Source/platform/heap/ThreadState.cpp:907: m_gcState = NoGCScheduled;
On 2015/05/28 12:15:13, sof wrote:
> Why are you doing this (way)?

As commented above, if we call setGCState(NoGCScheduled) then we hit an ASSERT
in checkThread(), because NoGCScheduled is expected to be set by each thread.
Here we're violating the rule.

One way to fix it would be to remove the checkThread().

Another way would be to introduce a NoSweepScheduled state so that preSweep()
(which is called in each thread) can call setGCState(NoGCScheduled). But we
won't want to introduce another GCState...

Any better idea?

[commit-bot: I haz the power, 2015-05-28 13:09:52]

Committed patchset #7 (id:120001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=196059

[sof, 2015-05-28 14:11:15]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/1143243006/ by sigbjornf@opera.com.

The reason for reverting is: ThreadedWeakness unit test is failing on various
bots,


http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%2032/buil...

http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Android%20%28Ne....

[haraken, 2015-05-29 00:22:51]

On 2015/05/28 14:11:15, sof wrote:
> A revert of this CL (patchset #7 id:120001) has been created in
> https://codereview.chromium.org/1143243006/ by mailto:sigbjornf@opera.com.
> 
> The reason for reverting is: ThreadedWeakness unit test is failing on various
> bots,
> 
> 
>
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%2032/buil...
> 
>
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Android%20%28Ne....

Thanks for the revert!

I cannot reproduce the crash locally, but I think that is because we're running
weak processings in the snapshot GC.

[haraken, 2015-05-29 01:27:03]

Updated the CL so that we don't register weak callbacks when taking a snapshot.
Running the oilpan bots.

[sof, 2015-05-29 12:07:33]

On 2015/05/29 01:27:03, haraken wrote:
> Updated the CL so that we don't register weak callbacks when taking a
snapshot.
> Running the oilpan bots.

Rebase & kick off again?

[haraken, 2015-05-29 12:40:49]

On 2015/05/29 12:07:33, sof wrote:
> On 2015/05/29 01:27:03, haraken wrote:
> > Updated the CL so that we don't register weak callbacks when taking a
> snapshot.
> > Running the oilpan bots.
> 
> Rebase & kick off again?

Actually it's not yet correct. I need to land
https://codereview.chromium.org/1160143002/ first :)

[sof, 2015-05-29 12:59:12]

On 2015/05/29 12:40:49, haraken wrote:
> On 2015/05/29 12:07:33, sof wrote:
> > On 2015/05/29 01:27:03, haraken wrote:
> > > Updated the CL so that we don't register weak callbacks when taking a
> > snapshot.
> > > Running the oilpan bots.
> > 
> > Rebase & kick off again?
> 
> Actually it's not yet correct. I need to land
> https://codereview.chromium.org/1160143002/ first :)

I see - they're all piling up by now. :-)

Let's try to land that independently of the one it was associated; looks doable.
i.e., I'd prefer "Canary baking" ToT's setGCState() related changes over the
weekend before we continue apace with improvements.

[sof, 2015-05-30 20:50:06]

https://codereview.chromium.org/1159773004/diff/140001/Source/platform/heap/H...
File Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/1159773004/diff/140001/Source/platform/heap/H...
Source/platform/heap/Heap.cpp:1887: if (GCScope::current()->gcType() ==
ThreadState::TakeSnapshot)
Did you explore providing MarkingVisitor<GlobalMarkingNoWeak> (a new MarkingMode
corresponding to what TakeSnapshot is after) which override
registerWeakMembers() and registerWeakCellWithCallback(), but otherwise behave
like MarkingVisitor<GlobalMarking> ?

[sof, 2015-05-30 20:50:06]

https://codereview.chromium.org/1159773004/diff/140001/Source/platform/heap/H...
File Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/1159773004/diff/140001/Source/platform/heap/H...
Source/platform/heap/Heap.cpp:1887: if (GCScope::current()->gcType() ==
ThreadState::TakeSnapshot)
Did you explore providing MarkingVisitor<GlobalMarkingNoWeak> (a new MarkingMode
corresponding to what TakeSnapshot is after) which override
registerWeakMembers() and registerWeakCellWithCallback(), but otherwise behave
like MarkingVisitor<GlobalMarking> ?

[haraken, 2015-05-31 00:06:45]

On 2015/05/30 20:50:06, sof wrote:
>
https://codereview.chromium.org/1159773004/diff/140001/Source/platform/heap/H...
> File Source/platform/heap/Heap.cpp (right):
> 
>
https://codereview.chromium.org/1159773004/diff/140001/Source/platform/heap/H...
> Source/platform/heap/Heap.cpp:1887: if (GCScope::current()->gcType() ==
> ThreadState::TakeSnapshot)
> Did you explore providing MarkingVisitor<GlobalMarkingNoWeak> (a new
MarkingMode
> corresponding to what TakeSnapshot is after) which override
> registerWeakMembers() and registerWeakCellWithCallback(), but otherwise behave
> like MarkingVisitor<GlobalMarking> ?

That is a great suggestion.

[haraken, 2015-06-01 05:30:40]

On 2015/05/31 00:06:45, haraken wrote:
> On 2015/05/30 20:50:06, sof wrote:
> >
>
https://codereview.chromium.org/1159773004/diff/140001/Source/platform/heap/H...
> > File Source/platform/heap/Heap.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1159773004/diff/140001/Source/platform/heap/H...
> > Source/platform/heap/Heap.cpp:1887: if (GCScope::current()->gcType() ==
> > ThreadState::TakeSnapshot)
> > Did you explore providing MarkingVisitor<GlobalMarkingNoWeak> (a new
> MarkingMode
> > corresponding to what TakeSnapshot is after) which override
> > registerWeakMembers() and registerWeakCellWithCallback(), but otherwise
behave
> > like MarkingVisitor<GlobalMarking> ?
> 
> That is a great suggestion.

I noticed that this requires a certain amount of refactoring (which is worth
doing and thus I'm planning to land patches from now).

The problem is that a bunch of code is using s_markingVisitor (which is a
visitor for GlobalMarking). We need to remove the global variable first, because
the visitor we're using is no longer guaranteed to be a global marking visitor.
It can be a thread-local marking visitor or a snapshot marking visitor. (Note:
We already have a bug here -- we're currently running thread-local weak
processing assuming that the visitor is a global marking visitor. But it can be
a thread-local marking visitor.)

In short, I'm planning to remove the global variable first:
Heap::s_markingVisitor.

[haraken, 2015-06-08 00:34:36]

I think the patch set 11 is finally ready for landing. Testing the bots.

[Primiano Tucci (use gerrit), 2015-06-08 01:29:16]

On 2015/06/08 00:34:36, haraken wrote:
> I think the patch set 11 is finally ready for landing. Testing the bots.

\o/

[haraken, 2015-06-08 02:06:42]

The CQ bit was checked by haraken@chromium.org

[haraken, 2015-06-08 02:06:43]

The patchset sent to the CQ was uploaded after l-g-t-m from keishi@chromium.org,
primiano@chromium.org
Link to the patchset: https://codereview.chromium.org/1159773004/#ps200001
(title: " ")

[commit-bot: I haz the power, 2015-06-08 02:06:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1159773004/200001

[commit-bot: I haz the power, 2015-06-08 02:09:42]

Committed patchset #11 (id:200001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=196643

[sof, 2015-06-08 07:37:22]

Unit test still failing on trunk after this one landing afaict,


http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.8/builds/...

[haraken, 2015-06-08 07:50:32]

On 2015/06/08 07:37:22, sof wrote:
> Unit test still failing on trunk after this one landing afaict,
> 
> 
>
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.8/builds/...

Hmm, I couldn't reproduce the failure locally. Also the failure is not observed
in the oilpan try bots... Looking.

[haraken, 2015-06-08 08:25:10]

On 2015/06/08 07:50:32, haraken wrote:
> On 2015/06/08 07:37:22, sof wrote:
> > Unit test still failing on trunk after this one landing afaict,
> > 
> > 
> >
>
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.8/builds/...
> 
> Hmm, I couldn't reproduce the failure locally. Also the failure is not
observed
> in the oilpan try bots... Looking.

I increased the number of snapshot GCs and ran the test a lot of times, but
cannot reproduce the crash locally. The crash indicates that thread-local weak
processing for HashTables is running in a different thread than the thread that
allocated the HashTables. It shouldn't happen...

At the moment, let me suppress the test failure to make the bot green.

[haraken, 2015-06-08 08:53:58]

On 2015/06/08 08:25:10, haraken wrote:
> On 2015/06/08 07:50:32, haraken wrote:
> > On 2015/06/08 07:37:22, sof wrote:
> > > Unit test still failing on trunk after this one landing afaict,
> > > 
> > > 
> > >
> >
>
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.8/builds/...
> > 
> > Hmm, I couldn't reproduce the failure locally. Also the failure is not
> observed
> > in the oilpan try bots... Looking.
> 
> I increased the number of snapshot GCs and ran the test a lot of times, but
> cannot reproduce the crash locally. The crash indicates that thread-local weak
> processing for HashTables is running in a different thread than the thread
that
> allocated the HashTables. It shouldn't happen...
> 
> At the moment, let me suppress the test failure to make the bot green.

I was able to reproduce it in Release builds. Looking.

[haraken, 2015-06-12 09:09:21]

On 2015/06/08 08:53:58, haraken wrote:
> On 2015/06/08 08:25:10, haraken wrote:
> > On 2015/06/08 07:50:32, haraken wrote:
> > > On 2015/06/08 07:37:22, sof wrote:
> > > > Unit test still failing on trunk after this one landing afaict,
> > > > 
> > > > 
> > > >
> > >
> >
>
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.8/builds/...
> > > 
> > > Hmm, I couldn't reproduce the failure locally. Also the failure is not
> > observed
> > > in the oilpan try bots... Looking.
> > 
> > I increased the number of snapshot GCs and ran the test a lot of times, but
> > cannot reproduce the crash locally. The crash indicates that thread-local
weak
> > processing for HashTables is running in a different thread than the thread
> that
> > allocated the HashTables. It shouldn't happen...
> > 
> > At the moment, let me suppress the test failure to make the bot green.
> 
> I was able to reproduce it in Release builds. Looking.

I've been looking into the crash but haven't yet figured out the root cause. The
crash is happening in HashTable::swap in some complicated manner. Looking.

[haraken, 2015-06-15 02:38:09]

ssid@: I'm sorry about the delay. Please go ahead with landing your CLs for
memory-infra.

The snapshot GC is still causing a crash but the crash happens only in an
artificial scenario where multiple threads aggressively contend with allocating
HashTables that need weak processing. Given that that won't happen in practice
and I won't have time to fix it this week, it would be better to move things
forward. So please go ahead :) I'll fix the issue next week.