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.

The results of benchmarks are:

$ ./node-slow bench && ./node-fast bench
4997.4 ns/op
4685.7 ns/op

NOTE: `fast` - was a patched node.js, `slow` - unpatched node.js with vanilla v8.

BUG=

Committed: https://crrev.com/9e3676da9ab1aaf7de3e8582cb3fdefcc3dbaf33
Cr-Commit-Position: refs/heads/master@{#30495}

[indutny, 2015-08-26 23:55:01]

fedor.indutny@gmail.com changed reviewers:
+ fedor.indutny@gmail.com, mlippautz@chromium.org

[fedor.indutny, 2015-08-27 05:34:29]

I'm no longer sure, which account was used to submit this CL. Emailing from the
second one, just in case.

[Michael Lippautz, 2015-08-27 11:22:13]

mlippautz@chromium.org changed reviewers:
+ hpayer@chromium.org
- fedor.indutny@gmail.com, svenpanne@google.com

[Michael Lippautz, 2015-08-27 11:22:13]

-svenpanne, -indutny, +hpayer

Generally fine; 

How about factoring out the whole logic into an ArrayBufferTracker that is tied
to heap or isolate? The logic has no dependency on heap and as far as I see
there are only calls into isolate. The only call from heap is to
FreeDeadArrayBuffers.

Hannes, wdyt?

[fedor.indutny, 2015-08-27 18:38:37]

Thank you, Michael!

I don't mind moving it into a separate class. But maybe I could do it in
separate CL?

[Michael Lippautz, 2015-08-28 08:05:02]

Hej,

Thanks for highlighting hickups and taking the effort to improve on this! Let's
leave the refactoring in a separate class for another issue. 

Can you share a bit more on the bottleneck in node.js when this code is
involved? Any workloads or benchmarks you are tracking?

In any case, if tracking the buffers results in a performance issue we should
definitely take the effort of having a closer look. In general, the XXXHelper()
functions don't make much sense as they are either one liners, or hide
additional loops.

The FreeDeadArrayBuffers() method is in particular messy. During inlining I
found a way to simplify this a lot, in the end resulting only in a single loop
over not-yet-discovered-buffers (which is a different map depending on the
caller).

I think we should go this way as otherwise the logic is too complicate to see
what's going on.

https://codereview.chromium.org/1316873004/diff/1/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/1316873004/diff/1/src/heap/heap.cc#newcode1739
src/heap/heap.cc:1739: void Heap::RegisterNewArrayBufferHelper(std::map<void*,
size_t>& live_buffers,
Please get rid of this function and inline it into RegisterNewArrayBuffer.

https://codereview.chromium.org/1316873004/diff/1/src/heap/heap.cc#newcode1745
src/heap/heap.cc:1745: void Heap::UnregisterArrayBufferHelper(
Ditto, please inline the call manually.

https://codereview.chromium.org/1316873004/diff/1/src/heap/heap.cc#newcode1754
src/heap/heap.cc:1754: void Heap::RegisterLiveArrayBufferHelper(
Please get rid of this function and inline it into RegisterLiveArrayBuffer.

https://codereview.chromium.org/1316873004/diff/1/src/heap/heap.cc#newcode1760
src/heap/heap.cc:1760: size_t Heap::FreeDeadArrayBuffersHelper(
We should get rid of this one. See below.

https://codereview.chromium.org/1316873004/diff/1/src/heap/heap.cc#newcode1784
src/heap/heap.cc:1784: void Heap::TearDownArrayBuffersHelper(
Also inline please.

https://codereview.chromium.org/1316873004/diff/1/src/heap/heap.cc#newcode1834
src/heap/heap.cc:1834: void Heap::FreeDeadArrayBuffers(bool from_scavenge) {
The logic here is way too complicated as the semantic is to maintain two
distinct sets for array buffers.

I inlined, reshuffled and consolidated the calls. I think we should get rid of
the helper method as the actual logic seems to be pretty simple. 

Please check the simplification. I am using .erase() with buffers (there exists
an overload that takes a map). The loops (over maps) get unfolded into a single
loop in the last step.


======== Step 0 (baseline; inlined FreeDeadArrayBuffersHelper call from patch)
======== 

FreeDeadArrayBuffers(from_scavenge):
  if from_scavenge:
   
not_yet_discovered_array_buffers_.erase(not_yet_discovered_array_buffers_for_scavenge_)
  else:
    live_array_buffers_for_scavenge_.erase(not_yet_discovered_array_buffers_)

  # inlined FreeDeadArrayBuffersHelper
  # parameters are unfolded here
  if from_scavenge:
    not_yet_discovered_buffers = not_yet_discovered_array_buffers_for_scavenge_
  else:
    not_yet_discovered_buffers = not_yet_discovered_array_buffers_

  for buffer in not_yet_discovered_buffers:
    isolate.free_array_buffer(buffer)
    if (!from_scavenge):
      live_array_buffers_.erase(buffer)
    live_array_buffers_for_scavenge_.erase(buffer)

  if (from_scavenge):
    not_yet_discovered_buffers = live_array_buffers_for_scavenge_;
  else:
    not_yet_discovered_buffers = live_array_buffers_;
    not_yet_discovered_buffers.insert(live_array_buffers_for_scavenge.begin(),
...end())


======== Step 1 (shuffle inlined stuff around) ======== 

FreeDeadArrayBuffers(from_scavenge):
  if from_scavenge:
   
not_yet_discovered_array_buffers_.erase(not_yet_discovered_array_buffers_for_scavenge_)
    not_yet_discovered_buffers = not_yet_discovered_array_buffers_for_scavenge_
    live_array_buffers_for_scavenge_.erase(not_yet_discovered_buffers)
  else:
    live_array_buffers_for_scavenge_.erase(not_yet_discovered_array_buffers_)
    not_yet_discovered_buffers = not_yet_discovered_array_buffers_
    live_array_buffers_for_scavenge_.erase(not_yet_discovered_buffers)
    live_array_buffers_.erase(not_yet_discovered_buffers)

  # rest of inlined FreeDeadArrayBuffersHelper

  for buffer in not_yet_discovered_buffers:
    isolate.free_array_buffer(buffer)

  if (from_scavenge):
    not_yet_discovered_buffers = live_array_buffers_for_scavenge_;
  else:
    not_yet_discovered_buffers = live_array_buffers_;
    not_yet_discovered_buffers.insert(live_array_buffers_for_scavenge_)


======== Step 2 (re-orde / remove duplicates) ======== 

FreeDeadArrayBuffers(from_scavenge):
  if from_scavenge:
    tmp_not_yet_discovered_buffers =
not_yet_discovered_array_buffers_for_scavenge_
    not_yet_discovered_array_buffers_.erase((tmp_not_yet_discovered_buffers)
    live_array_buffers_for_scavenge_.erase(tmp_not_yet_discovered_buffers)
  else:
    tmp_not_yet_discovered_buffers = not_yet_discovered_array_buffers_
    live_array_buffers_for_scavenge_.erase(tmp_not_yet_discovered_buffers)
    live_array_buffers_.erase(tmp_not_yet_discovered_buffers)

  # rest of inlined FreeDeadArrayBuffersHelper

  for buffer in tmp_not_yet_discovered_buffers:
    isolate.free_array_buffer(buffer)

  if (from_scavenge):
    not_yet_discovered_buffers = live_array_buffers_for_scavenge_;
  else:
    not_yet_discovered_buffers = live_array_buffers_;
    not_yet_discovered_buffers.insert(live_array_buffers_for_scavenge_)


======== Step 3 (consolidate into a single loop) ======== 

FreeDeadArrayBuffers(from_scavenge):
  if from_scavenge:
    tmp_not_yet_discovered_buffers =
not_yet_discovered_array_buffers_for_scavenge_
  else:
    tmp_not_yet_discovered_buffers = not_yet_discovered_array_buffers_

  for buffer in tmp_not_yet_discovered_buffers:
    isolate.free_array_buffer(buffer)
    live_array_buffers_for_scavenge_.erase(buffer)
    if (from_scavenge):
      not_yet_discovered_array_buffers_.erase(buffer)
    else:
      live_array_buffers_.erase(tmp_not_yet_discovered_buffers)

  if (from_scavenge):
    not_yet_discovered_buffers = live_array_buffers_for_scavenge_;
  else:
    not_yet_discovered_buffers = live_array_buffers_;
    not_yet_discovered_buffers.insert(live_array_buffers_for_scavenge_)

[fedor.indutny, 2015-08-28 09:48:40]

Hello again,

Thanks for the thorough review! Will fix everything mentioned as soon as
possible, hopefully today.

We see two bottlenecks in node.js: one happens during buffer allocation, one
during GC. Both add together and make things slightly slower than they could
potentially be. Here is a benchmark that we are using to figure out exact
numbers: https://gist.github.com/trevnorris/efe36274b0d0a23a78f5 .

This patch cuts down performance overhead by 15% according to Trevor Norris (it
was a bit bigger on my machine, so it probably fluctuates and depends on the
speed of memory allocation too).

Is there a reason why there is no API to have ArrayBuffer contents allocated
inline in the object itself? Not that we could solely use this API, but in many
cases it could potentially give us better performance, because of absence of
bookkeeping.

Thank you again,
Fedor.

[fedor.indutny, 2015-08-28 10:02:37]

https://codereview.chromium.org/1316873004/diff/1/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/1316873004/diff/1/src/heap/heap.cc#newcode1784
src/heap/heap.cc:1784: void Heap::TearDownArrayBuffersHelper(
On 2015/08/28 08:05:02, Michael Lippautz wrote:
> Also inline please.

This one is going to result in duplicated code, isn't it?

[Michael Lippautz, 2015-08-28 10:47:17]

On 2015/08/28 09:48:40, fedor.indutny wrote:
> Hello again,
> 
> Thanks for the thorough review! Will fix everything mentioned as soon as
> possible, hopefully today.
> 
> We see two bottlenecks in node.js: one happens during buffer allocation, one
> during GC. Both add together and make things slightly slower than they could
> potentially be. Here is a benchmark that we are using to figure out exact
> numbers: https://gist.github.com/trevnorris/efe36274b0d0a23a78f5 .
> 
> This patch cuts down performance overhead by 15% according to Trevor Norris
(it
> was a bit bigger on my machine, so it probably fluctuates and depends on the
> speed of memory allocation too).
> 

Ugh, a microbenchmark. Did you also see it in real-world applications?

The problem I see in general with this benchmark is that we allocate a lot of
memory without actually using it. Upon allocating a buffer this large you should
also use it (read/write), effectively dominating the bookkeeping overhead.

> Is there a reason why there is no API to have ArrayBuffer contents allocated
> inline in the object itself? Not that we could solely use this API, but in
many
> cases it could potentially give us better performance, because of absence of
> bookkeeping.

Usually objects contain room for some elements themselves and are backed by some
other store for larger sizes (and are thus tracked by the frontend object). As
far as I see the array buffer in question is already a backing store, so it
requires extra handling to be used stand alone. The reason why some store are
treated differently is often because of how memory can move between the embedder
and V8.

-Michael

[Michael Lippautz, 2015-08-28 10:48:10]

On 2015/08/28 10:02:37, fedor.indutny wrote:
> https://codereview.chromium.org/1316873004/diff/1/src/heap/heap.cc
> File src/heap/heap.cc (right):
> 
> https://codereview.chromium.org/1316873004/diff/1/src/heap/heap.cc#newcode1784
> src/heap/heap.cc:1784: void Heap::TearDownArrayBuffersHelper(
> On 2015/08/28 08:05:02, Michael Lippautz wrote:
> > Also inline please.
> 
> This one is going to result in duplicated code, isn't it?

Yes, I'd prefer two loops over two calls with single loops though. (Also we'd
get rid of all the helpers.)

[fedor.indutny, 2015-08-28 11:03:34]

Inlined everything, thank you again!

(Sorry, going to sleep now. Hope it looks good now ;) )

https://codereview.chromium.org/1316873004/diff/1/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/1316873004/diff/1/src/heap/heap.cc#newcode1739
src/heap/heap.cc:1739: void Heap::RegisterNewArrayBufferHelper(std::map<void*,
size_t>& live_buffers,
On 2015/08/28 08:05:02, Michael Lippautz wrote:
> Please get rid of this function and inline it into RegisterNewArrayBuffer.

Acknowledged.

https://codereview.chromium.org/1316873004/diff/1/src/heap/heap.cc#newcode1745
src/heap/heap.cc:1745: void Heap::UnregisterArrayBufferHelper(
On 2015/08/28 08:05:02, Michael Lippautz wrote:
> Ditto, please inline the call manually.

Acknowledged.

https://codereview.chromium.org/1316873004/diff/1/src/heap/heap.cc#newcode1754
src/heap/heap.cc:1754: void Heap::RegisterLiveArrayBufferHelper(
On 2015/08/28 08:05:02, Michael Lippautz wrote:
> Please get rid of this function and inline it into RegisterLiveArrayBuffer.

Acknowledged.

https://codereview.chromium.org/1316873004/diff/1/src/heap/heap.cc#newcode1760
src/heap/heap.cc:1760: size_t Heap::FreeDeadArrayBuffersHelper(
On 2015/08/28 08:05:02, Michael Lippautz wrote:
> We should get rid of this one. See below.

Acknowledged.

https://codereview.chromium.org/1316873004/diff/1/src/heap/heap.cc#newcode1784
src/heap/heap.cc:1784: void Heap::TearDownArrayBuffersHelper(
On 2015/08/28 08:05:02, Michael Lippautz wrote:
> Also inline please.

Acknowledged.

https://codereview.chromium.org/1316873004/diff/1/src/heap/heap.cc#newcode1834
src/heap/heap.cc:1834: void Heap::FreeDeadArrayBuffers(bool from_scavenge) {
On 2015/08/28 08:05:02, Michael Lippautz wrote:
> The logic here is way too complicated as the semantic is to maintain two
> distinct sets for array buffers.
> 
> I inlined, reshuffled and consolidated the calls. I think we should get rid of
> the helper method as the actual logic seems to be pretty simple. 
> 
> Please check the simplification. I am using .erase() with buffers (there
exists
> an overload that takes a map). The loops (over maps) get unfolded into a
single
> loop in the last step.
> 
> 
> ======== Step 0 (baseline; inlined FreeDeadArrayBuffersHelper call from patch)
> ======== 
> 
> FreeDeadArrayBuffers(from_scavenge):
>   if from_scavenge:
>    
>
not_yet_discovered_array_buffers_.erase(not_yet_discovered_array_buffers_for_scavenge_)
>   else:
>     live_array_buffers_for_scavenge_.erase(not_yet_discovered_array_buffers_)
> 
>   # inlined FreeDeadArrayBuffersHelper
>   # parameters are unfolded here
>   if from_scavenge:
>     not_yet_discovered_buffers =
not_yet_discovered_array_buffers_for_scavenge_
>   else:
>     not_yet_discovered_buffers = not_yet_discovered_array_buffers_
> 
>   for buffer in not_yet_discovered_buffers:
>     isolate.free_array_buffer(buffer)
>     if (!from_scavenge):
>       live_array_buffers_.erase(buffer)
>     live_array_buffers_for_scavenge_.erase(buffer)
> 
>   if (from_scavenge):
>     not_yet_discovered_buffers = live_array_buffers_for_scavenge_;
>   else:
>     not_yet_discovered_buffers = live_array_buffers_;
>     not_yet_discovered_buffers.insert(live_array_buffers_for_scavenge.begin(),
> ...end())
> 
> 
> ======== Step 1 (shuffle inlined stuff around) ======== 
> 
> FreeDeadArrayBuffers(from_scavenge):
>   if from_scavenge:
>    
>
not_yet_discovered_array_buffers_.erase(not_yet_discovered_array_buffers_for_scavenge_)
>     not_yet_discovered_buffers =
not_yet_discovered_array_buffers_for_scavenge_
>     live_array_buffers_for_scavenge_.erase(not_yet_discovered_buffers)
>   else:
>     live_array_buffers_for_scavenge_.erase(not_yet_discovered_array_buffers_)
>     not_yet_discovered_buffers = not_yet_discovered_array_buffers_
>     live_array_buffers_for_scavenge_.erase(not_yet_discovered_buffers)
>     live_array_buffers_.erase(not_yet_discovered_buffers)
> 
>   # rest of inlined FreeDeadArrayBuffersHelper
> 
>   for buffer in not_yet_discovered_buffers:
>     isolate.free_array_buffer(buffer)
> 
>   if (from_scavenge):
>     not_yet_discovered_buffers = live_array_buffers_for_scavenge_;
>   else:
>     not_yet_discovered_buffers = live_array_buffers_;
>     not_yet_discovered_buffers.insert(live_array_buffers_for_scavenge_)
> 
> 
> ======== Step 2 (re-orde / remove duplicates) ======== 
> 
> FreeDeadArrayBuffers(from_scavenge):
>   if from_scavenge:
>     tmp_not_yet_discovered_buffers =
> not_yet_discovered_array_buffers_for_scavenge_
>     not_yet_discovered_array_buffers_.erase((tmp_not_yet_discovered_buffers)
>     live_array_buffers_for_scavenge_.erase(tmp_not_yet_discovered_buffers)
>   else:
>     tmp_not_yet_discovered_buffers = not_yet_discovered_array_buffers_
>     live_array_buffers_for_scavenge_.erase(tmp_not_yet_discovered_buffers)
>     live_array_buffers_.erase(tmp_not_yet_discovered_buffers)
> 
>   # rest of inlined FreeDeadArrayBuffersHelper
> 
>   for buffer in tmp_not_yet_discovered_buffers:
>     isolate.free_array_buffer(buffer)
> 
>   if (from_scavenge):
>     not_yet_discovered_buffers = live_array_buffers_for_scavenge_;
>   else:
>     not_yet_discovered_buffers = live_array_buffers_;
>     not_yet_discovered_buffers.insert(live_array_buffers_for_scavenge_)
> 
> 
> ======== Step 3 (consolidate into a single loop) ======== 
> 
> FreeDeadArrayBuffers(from_scavenge):
>   if from_scavenge:
>     tmp_not_yet_discovered_buffers =
> not_yet_discovered_array_buffers_for_scavenge_
>   else:
>     tmp_not_yet_discovered_buffers = not_yet_discovered_array_buffers_
> 
>   for buffer in tmp_not_yet_discovered_buffers:
>     isolate.free_array_buffer(buffer)
>     live_array_buffers_for_scavenge_.erase(buffer)
>     if (from_scavenge):
>       not_yet_discovered_array_buffers_.erase(buffer)
>     else:
>       live_array_buffers_.erase(tmp_not_yet_discovered_buffers)
> 
>   if (from_scavenge):
>     not_yet_discovered_buffers = live_array_buffers_for_scavenge_;
>   else:
>     not_yet_discovered_buffers = live_array_buffers_;
>     not_yet_discovered_buffers.insert(live_array_buffers_for_scavenge_)
> 
> 
> 

Acknowledged.

[Michael Lippautz, 2015-08-28 14:25:31]

Fedor,

Getting there! Now we can actually see what's happening :)

The code will get much better (and I suspect much faster). The main idea is that
we only process XXX_scavenge_ maps during scavenges and everything during the
full GC.

Let me also note that V8’s new generation (scavenger) is a semi-space GC where
we copy once before we promote to old space. As a result we should not move
references from the scavenge map to the full map after the
FreeDeadArrayBuffers() call, but only on the PromoteArrayBuffer() call.

So what we really want is
  live_array_buffers_
  not_yet_discovered_array_buffers_
and
  live_array_buffers_for_scavenge_
  not_yet_discovered_array_for_buffers_scavenge_
to be completely disjoint.

Then we can process only XXX_scavenge_ during a scavenge and everything
(including XXX_scavenge_) during a full GC. Rest of the comments inlined.

Can you then post the results of the changed behavior vs tip-of-tree ?

Also, please remove Sven from the review field (R=).

Thanks a lot!

-Michael

https://codereview.chromium.org/1316873004/diff/20001/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/1316873004/diff/20001/src/heap/heap.cc#newcod...
src/heap/heap.cc:1752: void Heap::UnregisterArrayBuffer(bool in_new_space, void*
data) {
We can simplify the main part a lot:

std::map<void*, size_t>*live_buffers = 
    in_new_space ? &live_array_buffers_for_scavenge_ : &live_array_buffers_;
std::map<void*, size_t>* not_yet_discovered_buffers = 
    in_new_space ? &not_yet_discovered_buffers_for_scavenge_ :
&not_yet_discovered_buffers_;
DCHECK(live_array_buffers->count(data) > 0);
live_array_buffers->erase(data);
not_yet_discovered_array_buffers->erase(data);

https://codereview.chromium.org/1316873004/diff/20001/src/heap/heap.cc#newcod...
src/heap/heap.cc:1761: not_yet_discovered_array_buffers_.erase(data);
This  .erase() will not be part of the method as we keep the sets disjoint.

https://codereview.chromium.org/1316873004/diff/20001/src/heap/heap.cc#newcod...
src/heap/heap.cc:1784: void Heap::FreeDeadArrayBuffers(bool from_scavenge) {
Since we keep the sets disjoint, we only need to visit XXX_scavenge_ for the new
space part and in the other case we visit all of them:

for buffer in not_yet_discovered_buffers_scavenge:
  isolate.free(buffer)
  freed_memory += …
  live_array_buffers_for_scavenge_.clear(buffer)
if !from_scavenge:
  for buffer in not_yet_discovered_buffers_:
    isolate.free(buffer)
    freed_memory += …
    live_array_buffers_.clear(buffer)


not_yet_discovered_buffers_for_scavenge_ = live_array_buffers_for_scavenge_
if !from_scavenge:
  not_yet_discovered_buffers_ = live_array_buffers_

https://codereview.chromium.org/1316873004/diff/20001/src/heap/heap.cc#newcod...
src/heap/heap.cc:1828: void Heap::TearDownArrayBuffers() {
nit: While practically not relevant, we should still tear down gentle and record
the freed memory (like in FreeDeadArrayBuffers).

e.g.
  freed_memory += buffer.second;
throughout the loop

and 
  AdjustAmountOfExternalAllocatedMemory(freed_memory); 
in the end.

[fedor.indutny, 2015-08-28 20:49:41]

The results of benchmarks are:

./node-slow 1 && ./node-no-inline-fast 1 && ./node-fast 1
4997.4 ns/op
4701.6 ns/op
4685.7 ns/op

NOTE: `no-inline-fast` is the initial version of patch, `fast` - is current,
`slow` - vanilla v8.

The other thing that I have noticed is that there are not much scavenges
happening:

[65202:0x101804c00]       22 ms: Scavenge 1.9 (38.0) -> 1.8 (38.0) MB, 0.6 ms
[allocation failure].
[65202:0x101804c00]       23 ms: Scavenge 2.0 (38.0) -> 2.0 (39.0) MB, 0.5 ms
[allocation failure].
[65202:0x101804c00]       48 ms: Scavenge 3.5 (39.0) -> 3.1 (39.0) MB, 3.4 ms
[allocation failure].
[65202:0x101804c00]       57 ms: Mark-sweep 3.7 (39.0) -> 2.9 (40.0) MB, 4.7 ms
[external memory allocation limit reached.] [GC in old space requested].
[65202:0x101804c00]       68 ms: Mark-sweep 3.4 (40.0) -> 2.9 (40.0) MB, 9.4 ms
[external memory allocation limit reached.] [GC in old space requested].
[65202:0x101804c00]       81 ms: Mark-sweep 2.9 (40.0) -> 2.9 (40.0) MB, 12.4 ms
[external memory allocation limit reached.] [GC in old space requested].
[65202:0x101804c00]       82 ms: Mark-sweep 2.9 (40.0) -> 2.7 (40.0) MB, 1.7 ms
[external memory allocation limit reached.] [GC in old space requested].
[65202:0x101804c00]       91 ms: Mark-sweep 3.3 (40.0) -> 2.7 (40.0) MB, 3.4 ms
[external memory allocation limit reached.] [GC in old space requested].

I guess making them a scavenges might be the next step to improve the
performance.

Thanks for review!

https://codereview.chromium.org/1316873004/diff/20001/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/1316873004/diff/20001/src/heap/heap.cc#newcod...
src/heap/heap.cc:1752: void Heap::UnregisterArrayBuffer(bool in_new_space, void*
data) {
On 2015/08/28 14:25:30, Michael Lippautz wrote:
> We can simplify the main part a lot:
> 
> std::map<void*, size_t>*live_buffers = 
>     in_new_space ? &live_array_buffers_for_scavenge_ : &live_array_buffers_;
> std::map<void*, size_t>* not_yet_discovered_buffers = 
>     in_new_space ? &not_yet_discovered_buffers_for_scavenge_ :
> &not_yet_discovered_buffers_;
> DCHECK(live_array_buffers->count(data) > 0);
> live_array_buffers->erase(data);
> not_yet_discovered_array_buffers->erase(data);

Acknowledged.

https://codereview.chromium.org/1316873004/diff/20001/src/heap/heap.cc#newcod...
src/heap/heap.cc:1761: not_yet_discovered_array_buffers_.erase(data);
On 2015/08/28 14:25:30, Michael Lippautz wrote:
> This  .erase() will not be part of the method as we keep the sets disjoint.

Acknowledged.

https://codereview.chromium.org/1316873004/diff/20001/src/heap/heap.cc#newcod...
src/heap/heap.cc:1776: if (from_scavenge) {
This one will need to become unconditional.

https://codereview.chromium.org/1316873004/diff/20001/src/heap/heap.cc#newcod...
src/heap/heap.cc:1784: void Heap::FreeDeadArrayBuffers(bool from_scavenge) {
On 2015/08/28 14:25:30, Michael Lippautz wrote:
> Since we keep the sets disjoint, we only need to visit XXX_scavenge_ for the
new
> space part and in the other case we visit all of them:
> 
> for buffer in not_yet_discovered_buffers_scavenge:
>   isolate.free(buffer)
>   freed_memory += …
>   live_array_buffers_for_scavenge_.clear(buffer)
> if !from_scavenge:
>   for buffer in not_yet_discovered_buffers_:
>     isolate.free(buffer)
>     freed_memory += …
>     live_array_buffers_.clear(buffer)
> 
> 
> not_yet_discovered_buffers_for_scavenge_ = live_array_buffers_for_scavenge_
> if !from_scavenge:
>   not_yet_discovered_buffers_ = live_array_buffers_

Acknowledged.

https://codereview.chromium.org/1316873004/diff/20001/src/heap/heap.cc#newcod...
src/heap/heap.cc:1828: void Heap::TearDownArrayBuffers() {
On 2015/08/28 14:25:30, Michael Lippautz wrote:
> nit: While practically not relevant, we should still tear down gentle and
record
> the freed memory (like in FreeDeadArrayBuffers).
> 
> e.g.
>   freed_memory += buffer.second;
> throughout the loop
> 
> and 
>   AdjustAmountOfExternalAllocatedMemory(freed_memory); 
> in the end.

Acknowledged.

[Michael Lippautz, 2015-08-31 06:31:58]

On 2015/08/28 20:49:41, fedor.indutny wrote:
> The results of benchmarks are:
> 
> ./node-slow 1 && ./node-no-inline-fast 1 && ./node-fast 1
> 4997.4 ns/op
> 4701.6 ns/op
> 4685.7 ns/op
> 
> NOTE: `no-inline-fast` is the initial version of patch, `fast` - is current,
> `slow` - vanilla v8.

Please add the summary to the description.

> 
> The other thing that I have noticed is that there are not much scavenges
> happening:
> 
> [65202:0x101804c00]       22 ms: Scavenge 1.9 (38.0) -> 1.8 (38.0) MB, 0.6 ms
> [allocation failure].
> [65202:0x101804c00]       23 ms: Scavenge 2.0 (38.0) -> 2.0 (39.0) MB, 0.5 ms
> [allocation failure].
> [65202:0x101804c00]       48 ms: Scavenge 3.5 (39.0) -> 3.1 (39.0) MB, 3.4 ms
> [allocation failure].
> [65202:0x101804c00]       57 ms: Mark-sweep 3.7 (39.0) -> 2.9 (40.0) MB, 4.7
ms
> [external memory allocation limit reached.] [GC in old space requested].
> [65202:0x101804c00]       68 ms: Mark-sweep 3.4 (40.0) -> 2.9 (40.0) MB, 9.4
ms
> [external memory allocation limit reached.] [GC in old space requested].
> [65202:0x101804c00]       81 ms: Mark-sweep 2.9 (40.0) -> 2.9 (40.0) MB, 12.4
ms
> [external memory allocation limit reached.] [GC in old space requested].
> [65202:0x101804c00]       82 ms: Mark-sweep 2.9 (40.0) -> 2.7 (40.0) MB, 1.7
ms
> [external memory allocation limit reached.] [GC in old space requested].
> [65202:0x101804c00]       91 ms: Mark-sweep 3.3 (40.0) -> 2.7 (40.0) MB, 3.4
ms
> [external memory allocation limit reached.] [GC in old space requested].
> 
> I guess making them a scavenges might be the next step to improve the
> performance.
> 

I don't think there's much to improve on here with the current architecture.

The buffers in question are allocated externally (while the wrapping object
JSArrayBuffer is only conditionally allocated externally). We start an
incremental GC (see api.cc AdjustAmountOfExternalAllocatedMemory) once we hit
the limit. For the scavenge map you need the scavenge information and for the
full map you need a full transitive closure.

[Michael Lippautz, 2015-08-31 06:33:45]

lgtm with comments.

The next step would be moving the logic out of Heap into an ArrayBufferTracker
that lives in heap/ and finally add some tests. It would probably have to be a
cctest that checks the transitions from one map into another.

https://codereview.chromium.org/1316873004/diff/40001/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/1316873004/diff/40001/src/heap/heap.cc#newcod...
src/heap/heap.cc:1805: if (freed_memory) {
Let's make this explcit, i.e., freed_memory > 0

https://codereview.chromium.org/1316873004/diff/40001/src/heap/heap.cc#newcod...
src/heap/heap.cc:1828: if (freed_memory) {
Ditto here.

[fedor.indutny, 2015-08-31 06:39:55]

The CQ bit was checked by fedor@indutny.com

[fedor.indutny, 2015-08-31 06:39:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlippautz@chromium.org
Link to the patchset: https://codereview.chromium.org/1316873004/#ps60001
(title: "fix last nits")

[commit-bot: I haz the power, 2015-08-31 06:40:07]

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

[commit-bot: I haz the power, 2015-08-31 06:42:51]

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

[commit-bot: I haz the power, 2015-08-31 06:42:51]

Try jobs failed on following builders:
  v8_mac_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel/builds/9247)
  v8_win_nosnap_shared_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_compil...)

[fedor.indutny, 2015-08-31 06:45:29]

On 2015/08/31 06:31:58, Michael Lippautz wrote:
> On 2015/08/28 20:49:41, fedor.indutny wrote:
> > The results of benchmarks are:
> > 
> > ./node-slow 1 && ./node-no-inline-fast 1 && ./node-fast 1
> > 4997.4 ns/op
> > 4701.6 ns/op
> > 4685.7 ns/op
> > 
> > NOTE: `no-inline-fast` is the initial version of patch, `fast` - is current,
> > `slow` - vanilla v8.
> 
> Please add the summary to the description.

Argh, looks like I missed this one before hitting the commit button. Sorry!

> 
> > 
> > The other thing that I have noticed is that there are not much scavenges
> > happening:
> > 
> > [65202:0x101804c00]       22 ms: Scavenge 1.9 (38.0) -> 1.8 (38.0) MB, 0.6
ms
> > [allocation failure].
> > [65202:0x101804c00]       23 ms: Scavenge 2.0 (38.0) -> 2.0 (39.0) MB, 0.5
ms
> > [allocation failure].
> > [65202:0x101804c00]       48 ms: Scavenge 3.5 (39.0) -> 3.1 (39.0) MB, 3.4
ms
> > [allocation failure].
> > [65202:0x101804c00]       57 ms: Mark-sweep 3.7 (39.0) -> 2.9 (40.0) MB, 4.7
> ms
> > [external memory allocation limit reached.] [GC in old space requested].
> > [65202:0x101804c00]       68 ms: Mark-sweep 3.4 (40.0) -> 2.9 (40.0) MB, 9.4
> ms
> > [external memory allocation limit reached.] [GC in old space requested].
> > [65202:0x101804c00]       81 ms: Mark-sweep 2.9 (40.0) -> 2.9 (40.0) MB,
12.4
> ms
> > [external memory allocation limit reached.] [GC in old space requested].
> > [65202:0x101804c00]       82 ms: Mark-sweep 2.9 (40.0) -> 2.7 (40.0) MB, 1.7
> ms
> > [external memory allocation limit reached.] [GC in old space requested].
> > [65202:0x101804c00]       91 ms: Mark-sweep 3.3 (40.0) -> 2.7 (40.0) MB, 3.4
> ms
> > [external memory allocation limit reached.] [GC in old space requested].
> > 
> > I guess making them a scavenges might be the next step to improve the
> > performance.
> > 
> 
> I don't think there's much to improve on here with the current architecture.
> 
> The buffers in question are allocated externally (while the wrapping object
> JSArrayBuffer is only conditionally allocated externally). We start an
> incremental GC (see api.cc AdjustAmountOfExternalAllocatedMemory) once we hit
> the limit. For the scavenge map you need the scavenge information and for the
> full map you need a full transitive closure.

We are allocating `kInternalized` ArrayBuffers in node. Is there any other way
to allocate them "internally"?

All of the allocated buffers are in the new space, may I ask you to elaborate a
bit more on why the Scavenge is not possible in this case?

Thank you!

[fedor.indutny, 2015-08-31 06:46:54]

Made a typo in debug-only line. Going to rebuild it on my machine, and resubmit
it again. Sorry about this!

(Also sounds like a good chance to update the description)

[fedor.indutny, 2015-08-31 06:54:16]

The CQ bit was checked by fedor@indutny.com

[fedor.indutny, 2015-08-31 06:54:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlippautz@chromium.org
Link to the patchset: https://codereview.chromium.org/1316873004/#ps80001
(title: "fix typo")

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

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

[commit-bot: I haz the power, 2015-08-31 07:05:19]

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

[commit-bot: I haz the power, 2015-08-31 07:05:20]

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

[Michael Lippautz, 2015-08-31 07:16:39]

On 2015/08/31 06:45:29, fedor.indutny wrote:
> On 2015/08/31 06:31:58, Michael Lippautz wrote:
> > On 2015/08/28 20:49:41, fedor.indutny wrote:
> > > I guess making them a scavenges might be the next step to improve the
> > > performance.
> > > 
> > 
> > I don't think there's much to improve on here with the current architecture.
> > 
> > The buffers in question are allocated externally (while the wrapping object
> > JSArrayBuffer is only conditionally allocated externally). We start an
> > incremental GC (see api.cc AdjustAmountOfExternalAllocatedMemory) once we
hit
> > the limit. For the scavenge map you need the scavenge information and for
the
> > full map you need a full transitive closure.
> 
> We are allocating `kInternalized` ArrayBuffers in node. Is there any other way
> to allocate them "internally"?
> 

With kExternalized we do not explicitly keep track of the buffers we are handed
when creating a JSArrayBuffer object. We just ignore the buffer contents wrt. to
garbage collection. With kInternalized we use the functions we just modified to
keep track of them. 

As far as I see the api though, in both cases we actually use a special
allocator (v8::ArrayBuffer::Allocator  array_buffer_allocator) in the isolate to
allocate those buffers. This seems to be external at all times for now, i.e.,
even for d8 we use a so-called ShellArrayBufferAllocator that essentially just
wraps malloc. jochen@(currently OOO) should know why the buffers are allocated
externally at all times.

> All of the allocated buffers are in the new space, may I ask you to elaborate
a
> bit more on why the Scavenge is not possible in this case?
> 

JSTypedArray (the wrapping JS object) usually starts in new space. The buffers
(as described above) already live on some external heap.

[fedor.indutny, 2015-08-31 07:16:50]

Michael,

Looks like there is some issue on linux buildbot. I will investigate it, and
will try to submit a fix as soon as possible.

So far it looks like a double-free error to me.

[fedor.indutny, 2015-08-31 07:21:23]

On 2015/08/31 07:16:39, Michael Lippautz wrote:
> On 2015/08/31 06:45:29, fedor.indutny wrote:
> > On 2015/08/31 06:31:58, Michael Lippautz wrote:
> > > On 2015/08/28 20:49:41, fedor.indutny wrote:
> > > > I guess making them a scavenges might be the next step to improve the
> > > > performance.
> > > > 
> > > 
> > > I don't think there's much to improve on here with the current
architecture.
> > > 
> > > The buffers in question are allocated externally (while the wrapping
object
> > > JSArrayBuffer is only conditionally allocated externally). We start an
> > > incremental GC (see api.cc AdjustAmountOfExternalAllocatedMemory) once we
> hit
> > > the limit. For the scavenge map you need the scavenge information and for
> the
> > > full map you need a full transitive closure.
> > 
> > We are allocating `kInternalized` ArrayBuffers in node. Is there any other
way
> > to allocate them "internally"?
> > 
> 
> With kExternalized we do not explicitly keep track of the buffers we are
handed
> when creating a JSArrayBuffer object. We just ignore the buffer contents wrt.
to
> garbage collection. With kInternalized we use the functions we just modified
to
> keep track of them. 
> 
> As far as I see the api though, in both cases we actually use a special
> allocator (v8::ArrayBuffer::Allocator  array_buffer_allocator) in the isolate
to
> allocate those buffers. This seems to be external at all times for now, i.e.,
> even for d8 we use a so-called ShellArrayBufferAllocator that essentially just
> wraps malloc. jochen@(currently OOO) should know why the buffers are allocated
> externally at all times.

Great, would love to learn more about it! Thanks!

> 
> > All of the allocated buffers are in the new space, may I ask you to
elaborate
> a
> > bit more on why the Scavenge is not possible in this case?
> > 
> 
> JSTypedArray (the wrapping JS object) usually starts in new space. The buffers
> (as described above) already live on some external heap.

I didn't mean the contents of ArrayBuffers, they are not direct subject to
scavenge
anyway. What I meant is that all of these GCed buffers was actually in a new
space,
so the Scavenge should be the fastest way to handle it. Running incremental GC,
with no "dead" objects in Old Space seems to be a bit wasteful.

Is there any way to modify this behavior to make it better for both us and
Chrome?
It would be great to have some sort of overridable callback which will decide,
what
to do when we external memory limits.

[Michael Lippautz, 2015-08-31 08:09:44]

On 2015/08/31 07:21:23, fedor.indutny wrote:
> On 2015/08/31 07:16:39, Michael Lippautz wrote:
> > On 2015/08/31 06:45:29, fedor.indutny wrote:
> > > On 2015/08/31 06:31:58, Michael Lippautz wrote:
> > > > On 2015/08/28 20:49:41, fedor.indutny wrote:
> > > All of the allocated buffers are in the new space, may I ask you to
> elaborate
> > a
> > > bit more on why the Scavenge is not possible in this case?
> > > 
> > 
> > JSTypedArray (the wrapping JS object) usually starts in new space. The
buffers
> > (as described above) already live on some external heap.
> 
> I didn't mean the contents of ArrayBuffers, they are not direct subject to
> scavenge
> anyway. What I meant is that all of these GCed buffers was actually in a new
> space,
> so the Scavenge should be the fastest way to handle it. Running incremental
GC,
> with no "dead" objects in Old Space seems to be a bit wasteful.
> 

Let's fix some terminology:
* JS object: The frontend JS object.
* Buffer: the buffer holding the actual contents. 

The JS object (pretty small) holds a backend, our buffer. The buffer is
externally allocated. The JS object starts in new space and the scavenger IS
already handling it. However, the microbenchmark (that's why it's only a
microbenchmark...) is only allocating these small JS objects on the V8 heap and
thus only triggering a few scavenges (as you need to hit a limit to trigger a
scavenge). 

The incremental GC you see is triggered because the externally allocated buffers
(our backends) hit a limit where we need to synchronize with the embedder
(chrome, node, ...) using a GC.  The process with synchronizing with the
embedder is complex because we are dealing with a system where multiple GCs
interfere with each other and need to find a global transitive closure (or at
least a good enough approximation). This used to be a full GC but we switched to
an incremental one to keep the pause time small.

> Is there any way to modify this behavior to make it better for both us and
> Chrome?
> It would be great to have some sort of overridable callback which will decide,
> what
> to do when we external memory limits.

In general you don't know where the JS objects holding external memory live.
Doing a scavenge here only helps your specific microbenchmark (as you know that
most of you 65k allocated buffers are tied to objects in a tight loop.). You
don't gain general knowledge by doing a scavenge.

Strategies for hitting external allocation limits could also be discussed with
jochen@. Probably a good idea to get him in the loop once he is back. 

Having said all that, allocating a "SlowBuffer" of 65k in a tight loop is
probably not the best use case :)

[Michael Lippautz, 2015-08-31 16:08:14]

Besides fixing the double free, please also rebase on master. We had to land
https://codereview.chromium.org/1325643002/ as a bugfix today.

[fedor.indutny, 2015-08-31 20:50:01]

On 2015/08/31 16:08:14, Michael Lippautz wrote:
> Besides fixing the double free, please also rebase on master. We had to land
> https://codereview.chromium.org/1325643002/ as a bugfix today.

Thanks for letting me know! I think that it has a bug too ;) Commented there.

It looks like it might be the cause for this double-free, actually. Going to
reproduce it locally and see where are the roots of the problem in case of this
CL. If it is because of recursive GC - I'm going to just rebase and push it
again.

[fedor.indutny, 2015-08-31 20:51:32]

On 2015/08/31 20:50:01, fedor.indutny wrote:
> On 2015/08/31 16:08:14, Michael Lippautz wrote:
> > Besides fixing the double free, please also rebase on master. We had to land
> > https://codereview.chromium.org/1325643002/ as a bugfix today.
> 
> Thanks for letting me know! I think that it has a bug too ;) Commented there.
> 
> It looks like it might be the cause for this double-free, actually. Going to
> reproduce it locally and see where are the roots of the problem in case of
this
> CL. If it is because of recursive GC - I'm going to just rebase and push it
> again.

Looking more at it, it looks like `UnregisterArrayBuffer` should adjust the
amount of used external memory too. What do you think?

[fedor.indutny, 2015-09-01 01:19:19]

register new_space 0x1031335d0
register new_space 0x103560110
live inc 0x102381a30
live inc 0x102381970
live inc 0x103560110
live inc 0x1031335d0
PrepareArrayBufferDiscovery
live scavenge 0x1031335d0
live scavenge 0x103560110
enter adjust scavenge
leave adjust scavenge
[53385:0x102287910]    71238 ms: Scavenge 553.2 (589.7) -> 551.5 (589.7) MB,
14.7 / 0 ms (+ 748.7 ms in 30 steps since last GC) [allocation failure].
free [s] inc 0x1031335d0
free [s] inc 0x103560110
enter adjust inc
leave adjust inc
promote 0x1031335d0
promote 0x103560110
[53385:0x102287910]    71387 ms: Mark-sweep 551.7 (589.7) -> 32.7 (70.6) MB,
147.0 / 0 ms (+ 749.7 ms in 31 steps since start of marking, biggest step 167.7
ms) [GC interrupt] [GC in old space requested].

NOTE:

`live inc` means RegisterLiveBuffer during Mark-Sweep, `live scavenge` during
Scavenge
`enter adjust`/`exit adjust` are before and after adjust external memory size
`free [s]` free from scavenge list, `free [i]` free from mark-sweep list

It looks like it enters Scavenge from Mark-sweep. It doesn't look like there are
any adjustments triggering the Scavenge, but it is still called recursively. Is
it expected?
in_reply_to: 5686306919153664
send_mail: 1
subject: heap: make array buffer maps disjoint

[fedor.indutny, 2015-09-01 06:19:38]

Michael,

I have figured out the problem. See my changes to mark-compact.cc.

The buffers from new space was evacuated after being freed. It should be better
now.

Going to run tests locally and submit the thing to CQ again.

Thanks!

[fedor.indutny, 2015-09-01 06:25:57]

The CQ bit was checked by fedor@indutny.com

[fedor.indutny, 2015-09-01 06:25:57]

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

[commit-bot: I haz the power, 2015-09-01 06:26:06]

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

[commit-bot: I haz the power, 2015-09-01 06:51:59]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-09-01 06:52:21]

Patchset 8 (id:??) landed as
https://crrev.com/9e3676da9ab1aaf7de3e8582cb3fdefcc3dbaf33
Cr-Commit-Position: refs/heads/master@{#30495}

[Michael Lippautz, 2015-09-01 08:30:06]

Please wait next time when doing a change in another component, even if
l-g-t-m-ed already. The order of invocations during GC is not something to just
change.

Also, I feel we should be able to call FreeDeadArrayBuffers at any time after
marking. Please see comment and elaborate a bit more on it.

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

https://codereview.chromium.org/1316873004/diff/140001/src/heap/mark-compact....
src/heap/mark-compact.cc:4436: // NOTE: ArrayBuffers must be evacuated first,
before freeing them. Otherwise
Can you elaborate on this comment a bit more. Why are the not yet discovered? We
should've computed a transitive closure, marking ALL live objects, icluding new
space ones. This would move them into the live map of buffers.

[fedor.indutny, 2015-09-01 08:50:08]

Sorry for pushing it out without your ack. I hope this patch looks ok, if not -
please feel free to revert it.

https://codereview.chromium.org/1316873004/diff/40001/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/1316873004/diff/40001/src/heap/heap.cc#newcod...
src/heap/heap.cc:1805: if (freed_memory) {
On 2015/08/31 06:33:45, Michael Lippautz wrote:
> Let's make this explcit, i.e., freed_memory > 0

Acknowledged.

https://codereview.chromium.org/1316873004/diff/40001/src/heap/heap.cc#newcod...
src/heap/heap.cc:1828: if (freed_memory) {
On 2015/08/31 06:33:45, Michael Lippautz wrote:
> Ditto here.

Acknowledged.

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

https://codereview.chromium.org/1316873004/diff/140001/src/heap/mark-compact....
src/heap/mark-compact.cc:4436: // NOTE: ArrayBuffers must be evacuated first,
before freeing them. Otherwise
On 2015/09/01 08:30:06, Michael Lippautz wrote:
> Can you elaborate on this comment a bit more. Why are the not yet discovered?
We
> should've computed a transitive closure, marking ALL live objects, icluding
new
> space ones. This would move them into the live map of buffers.

I might not be understanding this correctly, but we are not really visiting the
new space objects until we are doing the Sweep phase. So we do erase ptrs in the
`not_yet_discovered` only on `EvacuateNewSpaceAndCandidates` call.

The marking phase do not register them, because it is backed by
`StaticMarkingVisitor`, and only `StaticNewSpaceVisitor` is capable of
registering.

[Michael Lippautz, 2015-09-01 09:01:36]

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

https://codereview.chromium.org/1316873004/diff/140001/src/heap/mark-compact....
src/heap/mark-compact.cc:4436: // NOTE: ArrayBuffers must be evacuated first,
before freeing them. Otherwise
On 2015/09/01 08:50:08, fedor.indutny wrote:
> On 2015/09/01 08:30:06, Michael Lippautz wrote:
> > Can you elaborate on this comment a bit more. Why are the not yet
discovered?
> We
> > should've computed a transitive closure, marking ALL live objects, icluding
> new
> > space ones. This would move them into the live map of buffers.
> 
> I might not be understanding this correctly, but we are not really visiting
the
> new space objects until we are doing the Sweep phase. So we do erase ptrs in
the
> `not_yet_discovered` only on `EvacuateNewSpaceAndCandidates` call.
> 
> The marking phase do not register them, because it is backed by
> `StaticMarkingVisitor`, and only `StaticNewSpaceVisitor` is capable of
> registering.

StaticMarkingVisitor is also registering live arrays and should do the right
thing. Mind investigating the root problem? It would be good to maintain the
invariant that we can free the dead buffers anywhere after marking.

[fedor.indutny, 2015-09-01 09:08:14]

On 2015/09/01 09:01:36, Michael Lippautz wrote:
>
https://codereview.chromium.org/1316873004/diff/140001/src/heap/mark-compact.cc
> File src/heap/mark-compact.cc (right):
> 
>
https://codereview.chromium.org/1316873004/diff/140001/src/heap/mark-compact....
> src/heap/mark-compact.cc:4436: // NOTE: ArrayBuffers must be evacuated first,
> before freeing them. Otherwise
> On 2015/09/01 08:50:08, fedor.indutny wrote:
> > On 2015/09/01 08:30:06, Michael Lippautz wrote:
> > > Can you elaborate on this comment a bit more. Why are the not yet
> discovered?
> > We
> > > should've computed a transitive closure, marking ALL live objects,
icluding
> > new
> > > space ones. This would move them into the live map of buffers.
> > 
> > I might not be understanding this correctly, but we are not really visiting
> the
> > new space objects until we are doing the Sweep phase. So we do erase ptrs in
> the
> > `not_yet_discovered` only on `EvacuateNewSpaceAndCandidates` call.
> > 
> > The marking phase do not register them, because it is backed by
> > `StaticMarkingVisitor`, and only `StaticNewSpaceVisitor` is capable of
> > registering.
> 
> StaticMarkingVisitor is also registering live arrays and should do the right
> thing. Mind investigating the root problem? It would be good to maintain the
> invariant that we can free the dead buffers anywhere after marking.

Argh, you are right. Looking into it.

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

Ok, you are definitely right. Looking through the logs that I have collected I
see the following scenario:

1. Marking starts
2. ArrayBuffers in NewSpace are visited and marked as live by removing them from
not_yet_..._for_scavenge
3. Scavenge starts, PrepareArrayBufferDiscovery is called,
not_yet_..._for_scavenge is overwritten
4. ArrayBuffers in NewSpace are visited again and removed from the
not_yet_..._for_scavenge
5. Scavenge ends, buffers are not freed `not_yet_..._for_scavenge` is
overwritten by `live_..._for_scavenge`
6. Mark-Compact GC resumes
7. `live_..._for_scavenge === not_yet_..._for_scavenge`
8. Buffers are freed
9. EvacuateNewSpaceAndCandidates is invoked, it finds these buffers and promotes
them to Old Space
10. Crash happens when Old Space is GCed.

I think the reordering `Evacuate` and `FreeDeadBuffers` fixes it with the price
of loosing the invariant. The other choice is to disable Scavenge during
Mark-Compact GC.

Thoughts?

[Michael Lippautz, 2015-09-01 09:56:52]

On 2015/09/01 09:15:46, fedor.indutny wrote:
> Ok, you are definitely right. Looking through the logs that I have collected I
> see the following scenario:
> 
> 1. Marking starts
> 2. ArrayBuffers in NewSpace are visited and marked as live by removing them
from
> not_yet_..._for_scavenge
> 3. Scavenge starts, PrepareArrayBufferDiscovery is called,
> not_yet_..._for_scavenge is overwritten
> 4. ArrayBuffers in NewSpace are visited again and removed from the
> not_yet_..._for_scavenge
> 5. Scavenge ends, buffers are not freed `not_yet_..._for_scavenge` is
> overwritten by `live_..._for_scavenge`
> 6. Mark-Compact GC resumes
> 7. `live_..._for_scavenge === not_yet_..._for_scavenge`
> 8. Buffers are freed
> 9. EvacuateNewSpaceAndCandidates is invoked, it finds these buffers and
promotes
> them to Old Space
> 10. Crash happens when Old Space is GCed.
> 
> I think the reordering `Evacuate` and `FreeDeadBuffers` fixes it with the
price
> of loosing the invariant. The other choice is to disable Scavenge during
> Mark-Compact GC.
> 
> Thoughts?

As far as I see this only mitigates the problem.

The problem is resetting the buffers in your step 5. As soon as you do the
mark-compact GC you would delete all buffers in the not_yet_discovered set
(which has been reset for scavenge).

Moving it after evacuate helps for fixing the state of promoted objects.
However, objects could be allocated between a scavenge and the final GC, which
would would not make them eligible for promotion (objects are copied once before
promoted). As a result, for these objects is that they are live, but freed in
FreeDeadArrayBuffers, as there state is not fixed during PromoteArrayBuffer.

The solution is to not only fix promoted objects, but also those that are only
copied in new space during
MarkCompactCollector::DiscoverAndEvacuateBlackObjectsOnPage. 

I will precautionally revert this as I would like it to be a single CL. You can
send me another CL that includes the additional fix in mark-compact.cc. The
order of EvacuateNewSpaceAndCandidates and FreeDeadArrayBuffers can stay as in
this CL.

[Michael Lippautz, 2015-09-01 09:58:07]

A revert of this CL (patchset #8 id:140001) has been created in
https://codereview.chromium.org/1302233007/ by mlippautz@chromium.org.

The reason for reverting is: Precautionary revert. The change is incomplete..