[heap] introduce ArrayBufferTracker

[heap] introduce ArrayBufferTracker

Move various ArrayBuffer-related methods from Heap class to the newly
created ArrayBufferTracker. Consolidate and simplify things!

BUG=
R=mlippautz@chromium.org

Committed: https://crrev.com/c7392f2e419a36c820f3fd8338fdd6266247acd7
Cr-Commit-Position: refs/heads/master@{#30624}

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

Hello Michael!

The refactoring as previously discussed.

Any suggestions on how to do regression testing for the problems that we have
observed with this things at first?

Thanks!

[Michael Lippautz, 2015-09-03 10:37:54]

Thanks, that's going into the direction I had in mind.

For the first round:

Please move ArrayBufferTracker to its own .h/.cc files in src/heap/, add a
forward declaration in heap.h. Don't inline the object, but rather store a
pointer for an object that get's initialized properly.

Similar things happened for ObjectStats (which was also part of Heap) just
recently. See https://codereview.chromium.org/1326793002/ (especially for the
build related files).

As for the tests, I will think of something.

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

On a completely unrelated note. Do you think that the way ArrayBuffers are
managed right now is faster than the way External strings are?

The strings appears to be living in the table (list), and this list is iterated
on GC, freeing the items that have not been marked.

The arraybuffers live in a maps. Maps are copied, partially erased, and
everything is freed from them.

The performance-wise notes:

1. std::map.insert is slower than pushing stuff to the list
2. iterating over the list should have comparable speed (if not the same)
3. erasing might have equal speed, more on this in next paragraph

Erasure does not happen it all for external string, and for std::map it is doing
a RB-tree lookup. But when freeing stuff from the external map we are reading
all the markings that were inserted by GC.

It seems to me that if the erasure takes the same time, we may want to adopt
this concept for ArrayBuffers too, because std::map.insert is very slow.

What are your thoughts on this?

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

And I have just split out the implementation, as you requested

[Michael Lippautz, 2015-09-04 08:24:16]

Thanks, here we go again. ;)

Performance: Let's not optimize this scenario to death unless we have a
real-world workload that needs to be considered. I agree that it's unfortunate
that we have different implementations for tracking external strings and
external arraybuffers though. You are right about lookups and clears, but I
don't think that it's currently a bottle-neck. If you feel that this is really
important and have a real-world workload I am open for discussion. We should
also include e.g. jochen@ in this case as he knows a lot more about other
embedders (chrome). 

Refactoring: We try really hard to get our Heap properly encapsulated (still
work in progress) and offer a basic heap api which requires only including
src/heap/heap.h. We don't want the users of heap, i.e., general v8 api (api.cc),
other runtime parts (object.cc, runtime-typed-arrays.cc) to know about how we
track arraybuffers. If they register/unregister properly than they should be
fine. The comments below address these issues.

Testing: I think you should be able to write a cctest (in
test/cctest/test-heap.cc for example) which basically creates array buffers,
calls the GC (scavenge, full) and tracks amount_of_externally_allocated_memory_
along the way.

https://codereview.chromium.org/1324023007/diff/20001/src/DEPS
File src/DEPS (right):

https://codereview.chromium.org/1324023007/diff/20001/src/DEPS#newcode8
src/DEPS:8: "+src/heap/array-buffer-tracker.h",
Remove this one.

https://codereview.chromium.org/1324023007/diff/20001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/1324023007/diff/20001/src/api.cc#newcode35
src/api.cc:35: #include "src/heap/array-buffer-tracker.h"
Include only src/heap/heap.h

https://codereview.chromium.org/1324023007/diff/20001/src/api.cc#newcode6513
src/api.cc:6513: isolate->heap()->array_buffer_tracker()->Unregister(*self);
Use the wrapper call for Unregister.

https://codereview.chromium.org/1324023007/diff/20001/src/api.cc#newcode6722
src/api.cc:6722: isolate->heap()->array_buffer_tracker()->Unregister(*self);
Ditto.

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

https://codereview.chromium.org/1324023007/diff/20001/src/heap/heap.cc#newcod...
src/heap/heap.cc:168: array_buffer_tracker_ = new ArrayBufferTracker(this);
Move this into SetUp() somewhere next to ObjectStats.

The reason we do it this (in a separate SetUp call) way is that initializer
order (= declaration order) does -- unfortunately -- not match with the order we
need for some components.

https://codereview.chromium.org/1324023007/diff/20001/src/heap/heap.h
File src/heap/heap.h (left):

