Allows the host application to provide a mechanism to be notified and perfor...

Allows the host application to provide a mechanism for notification and custom logging through a callback (set through the API V8::SetExecutableAllocCallback), when V8 Allocates Executable Memory.

This replaces the current histogram data "V8.ExecutableMemoryMax" by allowing usage data to be logged by the application as required rather than always through V8.

BUG=54222
TEST=Set the callback function with the API and cause V8 to allocate executable memory.

Committed: http://code.google.com/p/v8/source/detail?r=5403

[Søren Thygesen Gjesse, 2010-09-02 07:44:03]

http://codereview.chromium.org/3329006/diff/2001/3001
File include/v8.h (right):

http://codereview.chromium.org/3329006/diff/2001/3001#newcode2362
include/v8.h:2362: typedef void (*ExecutableAllocCallback)(int size_executable);
I think this callback type is to specific in just reporting executable memory.
How about having

  MemoryAllocationCallback(AllocationSpace space, MemoryType type,
AllocationAction action, int size)

with enums

  enum AllocationSpace {
    kAllocationSpaceYoung,
    kAllocationSpacePaged,
    kAllocationSpaceLarge
  }

  enum MemoryType {
    kMemoryTypeData,
    kMemoryTypeExecutable
  }

  enum AllocationAction {
    kAllocationActionAllocate,
    kAllocationActionFree
  }

Something like GCPrologueCallback and GCEpilogueCallback.

http://codereview.chromium.org/3329006/diff/2001/3001#newcode2520
include/v8.h:2520: static void
SetExecutableAllocCallback(ExecutableAllocCallback);
With the more general callback above this could take filter arguments of type
AllocationSpace, MemoryType and AllocationAction (the enums would have to be
bits then). Also consider whether more than one callback should be allowed to be
active at any one time. For GC we deprecated SetGlobalGCPrologueCallback and
SetGlobalGCEpilogueCallback in favour of
AddGCPrologueCallback/RemoveGCPrologueCallback and
AddGCEpilogueCallback/RemoveGCEpilogueCallback at some point.

http://codereview.chromium.org/3329006/diff/2001/3003
File src/spaces.cc (right):

http://codereview.chromium.org/3329006/diff/2001/3003#newcode368
src/spaces.cc:368: if (executable_alloc_callback_function_)
Please add != NULL to this type of condition.

