heap: make array buffer maps disjoint

heap: make array buffer maps disjoint

Remove intersection from the `std::map`s representing current live
ArrayBuffers. While being simpler to understand, it poses significant
performance issue for the active ArrayBuffer users (like node.js).

Store buffers separately, and process them together during mark-sweep
phase.

BUG=
R=mlippautz@chromium.org

Committed: https://crrev.com/15a0ace533f5810bf87382decbbaf4799f6cac8e
Cr-Commit-Position: refs/heads/master@{#30539}

[fedor.indutny, 2015-09-01 10:02:23]

See: https://codereview.chromium.org/1316873004/ for previous details.

Michael, I'm going to fix it tomorrow. Will push a fix here as soon as possible.

Thanks!

[fedor.indutny, 2015-09-01 18:24:15]

Michael,

Just pushed out the update. Looks like this change should be enough to make the
not_yet_discovered_..._for_scavenge list up-to-date.

Do you think there could be a way to test it reliably? Is there any way to
trigger Scavenge during Mark-Compact manually?

Cheers!

[fedor.indutny, 2015-09-01 18:37:38]

The other thing that makes me worried is that it registers the buffers twice
now. It could probably be worked around by registering only those buffers that
live in the Old Space, leaving the New Space to the Evacuate stage.

[fedor.indutny, 2015-09-01 18:53:45]

I think I have addressed the last concern about marking. Explicitly checking the
space where we are registering the array buffers sounds like a good solution for
this.

Should we employ the similar check in `heap.cc` too? What is cheaper - check the
space the object belongs to, or do `std::map()->erase()`?

[fedor.indutny, 2015-09-02 06:14:06]

Michael,

I have pushed one more update, which removes extra `erase()` call.

[Michael Lippautz, 2015-09-02 06:16:14]

Will do the proper review soon. Thanks for hanging onto this!

https://codereview.chromium.org/1326613002/diff/60001/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/1326613002/diff/60001/src/heap/heap.cc#newcod...
src/heap/heap.cc:1774: not_yet_discovered_buffers->erase(data);
Adjust the amount of recorded external memory.

[fedor.indutny, 2015-09-02 06:25:47]

Done.

https://codereview.chromium.org/1326613002/diff/60001/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/1326613002/diff/60001/src/heap/heap.cc#newcod...
src/heap/heap.cc:1774: not_yet_discovered_buffers->erase(data);
On 2015/09/02 06:16:14, Michael Lippautz wrote:
> Adjust the amount of recorded external memory.

Acknowledged.

[Michael Lippautz, 2015-09-02 07:23:00]

We're getting there.

https://codereview.chromium.org/1326613002/diff/80001/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/1326613002/diff/80001/src/heap/heap.cc#newcod...
src/heap/heap.cc:1778: reinterpret_cast<v8::Isolate*>(isolate_)
amount_of_external_allocated_memory_ -= length;

https://codereview.chromium.org/1326613002/diff/80001/src/heap/mark-compact.cc
File src/heap/mark-compact.cc (right):

https://codereview.chromium.org/1326613002/diff/80001/src/heap/mark-compact.c...
src/heap/mark-compact.cc:1870: // If we end up needing more special cases, we
should factor this out.
Remove this comment.

https://codereview.chromium.org/1326613002/diff/80001/src/heap/mark-compact.c...
src/heap/mark-compact.cc:4443: // will be erroneously freed.
Replace to:

EvacuateNewSpaceAndCandidates iterates over new space objects and for
ArrayBuffers either re-registers them as live or promotes them. This is needed
to properly free them.

https://codereview.chromium.org/1326613002/diff/80001/src/heap/objects-visiti...
File src/heap/objects-visiting-inl.h (right):

https://codereview.chromium.org/1326613002/diff/80001/src/heap/objects-visiti...
src/heap/objects-visiting-inl.h:538:
JSArrayBuffer::cast(object)->backing_store());
Can't we just call this as
  heap->RegisterLiveArrayBuffer(
      heap->InNewSpace(object), 
      JSArrayBuffer::cast(object)->backing_store()));