https://codereview.chromium.org/1324023007/diff/20001/src/heap/heap.h#oldcode...
src/heap/heap.h:1016: void RegisterNewArrayBuffer(bool in_new_space, void* data,
size_t length);
Create public wrapper calls only for RegisterNewArrayBuffer and
UnregisterNewArrayBuffer in Heap. These calls should forward and provide the
interface into the heap.

https://codereview.chromium.org/1324023007/diff/20001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1324023007/diff/20001/src/objects.cc#newcode30
src/objects.cc:30: #include "src/heap/array-buffer-tracker.h"
Only include src/heap/heap.h

https://codereview.chromium.org/1324023007/diff/20001/src/objects.cc#newcode1...
src/objects.cc:15947: isolate->heap()->array_buffer_tracker()->RegisterNew(
Use the wrapper call for Register.

https://codereview.chromium.org/1324023007/diff/20001/src/objects.cc#newcode1...
src/objects.cc:16002: isolate->heap()->array_buffer_tracker()->RegisterNew(
Ditto.

https://codereview.chromium.org/1324023007/diff/20001/src/runtime/runtime-typ...
File src/runtime/runtime-typedarray.cc (right):

https://codereview.chromium.org/1324023007/diff/20001/src/runtime/runtime-typ...
src/runtime/runtime-typedarray.cc:9: #include "src/heap/array-buffer-tracker.h"
Only include src/heap/heap.h

https://codereview.chromium.org/1324023007/diff/20001/src/runtime/runtime-typ...
src/runtime/runtime-typedarray.cc:96:
isolate->heap()->array_buffer_tracker()->Unregister(*array_buffer);
Use the wrapper call for Unregister.

[fedor.indutny, 2015-09-04 08:39:17]

Just added jochen@ as you have suggested, Mikeal. Additionally, I have cc-ed
Trevor Norris from node.js team.

I think this inconsistency has to be addressed, we can probably even unite this
code and make a single table for both ArrayBuffers and External strings.

Trevor, may I ask you to share your insights on Buffer performance regression
that has happened during recent upgrades of v8 in node? Are there any non-micro
benchmarks that reproduce the problem, does it affect performance of http or
net.Server in global?

Thanks!

[fedor.indutny, 2015-09-04 08:58:05]

Pushed out the fixes that make the ArrayBufferTracker isolated. Going to see
what could be done with the tests. Should it be a separate CL?

https://codereview.chromium.org/1324023007/diff/20001/src/DEPS
File src/DEPS (right):

https://codereview.chromium.org/1324023007/diff/20001/src/DEPS#newcode8
src/DEPS:8: "+src/heap/array-buffer-tracker.h",
On 2015/09/04 08:24:15, Michael Lippautz wrote:
> Remove this one.

Acknowledged.

https://codereview.chromium.org/1324023007/diff/20001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/1324023007/diff/20001/src/api.cc#newcode35
src/api.cc:35: #include "src/heap/array-buffer-tracker.h"
On 2015/09/04 08:24:15, Michael Lippautz wrote:
> Include only src/heap/heap.h

Acknowledged, doesn't look like it is needed.

https://codereview.chromium.org/1324023007/diff/20001/src/api.cc#newcode6513
src/api.cc:6513: isolate->heap()->array_buffer_tracker()->Unregister(*self);
On 2015/09/04 08:24:15, Michael Lippautz wrote:
> Use the wrapper call for Unregister.

Acknowledged.

https://codereview.chromium.org/1324023007/diff/20001/src/api.cc#newcode6722
src/api.cc:6722: isolate->heap()->array_buffer_tracker()->Unregister(*self);
On 2015/09/04 08:24:15, Michael Lippautz wrote:
> Ditto.

Acknowledged.

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

https://codereview.chromium.org/1324023007/diff/20001/src/heap/heap.cc#newcod...
src/heap/heap.cc:168: array_buffer_tracker_ = new ArrayBufferTracker(this);
On 2015/09/04 08:24:15, Michael Lippautz wrote:
> Move this into SetUp() somewhere next to ObjectStats.
> 
> The reason we do it this (in a separate SetUp call) way is that initializer
> order (= declaration order) does -- unfortunately -- not match with the order
we
> need for some components.

Acknowledged.

https://codereview.chromium.org/1324023007/diff/20001/src/heap/heap.h
File src/heap/heap.h (left):

https://codereview.chromium.org/1324023007/diff/20001/src/heap/heap.h#oldcode...
src/heap/heap.h:1016: void RegisterNewArrayBuffer(bool in_new_space, void* data,
size_t length);
On 2015/09/04 08:24:15, Michael Lippautz wrote:
> Create public wrapper calls only for RegisterNewArrayBuffer and
> UnregisterNewArrayBuffer in Heap. These calls should forward and provide the
> interface into the heap.

Acknowledged.

https://codereview.chromium.org/1324023007/diff/20001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1324023007/diff/20001/src/objects.cc#newcode30
src/objects.cc:30: #include "src/heap/array-buffer-tracker.h"
On 2015/09/04 08:24:15, Michael Lippautz wrote:
> Only include src/heap/heap.h

Acknowledged, doesn't look like it is needed.

https://codereview.chromium.org/1324023007/diff/20001/src/objects.cc#newcode1...
src/objects.cc:15947: isolate->heap()->array_buffer_tracker()->RegisterNew(
On 2015/09/04 08:24:15, Michael Lippautz wrote:
> Use the wrapper call for Register.

Acknowledged.

https://codereview.chromium.org/1324023007/diff/20001/src/objects.cc#newcode1...
src/objects.cc:16002: isolate->heap()->array_buffer_tracker()->RegisterNew(
On 2015/09/04 08:24:15, Michael Lippautz wrote:
> Ditto.

Acknowledged.

https://codereview.chromium.org/1324023007/diff/20001/src/runtime/runtime-typ...
File src/runtime/runtime-typedarray.cc (right):

https://codereview.chromium.org/1324023007/diff/20001/src/runtime/runtime-typ...
src/runtime/runtime-typedarray.cc:9: #include "src/heap/array-buffer-tracker.h"
On 2015/09/04 08:24:15, Michael Lippautz wrote:
> Only include src/heap/heap.h

Acknowledged, doesn't look like it is needed.

https://codereview.chromium.org/1324023007/diff/20001/src/runtime/runtime-typ...
src/runtime/runtime-typedarray.cc:96:
isolate->heap()->array_buffer_tracker()->Unregister(*array_buffer);
On 2015/09/04 08:24:16, Michael Lippautz wrote:
> Use the wrapper call for Unregister.

Acknowledged.

[Michael Lippautz, 2015-09-04 08:58:16]

On 2015/09/04 08:39:17, fedor.indutny wrote:
> Just added jochen@ as you have suggested, Mikeal. Additionally, I have cc-ed
> Trevor Norris from node.js team.
> 
> I think this inconsistency has to be addressed, we can probably even unite
this
> code and make a single table for both ArrayBuffers and External strings.
> 
> Trevor, may I ask you to share your insights on Buffer performance regression
> that has happened during recent upgrades of v8 in node? Are there any
non-micro
> benchmarks that reproduce the problem, does it affect performance of http or
> net.Server in global?
> 
> Thanks!

Let's keep the discussion separated from the code review, please. You can open a
thread on 
  v8-dev@googlegroups.com

Please explicitly cc the people involved so that people don't miss it.

[Michael Lippautz, 2015-09-04 09:09:57]

Since this is refactoring and almost done I am fine with a separate CL.

https://codereview.chromium.org/1324023007/diff/40001/src/heap/array-buffer-t...
File src/heap/array-buffer-tracker.cc (right):

https://codereview.chromium.org/1324023007/diff/40001/src/heap/array-buffer-t...
src/heap/array-buffer-tracker.cc:32: bool in_new_space =
heap()->InNewSpace(buffer);
Move below !data check.

https://codereview.chromium.org/1324023007/diff/40001/src/heap/array-buffer-t...
src/heap/array-buffer-tracker.cc:133: }
nit:
}

}  // namespace internal
}  // namespace v8