http://codereview.chromium.org/3329006/diff/2001/3003#newcode397
src/spaces.cc:397: void
MemoryAllocator::SetExecutableAllocCallback(ExecutableAllocCallback f) {
Rename f to callback.

[Valoo, 2010-09-02 08:31:05]

http://codereview.chromium.org/3329006/diff/2001/3001
File include/v8.h (right):

http://codereview.chromium.org/3329006/diff/2001/3001#newcode2362
include/v8.h:2362: typedef void (*ExecutableAllocCallback)(int size_executable);
Agreed, I'll make the changes shortly.

On 2010/09/02 07:44:04, Søren Gjesse wrote:
> I think this callback type is to specific in just reporting executable memory.
> How about having
> 
>   MemoryAllocationCallback(AllocationSpace space, MemoryType type,
> AllocationAction action, int size)
> 
> with enums
> 
>   enum AllocationSpace {
>     kAllocationSpaceYoung,
>     kAllocationSpacePaged,
>     kAllocationSpaceLarge
>   }
> 
>   enum MemoryType {
>     kMemoryTypeData,
>     kMemoryTypeExecutable
>   }
> 
>   enum AllocationAction {
>     kAllocationActionAllocate,
>     kAllocationActionFree
>   }
> 
> Something like GCPrologueCallback and GCEpilogueCallback.

http://codereview.chromium.org/3329006/diff/2001/3001#newcode2520
include/v8.h:2520: static void
SetExecutableAllocCallback(ExecutableAllocCallback);
For my purposes its not required, however since Set was deprecated in favor of
Add it makes perfect sense.  Ill make the adjustment.

On 2010/09/02 07:44:04, Søren Gjesse wrote:
> With the more general callback above this could take filter arguments of type
> AllocationSpace, MemoryType and AllocationAction (the enums would have to be
> bits then). Also consider whether more than one callback should be allowed to
be
> active at any one time. For GC we deprecated SetGlobalGCPrologueCallback and
> SetGlobalGCEpilogueCallback in favour of
> AddGCPrologueCallback/RemoveGCPrologueCallback and
> AddGCEpilogueCallback/RemoveGCEpilogueCallback at some point.

http://codereview.chromium.org/3329006/diff/2001/3003
File src/spaces.cc (right):

http://codereview.chromium.org/3329006/diff/2001/3003#newcode368
src/spaces.cc:368: if (executable_alloc_callback_function_)
On 2010/09/02 07:44:04, Søren Gjesse wrote:
> Please add != NULL to this type of condition.

Done.

http://codereview.chromium.org/3329006/diff/2001/3003#newcode397
src/spaces.cc:397: void
MemoryAllocator::SetExecutableAllocCallback(ExecutableAllocCallback f) {
On 2010/09/02 07:44:04, Søren Gjesse wrote:
> Rename f to callback.

Done.

[Søren Thygesen Gjesse, 2010-09-02 13:17:38]

http://codereview.chromium.org/3329006/diff/13003/17001
File include/v8.h (right):

http://codereview.chromium.org/3329006/diff/13003/17001#newcode2363
include/v8.h:2363: enum ObjectSpace {
Actually thinking about this again we could get rid of the AllocationType enum,
and have kObjectSpaceCode here instead. Allocation in code space will always be
executable and allocation in all other spaces will be non-executable.

This should have values 1:1 to v8::internal::AllocationSpace

And then have some masks e.g. kObjectSpaceTenured =
kObjectSpaceTenuredPointerSpace | kObjectSpaceTenuredDataSpace |
kObjectSpaceMapSpace | kObjectSpaceCellSpace

As far as I can see MemoryAllocator::AllocateRawMemory is only called from a few
places so tracking the space should not be that difficult.

http://codereview.chromium.org/3329006/diff/13003/17001#newcode2384
include/v8.h:2384: AllocationAction action);
This typedef is missing an int argument for the size of the allocation.

http://codereview.chromium.org/3329006/diff/13003/17003
File src/spaces.cc (right):

http://codereview.chromium.org/3329006/diff/13003/17003#newcode376
src/spaces.cc:376: AllocationType type = (executable == EXECUTABLE) ?
kAllocationTypeCode :
Please format like this

AllocationType type = 
    (executable == EXECUTABLE) ? kAllocationTypeCode : kAllocationTypeData;

http://codereview.chromium.org/3329006/diff/13003/17003#newcode380
src/spaces.cc:380: filter.callback(filter.space, type,
kAllocationActionAllocate);
I think you should pass 0 here instead of filter.space whilt this is not yet
implemented.

http://codereview.chromium.org/3329006/diff/13003/17003#newcode403
src/spaces.cc:403: AllocationType type = (executable == EXECUTABLE) ?
kAllocationTypeCode :
Ditto.

http://codereview.chromium.org/3329006/diff/13003/17004
File src/spaces.h (right):

http://codereview.chromium.org/3329006/diff/13003/17004#newcode657
src/spaces.h:657: struct MemoryAllocationCallbackFilter {
How about renaming MemoryAllocationCallbackFilter to
MemoryAllocationCallbackRegistration?

http://codereview.chromium.org/3329006/diff/13003/17004#newcode662
src/spaces.h:662: : callback(callback),
4 space indent of constructor initializer list, see
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Constr...

You can fit the initializer list on one line then.

http://codereview.chromium.org/3329006/diff/13003/17004#newcode667
src/spaces.h:667: bool operator==(const MemoryAllocationCallbackFilter& filter)
const {
I don't like this == operator which only compares the callback. I suggest
replacing List<>::Contains with a loop instead.

[Valoo, 2010-09-03 10:59:07]

http://codereview.chromium.org/3329006/diff/13003/17001
File include/v8.h (right):

http://codereview.chromium.org/3329006/diff/13003/17001#newcode2384
include/v8.h:2384: AllocationAction action);
On 2010/09/02 13:17:38, Søren Gjesse wrote:
> This typedef is missing an int argument for the size of the allocation.

Done.

http://codereview.chromium.org/3329006/diff/13003/17003
File src/spaces.cc (right):

http://codereview.chromium.org/3329006/diff/13003/17003#newcode376
src/spaces.cc:376: AllocationType type = (executable == EXECUTABLE) ?
kAllocationTypeCode :
On 2010/09/02 13:17:38, Søren Gjesse wrote:
> Please format like this
> 
> AllocationType type = 
>     (executable == EXECUTABLE) ? kAllocationTypeCode : kAllocationTypeData;

Done.

http://codereview.chromium.org/3329006/diff/13003/17003#newcode380
src/spaces.cc:380: filter.callback(filter.space, type,
kAllocationActionAllocate);
Because the space is determined prior to MemoryAllocator::AllocateRawMemory, the
callback is now performed in
MemoryAllocator::PerformAllocationCallback(ObjectSpace space, AllocationAction
action, int size);

On 2010/09/02 13:17:38, Søren Gjesse wrote:
> I think you should pass 0 here instead of filter.space whilt this is not yet
> implemented.

http://codereview.chromium.org/3329006/diff/13003/17003#newcode403
src/spaces.cc:403: AllocationType type = (executable == EXECUTABLE) ?
kAllocationTypeCode :
On 2010/09/02 13:17:38, Søren Gjesse wrote:
> Ditto.

Done.

http://codereview.chromium.org/3329006/diff/13003/17004
File src/spaces.h (right):

http://codereview.chromium.org/3329006/diff/13003/17004#newcode657
src/spaces.h:657: struct MemoryAllocationCallbackFilter {
On 2010/09/02 13:17:38, Søren Gjesse wrote:
> How about renaming MemoryAllocationCallbackFilter to
> MemoryAllocationCallbackRegistration?

Done.

http://codereview.chromium.org/3329006/diff/13003/17004#newcode662
src/spaces.h:662: : callback(callback),
On 2010/09/02 13:17:38, Søren Gjesse wrote:
> 4 space indent of constructor initializer list, see
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Constr...
> 
> You can fit the initializer list on one line then.

Done.

http://codereview.chromium.org/3329006/diff/13003/17004#newcode667
src/spaces.h:667: bool operator==(const MemoryAllocationCallbackFilter& filter)
const {
On 2010/09/02 13:17:38, Søren Gjesse wrote:
> I don't like this == operator which only compares the callback. I suggest
> replacing List<>::Contains with a loop instead.

Done.

[Søren Thygesen Gjesse, 2010-09-03 11:33:45]

LGTM

Committed: http://code.google.com/p/v8/source/detail?r=5403

Closing issue