Skip write barriers when updating the weak hash table.

Skip write barriers when updating the weak hash table.

Write barrier on the weak hash table makes all its pointers strong,
which can cause a memory leak.

BUG=359401
LOG=Y
TEST=cctest/test-heap/NoWeakHashTableLeakWithIncrementalMarking
R=hpayer@chromium.org, mstarzinger@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=21299

[ulan, 2014-05-13 17:16:33]

PTAL. This is the quick fix with regression test we discussed.

[Hannes Payer (out of office), 2014-05-14 06:47:45]

lgtm with nits

https://codereview.chromium.org/284773004/diff/10001/test/cctest/test-heap.cc
File test/cctest/test-heap.cc (right):

https://codereview.chromium.org/284773004/diff/10001/test/cctest/test-heap.cc...
test/cctest/test-heap.cc:3907: i::FLAG_compilation_cache = false;
why i::FLAG_compilation_cache = false; ?

https://codereview.chromium.org/284773004/diff/10001/test/cctest/test-heap.cc...
test/cctest/test-heap.cc:3932: StartIncrementalMarking();
Why do we start incremental marking again here? We started it above.

[ulan, 2014-05-14 07:27:09]

https://codereview.chromium.org/284773004/diff/10001/test/cctest/test-heap.cc
File test/cctest/test-heap.cc (right):

https://codereview.chromium.org/284773004/diff/10001/test/cctest/test-heap.cc...
test/cctest/test-heap.cc:3907: i::FLAG_compilation_cache = false;
On 2014/05/14 06:47:45, Hannes Payer wrote:
> why i::FLAG_compilation_cache = false; ?
To avoid caching the generated code below. So that nothing keeps code objects
alive and it dies as soon as we leave the local scope.

https://codereview.chromium.org/284773004/diff/10001/test/cctest/test-heap.cc...
test/cctest/test-heap.cc:3932: StartIncrementalMarking();
On 2014/05/14 06:47:45, Hannes Payer wrote:
> Why do we start incremental marking again here? We started it above.
This functions starts incremental marking if it is not active.

The JS call above can trigger GC, we might need to restart marking here,
otherwise CompleteIncrementalMarking will fail.

[Michael Starzinger, 2014-05-14 08:38:29]

Looking good.

https://codereview.chromium.org/284773004/diff/10001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/284773004/diff/10001/src/objects.cc#newcode16171
src/objects.cc:16171: table->set(EntryToValueIndex(entry), *value,
SKIP_WRITE_BARRIER);
nit: Can we add a TODO here that skipping the WB is a temporary workaround?

https://codereview.chromium.org/284773004/diff/10001/src/objects.cc#newcode16187
src/objects.cc:16187: set(EntryToIndex(entry), *key, SKIP_WRITE_BARRIER);
nit: Likewise.

https://codereview.chromium.org/284773004/diff/10001/test/cctest/test-heap.cc
File test/cctest/test-heap.cc (right):

https://codereview.chromium.org/284773004/diff/10001/test/cctest/test-heap.cc...
test/cctest/test-heap.cc:3915: StartIncrementalMarking();
I don't fully understand the reason for splitting SimuateIncrementalMarking.
Would it have the same effect to call SimulateIncrementalMarking here (before
the compilation) and below (after the compilation)? It should have the same
semantics AFAICT. If my assumption holds, then let's undo the split.

[ulan, 2014-05-14 08:54:04]

https://codereview.chromium.org/284773004/diff/10001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/284773004/diff/10001/src/objects.cc#newcode16171
src/objects.cc:16171: table->set(EntryToValueIndex(entry), *value,
SKIP_WRITE_BARRIER);
On 2014/05/14 08:38:30, Michael Starzinger wrote:
> nit: Can we add a TODO here that skipping the WB is a temporary workaround?

Done.

https://codereview.chromium.org/284773004/diff/10001/src/objects.cc#newcode16187
src/objects.cc:16187: set(EntryToIndex(entry), *key, SKIP_WRITE_BARRIER);
On 2014/05/14 08:38:30, Michael Starzinger wrote:
> nit: Likewise.

Done.

https://codereview.chromium.org/284773004/diff/10001/test/cctest/test-heap.cc
File test/cctest/test-heap.cc (right):

https://codereview.chromium.org/284773004/diff/10001/test/cctest/test-heap.cc...
test/cctest/test-heap.cc:3915: StartIncrementalMarking();
On 2014/05/14 08:38:30, Michael Starzinger wrote:
> I don't fully understand the reason for splitting SimuateIncrementalMarking.
> Would it have the same effect to call SimulateIncrementalMarking here (before
> the compilation) and below (after the compilation)? It should have the same
> semantics AFAICT. If my assumption holds, then let's undo the split.
You're right! Calling SimulateIncrementalMarking at the beginning is sufficient
to reproduce the leak. I reverted this part of the change.

[Michael Starzinger, 2014-05-14 09:00:15]

LGTM from my end.

[ulan, 2014-05-14 09:12:28]

Committed patchset #3 manually as r21299 (presubmit successful).