https://codereview.chromium.org/1324023007/diff/40001/src/heap/array-buffer-t...
File src/heap/array-buffer-tracker.h (right):

https://codereview.chromium.org/1324023007/diff/40001/src/heap/array-buffer-t...
src/heap/array-buffer-tracker.h:73: }
nit:
};

}  // namespace internal
}  // namespace v8

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

https://codereview.chromium.org/1324023007/diff/40001/src/heap/heap.cc#newcod...
src/heap/heap.cc:1978: void Heap::RegisterNewArrayBuffer(bool in_new_space,
void* data,
Anything holding us back to change the signature to take a JSArrayBuffer*
buffer? If not, please adjust.

https://codereview.chromium.org/1324023007/diff/40001/src/heap/heap.h
File src/heap/heap.h (right):

https://codereview.chromium.org/1324023007/diff/40001/src/heap/heap.h#newcode...
src/heap/heap.h:1105: inline ArrayBufferTracker* array_buffer_tracker() {
nit: You can move this into the ArrayBufferTracker section.

https://codereview.chromium.org/1324023007/diff/40001/src/heap/heap.h#newcode...
src/heap/heap.h:1512: // ArrayBufferTracker proxies.
===============================================
nit: Not an actual proxy. Remove this word please ;)

[fedor.indutny, 2015-09-04 17:21:01]

All fixed!

https://codereview.chromium.org/1324023007/diff/40001/src/heap/array-buffer-t...
File src/heap/array-buffer-tracker.cc (right):

https://codereview.chromium.org/1324023007/diff/40001/src/heap/array-buffer-t...
src/heap/array-buffer-tracker.cc:32: bool in_new_space =
heap()->InNewSpace(buffer);
On 2015/09/04 09:09:56, Michael Lippautz wrote:
> Move below !data check.

Acknowledged.

https://codereview.chromium.org/1324023007/diff/40001/src/heap/array-buffer-t...
src/heap/array-buffer-tracker.cc:133: }
On 2015/09/04 09:09:56, Michael Lippautz wrote:
> nit:
> }
> 
> }  // namespace internal
> }  // namespace v8