?

[fedor.indutny, 2015-09-02 08:56:08]

All fixed! Let's Land Zis Thing! ;)

Thanks!

https://codereview.chromium.org/1326613002/diff/80001/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/1326613002/diff/80001/src/heap/heap.cc#newcod...
src/heap/heap.cc:1778: reinterpret_cast<v8::Isolate*>(isolate_)
On 2015/09/02 07:22:59, Michael Lippautz wrote:
> amount_of_external_allocated_memory_ -= length;

Acknowledged.

https://codereview.chromium.org/1326613002/diff/80001/src/heap/mark-compact.cc
File src/heap/mark-compact.cc (right):

https://codereview.chromium.org/1326613002/diff/80001/src/heap/mark-compact.c...
src/heap/mark-compact.cc:1870: // If we end up needing more special cases, we
should factor this out.
On 2015/09/02 07:23:00, Michael Lippautz wrote:
> Remove this comment.

Acknowledged.

https://codereview.chromium.org/1326613002/diff/80001/src/heap/mark-compact.c...
src/heap/mark-compact.cc:4443: // will be erroneously freed.
On 2015/09/02 07:23:00, Michael Lippautz wrote:
> Replace to:
> 
> EvacuateNewSpaceAndCandidates iterates over new space objects and for
> ArrayBuffers either re-registers them as live or promotes them. This is needed
> to properly free them.

Acknowledged.

https://codereview.chromium.org/1326613002/diff/80001/src/heap/objects-visiti...
File src/heap/objects-visiting-inl.h (right):

https://codereview.chromium.org/1326613002/diff/80001/src/heap/objects-visiti...
src/heap/objects-visiting-inl.h:538:
JSArrayBuffer::cast(object)->backing_store());
On 2015/09/02 07:23:00, Michael Lippautz wrote:
> Can't we just call this as
>   heap->RegisterLiveArrayBuffer(
>       heap->InNewSpace(object), 
>       JSArrayBuffer::cast(object)->backing_store()));
> ?

Not really, the idea to have mark+evacuate register all buffers. Mark is going
to register old space buffers, evacuate - new space only.

[Michael Lippautz, 2015-09-02 09:11:06]

lgtm -- let's try again; since we lack test coverage in this area be prepared
that we might need to revert it

The next steps (if you would like to continue on this topic) would be 
- Tests! (cannot emphasize this enough)
- Refactoring out in a separate class that is not embedded
- RegisterLiveArrayBuffer -> MarkLiveArrayBuffer
- Make those methods taking JSArrayBuffer and figure out whether in_new_space is
true based on that.

[fedor.indutny, 2015-09-02 09:13:05]

The CQ bit was checked by fedor@indutny.com

[commit-bot: I haz the power, 2015-09-02 09:13:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1326613002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1326613002/100001

[commit-bot: I haz the power, 2015-09-02 09:17:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-02 09:17:02]

Try jobs failed on following builders:
  v8_android_arm_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...)

[fedor.indutny, 2015-09-02 09:22:28]

Michael,

Looks like there was some error about std::map not having ->at(). I have fixed
it by doing (*map)[] instead. Mind LGTMing it again?

Thanks!

[Michael Lippautz, 2015-09-02 09:24:16]

lgtm

l

[fedor.indutny, 2015-09-02 09:39:33]

The CQ bit was checked by fedor@indutny.com

[commit-bot: I haz the power, 2015-09-02 09:39:38]

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

[commit-bot: I haz the power, 2015-09-02 10:05:02]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-09-02 10:05:22]

Patchset 7 (id:??) landed as
https://crrev.com/15a0ace533f5810bf87382decbbaf4799f6cac8e
Cr-Commit-Position: refs/heads/master@{#30539}