[api] Clarify expectations of ArrayBuffer::Allocator in API

[api] Clarify expectations of ArrayBuffer::Allocator in API

BUG=chromium:611688
LOG=Y
R=jochen@chromium.org

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

[Michael Lippautz, 2016-05-13 07:48:38]

Description was changed from

==========
[api] Clarify expectations for ArrayBuffer::Allocator in API docs

BUG=
==========

to

==========
[api] Clarify expectations for ArrayBuffer::Allocator in API docs

LOG=Y
R=jochen@chromium.org
==========

[Michael Lippautz, 2016-05-13 07:48:39]

mlippautz@chromium.org changed reviewers:
+ jochen@chromium.org

[Michael Lippautz, 2016-05-13 07:48:57]

Description was changed from

==========
[api] Clarify expectations for ArrayBuffer::Allocator in API docs

LOG=Y
R=jochen@chromium.org
==========

to

==========
[api] Clarify expectations of ArrayBuffer::Allocator in API

LOG=Y
R=jochen@chromium.org
==========

[Michael Lippautz, 2016-05-13 07:48:57]

Description was changed from

==========
[api] Clarify expectations for ArrayBuffer::Allocator in API docs

LOG=Y
R=jochen@chromium.org
==========

to

==========
[api] Clarify expectations of ArrayBuffer::Allocator in API

LOG=Y
R=jochen@chromium.org
==========

[Michael Lippautz, 2016-05-13 07:49:48]

The CQ bit was checked by mlippautz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-13 07:49:52]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1978773002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1978773002/1

[commit-bot: I haz the power, 2016-05-13 08:22:10]

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

[commit-bot: I haz the power, 2016-05-13 08:22:11]

Dry run: This issue passed the CQ dry run.

[jochen (gone - plz use gerrit), 2016-05-13 09:03:32]

https://codereview.chromium.org/1978773002/diff/1/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/1978773002/diff/1/include/v8.h#newcode3460
include/v8.h:3460: * Memory allocated through this allocator is accounted for as
external
But just of it's allocated by v8?

And who has to do the accounting if an array buffer is externalized?

And if the embedder allocates the backing store but then gives it as
internalized to v8?

[Michael Lippautz, 2016-05-13 09:11:23]

https://codereview.chromium.org/1978773002/diff/1/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/1978773002/diff/1/include/v8.h#newcode3460
include/v8.h:3460: * Memory allocated through this allocator is accounted for as
external
On 2016/05/13 09:03:32, jochen (OOO) wrote:
> But just of it's allocated by v8?
> 
Yes.

> And who has to do the accounting if an array buffer is externalized?

Externlize unregisters it from being tracked in V8, also updating the
accounting. So the embedder needs to adjust accounting if it keeps the buffer
alive.

> 
> And if the embedder allocates the backing store but then gives it as
> internalized to v8?

Other way round. The embedder should take care of first reducing the count,
because V8 will keep track of it.

That's how the implementation details currently look like -- or probably should
look like since we currently do something different (double-accounting).

[jochen (gone - plz use gerrit), 2016-05-13 09:15:04]

https://codereview.chromium.org/1978773002/diff/1/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/1978773002/diff/1/include/v8.h#newcode3460
include/v8.h:3460: * Memory allocated through this allocator is accounted for as
external
On 2016/05/13 at 09:11:22, Michael Lippautz wrote:
> On 2016/05/13 09:03:32, jochen (OOO) wrote:
> > But just of it's allocated by v8?
> > 
> Yes.
> 
> > And who has to do the accounting if an array buffer is externalized?
> 
> Externlize unregisters it from being tracked in V8, also updating the
accounting. So the embedder needs to adjust accounting if it keeps the buffer
alive.
> 
> > 
> > And if the embedder allocates the backing store but then gives it as
> > internalized to v8?
> 
> Other way round. The embedder should take care of first reducing the count,
because V8 will keep track of it.
> 
> That's how the implementation details currently look like -- or probably
should look like since we currently do something different (double-accounting).

Can you include all that in the comment?

[Michael Lippautz, 2016-05-13 09:26:44]

https://codereview.chromium.org/1978773002/diff/1/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/1978773002/diff/1/include/v8.h#newcode3460
include/v8.h:3460: * Memory allocated through this allocator is accounted for as
external
On 2016/05/13 09:15:03, jochen (OOO) wrote:
> On 2016/05/13 at 09:11:22, Michael Lippautz wrote:
> > On 2016/05/13 09:03:32, jochen (OOO) wrote:
> > > But just of it's allocated by v8?
> > > 
> > Yes.
> > 
> > > And who has to do the accounting if an array buffer is externalized?
> > 
> > Externlize unregisters it from being tracked in V8, also updating the
> accounting. So the embedder needs to adjust accounting if it keeps the buffer
> alive.
> > 
> > > 
> > > And if the embedder allocates the backing store but then gives it as
> > > internalized to v8?
> > 
> > Other way round. The embedder should take care of first reducing the count,
> because V8 will keep track of it.
> > 
> > That's how the implementation details currently look like -- or probably
> should look like since we currently do something different
(double-accounting).
> 
> Can you include all that in the comment?

Done.

[jochen (gone - plz use gerrit), 2016-05-13 09:29:06]

lgtm

[Michael Lippautz, 2016-05-13 09:31:40]

Description was changed from

==========
[api] Clarify expectations of ArrayBuffer::Allocator in API

LOG=Y
R=jochen@chromium.org
==========

to

==========
[api] Clarify expectations of ArrayBuffer::Allocator in API

BUG=chromium:611688
LOG=Y
R=jochen@chromium.org
==========

[Michael Lippautz, 2016-05-13 09:31:56]

The CQ bit was checked by mlippautz@chromium.org

[commit-bot: I haz the power, 2016-05-13 09:32:06]

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

[Michael Lippautz, 2016-05-13 09:39:59]

+ulan

I added one more sentence that clearly states that it is unsafe to call back
into V8 from any of those functions.

[Michael Lippautz, 2016-05-13 09:40:04]

The CQ bit was checked by mlippautz@chromium.org

[Michael Lippautz, 2016-05-13 09:40:04]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/1978773002/#ps40001
(title: "Add one more comment")

[commit-bot: I haz the power, 2016-05-13 09:40:07]

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

[commit-bot: I haz the power, 2016-05-13 10:11:39]

Description was changed from

==========
[api] Clarify expectations of ArrayBuffer::Allocator in API

BUG=chromium:611688
LOG=Y
R=jochen@chromium.org
==========

to

==========
[api] Clarify expectations of ArrayBuffer::Allocator in API

BUG=chromium:611688
LOG=Y
R=jochen@chromium.org
==========

[commit-bot: I haz the power, 2016-05-13 10:11:41]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-05-13 10:13:46]

Description was changed from

==========
[api] Clarify expectations of ArrayBuffer::Allocator in API

BUG=chromium:611688
LOG=Y
R=jochen@chromium.org
==========

to

==========
[api] Clarify expectations of ArrayBuffer::Allocator in API

BUG=chromium:611688
LOG=Y
R=jochen@chromium.org

Committed: https://crrev.com/b32f9599bee0f37fd75129482b0008b953870129
Cr-Commit-Position: refs/heads/master@{#36231}
==========

[commit-bot: I haz the power, 2016-05-13 10:13:47]

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