Acknowledged.

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

https://codereview.chromium.org/1324023007/diff/40001/src/heap/heap.cc#newcod...
src/heap/heap.cc:1978: void Heap::RegisterNewArrayBuffer(bool in_new_space,
void* data,
On 2015/09/04 09:09:57, Michael Lippautz wrote:
> Anything holding us back to change the signature to take a JSArrayBuffer*
> buffer? If not, please adjust.

Acknowledged.

https://codereview.chromium.org/1324023007/diff/40001/src/heap/heap.h
File src/heap/heap.h (right):

https://codereview.chromium.org/1324023007/diff/40001/src/heap/heap.h#newcode...
src/heap/heap.h:1105: inline ArrayBufferTracker* array_buffer_tracker() {
On 2015/09/04 09:09:57, Michael Lippautz wrote:
> nit: You can move this into the ArrayBufferTracker section.

Acknowledged.

https://codereview.chromium.org/1324023007/diff/40001/src/heap/heap.h#newcode...
src/heap/heap.h:1512: // ArrayBufferTracker proxies.
===============================================
On 2015/09/04 09:09:57, Michael Lippautz wrote:
> nit: Not an actual proxy. Remove this word please ;)

Acknowledged.

[Michael Lippautz, 2015-09-07 06:39:34]

lgtm % 1 comment.

https://codereview.chromium.org/1324023007/diff/60001/src/heap/array-buffer-t...
File src/heap/array-buffer-tracker.cc (right):

https://codereview.chromium.org/1324023007/diff/60001/src/heap/array-buffer-t...
src/heap/array-buffer-tracker.cc:132: reinterpret_cast<v8::Isolate*>(isolate)
Use update_amount_of_external_allocated_memory().

[fedor.indutny, 2015-09-07 07:05:25]

Thank you, fixed! Please take a look.

https://codereview.chromium.org/1324023007/diff/60001/src/heap/array-buffer-t...
File src/heap/array-buffer-tracker.cc (right):

https://codereview.chromium.org/1324023007/diff/60001/src/heap/array-buffer-t...
src/heap/array-buffer-tracker.cc:132: reinterpret_cast<v8::Isolate*>(isolate)
On 2015/09/07 06:39:33, Michael Lippautz wrote:
> Use update_amount_of_external_allocated_memory().

Acknowledged.

[Michael Lippautz, 2015-09-07 08:00:42]

lgtm, give it a go

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

The CQ bit was checked by fedor@indutny.com

[commit-bot: I haz the power, 2015-09-07 08:15:00]

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

[commit-bot: I haz the power, 2015-09-07 08:20:14]

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

[commit-bot: I haz the power, 2015-09-07 08:20:14]

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

[fedor.indutny, 2015-09-07 08:26:26]

Oh, this was pretty stupid. I have just renamed the TearDown to
~ArrayBufferTracker, and added `delete` statement to the `Heap::TearDown`.
Please take another look, sorry!

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

Just in case, I'm going to wait for your LGTM, because I have made more changes
:) Sorry if the previous message was confusing.

[Michael Lippautz, 2015-09-07 10:31:23]

still lgtm

[fedor.indutny, 2015-09-07 18:49:32]

The CQ bit was checked by fedor@indutny.com

[commit-bot: I haz the power, 2015-09-07 18:49:36]

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

[commit-bot: I haz the power, 2015-09-07 19:38:19]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-09-07 19:38:38]

Patchset 6 (id:??) landed as
https://crrev.com/c7392f2e419a36c820f3fd8338fdd6266247acd7
Cr-Commit-Position: refs/heads/master@{#30624}