New GCCallbacks with additional parameters.

New GCCallbacks with additional parameters.

See http://codereview.chromium.org/1165004.

[loislo, 2010-03-22 14:06:49]

New GCCallback related functions were introduced in V8 api.
I think we should support multiple set/remove operations for the callbacks.
That is the reason why I've used hash map of callback pointers.

[yurys, 2010-03-22 14:20:28]

http://codereview.chromium.org/1094002/diff/6001/7003
File include/v8.h (right):

http://codereview.chromium.org/1094002/diff/6001/7003#newcode2320
include/v8.h:2320: static int SetGCPrologueCallback(GCPrologueCallback
callback);
I'd rather call it AddGCPrologueCallback

http://codereview.chromium.org/1094002/diff/6001/7002
File src/heap.h (right):

http://codereview.chromium.org/1094002/diff/6001/7002#newcode1106
src/heap.h:1106: HashMap* gc_callbacks_;
why not use a linked list or a vector for that?

[mnaganov (inactive), 2010-03-22 14:22:33]

http://codereview.chromium.org/1094002/diff/6001/7002
File src/heap.h (right):

http://codereview.chromium.org/1094002/diff/6001/7002#newcode1062
src/heap.h:1062: template <typename T>
I'd say, there is no real need here for templatizing code -- it will just create
unnecessary code duplication. I think, storing void* is enough.

http://codereview.chromium.org/1094002/diff/6001/7002#newcode1073
src/heap.h:1073: void InvokeGCCallbacks(GCType gc_type, GCCallbackFlags
callback_flag) {
Please put implementation in a separate include file (heap-inl.h)

[Søren Thygesen Gjesse, 2010-03-22 14:48:02]

Please add a test case to test-api.cc.

http://codereview.chromium.org/1094002/diff/6001/7003
File include/v8.h (right):

http://codereview.chromium.org/1094002/diff/6001/7003#newcode2313
include/v8.h:2313: /**
The comments have not been updated. They still mention major GC's. Please keep
the old comments together with the old functions, and extend these comments
saying that these API's are deprecated and that xxx should be used instead.

http://codereview.chromium.org/1094002/diff/6001/7003#newcode2320
include/v8.h:2320: static int SetGCPrologueCallback(GCPrologueCallback
callback);
How about changing enum GCType into a bitfield 
enum GCType {
  kGCTypeScavenge = 1 << 0,
  kGCTypeMarkSweep = 1 << 2,
  kGCTypeMarkCompact = 1 << 3,
  kGCTypeAll = kGCTypeScavenge | kGCTypeMarkSweep | kGCTypeMarkCompact
};

and add an argument to these AddGCXXX to indicate which GC's to receive a
callback for

  AddGCPrologueCallback(GCType types, GCPrologueCallback callback)

This way we could use this new API for what the SetGlobalGCPrologueCallback and
SetGlobalGCEpilogueCallback does now and be able to retire these API's at some
point.

http://codereview.chromium.org/1094002/diff/6001/7002
File src/heap.h (right):

http://codereview.chromium.org/1094002/diff/6001/7002#newcode692
src/heap.h:692: static void SetGlobalGCPrologueCallback(GCCallback callback) {
I don't understand this ASSERT. Please always use != NULL or == NULL when
dealing with pointers.

http://codereview.chromium.org/1094002/diff/6001/7002#newcode696
src/heap.h:696: static void SetGlobalGCEpilogueCallback(GCCallback callback) {
Ditto.

http://codereview.chromium.org/1094002/diff/6001/7002#newcode1068
src/heap.h:1068: if (gc_callbacks_) {
(gc_callbacks_ != NULL) instead of just (gc_callbacks_).

Same for the other uses of (gc_callbacks_) and (!gc_callbacks_) including the
ones in ASSERT.

http://codereview.chromium.org/1094002/diff/6001/7002#newcode1074
src/heap.h:1074: if (!isEmpty())
Please add {}' for this if.

[loislo, 2010-03-22 20:32:55]

HashMap was replaced by List<>.
test was added.

http://codereview.chromium.org/1094002/diff/6001/7003
File include/v8.h (right):

http://codereview.chromium.org/1094002/diff/6001/7003#newcode2320
include/v8.h:2320: static int SetGCPrologueCallback(GCPrologueCallback
callback);
On 2010/03/22 14:20:28, Yury Semikhatsky wrote:
> I'd rather call it AddGCPrologueCallback 

Done.

http://codereview.chromium.org/1094002/diff/6001/7003#newcode2320
include/v8.h:2320: static int SetGCPrologueCallback(GCPrologueCallback
callback);
On 2010/03/22 14:48:02, Søren Gjesse wrote:
> How about changing enum GCType into a bitfield 
> enum GCType {
>   kGCTypeScavenge = 1 << 0,
>   kGCTypeMarkSweep = 1 << 2,
>   kGCTypeMarkCompact = 1 << 3,
>   kGCTypeAll = kGCTypeScavenge | kGCTypeMarkSweep | kGCTypeMarkCompact
> };
> 
> and add an argument to these AddGCXXX to indicate which GC's to receive a
> callback for
> 
>   AddGCPrologueCallback(GCType types, GCPrologueCallback callback)
> 
> This way we could use this new API for what the SetGlobalGCPrologueCallback
and
> SetGlobalGCEpilogueCallback does now and be able to retire these API's at some
> point.

Done.

http://codereview.chromium.org/1094002/diff/6001/7002
File src/heap.h (right):

http://codereview.chromium.org/1094002/diff/6001/7002#newcode692
src/heap.h:692: static void SetGlobalGCPrologueCallback(GCCallback callback) {
On 2010/03/22 14:48:02, Søren Gjesse wrote:
> I don't understand this ASSERT. Please always use != NULL or == NULL when
> dealing with pointers.

Done.

http://codereview.chromium.org/1094002/diff/6001/7002#newcode696
src/heap.h:696: static void SetGlobalGCEpilogueCallback(GCCallback callback) {
On 2010/03/22 14:48:02, Søren Gjesse wrote:
> Ditto.

Done.

http://codereview.chromium.org/1094002/diff/6001/7002#newcode1062
src/heap.h:1062: template <typename T>
On 2010/03/22 14:22:33, Michail Naganov wrote:
> I'd say, there is no real need here for templatizing code -- it will just
create
> unnecessary code duplication. I think, storing void* is enough.

The implementation was replaced to List<>.

http://codereview.chromium.org/1094002/diff/6001/7002#newcode1073
src/heap.h:1073: void InvokeGCCallbacks(GCType gc_type, GCCallbackFlags
callback_flag) {
On 2010/03/22 14:22:33, Michail Naganov wrote:
> Please put implementation in a separate include file (heap-inl.h)

Done.

http://codereview.chromium.org/1094002/diff/6001/7002#newcode1074
src/heap.h:1074: if (!isEmpty())
On 2010/03/22 14:48:02, Søren Gjesse wrote:
> Please add {}' for this if.

Done.

http://codereview.chromium.org/1094002/diff/6001/7002#newcode1106
src/heap.h:1106: HashMap* gc_callbacks_;
On 2010/03/22 14:20:28, Yury Semikhatsky wrote:
> why not use a linked list or a vector for that?

Done.

[mnaganov (inactive), 2010-03-23 09:01:05]

LGTM with comments.

http://codereview.chromium.org/1094002/diff/41001/42002
File src/heap.h (right):

http://codereview.chromium.org/1094002/diff/41001/42002#newcode35
src/heap.h:35: #include "list.h"
Is there a real need for this include? List was already used in Heap (see line
1790). Also, as List is a template class, you should include "list-inl.h".

Anyway, in V8, every .cc file includes "v8.h" which usually includes all
necessary stuff, so including individual headers in .h files is really rare.

http://codereview.chromium.org/1094002/diff/41001/42005
File test/cctest/test-api.cc (right):

http://codereview.chromium.org/1094002/diff/41001/42005#newcode9985
test/cctest/test-api.cc:9985: i::Heap::CollectAllGarbage(false);
nit: for clarity, I'd add

CHECK_EQ(0, prologue_call_count);
CHECK_EQ(0, epilogue_call_count);

prior to GC.

[Søren Thygesen Gjesse, 2010-03-23 09:20:21]

http://codereview.chromium.org/1094002/diff/41001/42003
File include/v8.h (right):

http://codereview.chromium.org/1094002/diff/41001/42003#newcode2324
include/v8.h:2324: static void AddGCPrologueCallback(GCPrologueCallback
callback,
Please format like this

static void AddGCPrologueCallback(
    GCPrologueCallback callback, GCType gc_type_filter = kGCTypeAll);

if aligning the arguments does not fit within 80 characters.

http://codereview.chromium.org/1094002/diff/41001/42003#newcode2353
include/v8.h:2353: static void AddGCEpilogueCallback(GCEpilogueCallback
callback,
Ditto.

http://codereview.chromium.org/1094002/diff/41001/42001
File src/heap.cc (right):

http://codereview.chromium.org/1094002/diff/41001/42001#newcode554
src/heap.cc:554: GCType gc_type =
Looking more at this it seems that we have to change the reporting a bit. As we
cannot know beforehand whether the mark sweep will compact or not we should only
have two GC types instead of three. We can then have a flag which in the
epilogue callback can indicate whether compacting took place.

enum GCType {
  kGCTypeScavenge = 1 << 0,
  kGCTypeMarkSweepCompact = 1 << 1,
  kGCTypeAll = kGCTypeScavenge | kGCTypeMarkSweepCompact
};

enum GCCallbackFlags {
  kNoGCCallbackFlags = 0
  kGCCallbackFlagCompacted = 1 << 0
};

You can get that information from the GCTracer.

http://codereview.chromium.org/1094002/diff/41001/42001#newcode3818
src/heap.cc:3818: gc_prologue_callbacks_.Remove(i);
As we are breaking here, maybe add an ASSERT checking for adding the same
callback twice.

http://codereview.chromium.org/1094002/diff/41001/42002
File src/heap.h (right):

http://codereview.chromium.org/1094002/diff/41001/42002#newcode677
src/heap.h:677: static void AddGCPrologueCallback(GCEpilogueCallback callback,
Please align arguments.

http://codereview.chromium.org/1094002/diff/41001/42002#newcode681
src/heap.h:681: static void AddGCEpilogueCallback(GCEpilogueCallback callback,
Ditto.

http://codereview.chromium.org/1094002/diff/41001/42002#newcode1056
src/heap.h:1056: GCPrologueCallbackPair(GCPrologueCallback callback, GCType
gc_type)
4 space indent of :.

http://codereview.chromium.org/1094002/diff/41001/42002#newcode1065
src/heap.h:1065: GCEpilogueCallbackPair(GCEpilogueCallback callback, GCType
gc_type)
Ditto.

[loislo, 2010-03-23 11:10:05]

http://codereview.chromium.org/1094002/diff/41001/42003
File include/v8.h (right):

http://codereview.chromium.org/1094002/diff/41001/42003#newcode2324
include/v8.h:2324: static void AddGCPrologueCallback(GCPrologueCallback
callback,
On 2010/03/23 09:20:22, Søren Gjesse wrote:
> Please format like this
> 
> static void AddGCPrologueCallback(
>     GCPrologueCallback callback, GCType gc_type_filter = kGCTypeAll);
> 
> if aligning the arguments does not fit within 80 characters.

Done.

http://codereview.chromium.org/1094002/diff/41001/42003#newcode2353
include/v8.h:2353: static void AddGCEpilogueCallback(GCEpilogueCallback
callback,
On 2010/03/23 09:20:22, Søren Gjesse wrote:
> Ditto.

Done.

http://codereview.chromium.org/1094002/diff/41001/42001
File src/heap.cc (right):

http://codereview.chromium.org/1094002/diff/41001/42001#newcode554
src/heap.cc:554: GCType gc_type =
On 2010/03/23 09:20:22, Søren Gjesse wrote:
> Looking more at this it seems that we have to change the reporting a bit. As
we
> cannot know beforehand whether the mark sweep will compact or not we should
only
> have two GC types instead of three. We can then have a flag which in the
> epilogue callback can indicate whether compacting took place.
> 
> enum GCType {
>   kGCTypeScavenge = 1 << 0,
>   kGCTypeMarkSweepCompact = 1 << 1,
>   kGCTypeAll = kGCTypeScavenge | kGCTypeMarkSweepCompact
> };
> 
> enum GCCallbackFlags {
>   kNoGCCallbackFlags = 0
>   kGCCallbackFlagCompacted = 1 << 0
> };
> 
> You can get that information from the GCTracer.

Done.

http://codereview.chromium.org/1094002/diff/41001/42001#newcode3818
src/heap.cc:3818: gc_prologue_callbacks_.Remove(i);
On 2010/03/23 09:20:22, Søren Gjesse wrote:
> As we are breaking here, maybe add an ASSERT checking for adding the same
> callback twice.

Done.

http://codereview.chromium.org/1094002/diff/41001/42002
File src/heap.h (right):

http://codereview.chromium.org/1094002/diff/41001/42002#newcode35
src/heap.h:35: #include "list.h"
On 2010/03/23 09:01:05, Michail Naganov wrote:
> Is there a real need for this include? List was already used in Heap (see line
> 1790). Also, as List is a template class, you should include "list-inl.h".
> 
> Anyway, in V8, every .cc file includes "v8.h" which usually includes all
> necessary stuff, so including individual headers in .h files is really rare.

Done.

http://codereview.chromium.org/1094002/diff/41001/42002#newcode677
src/heap.h:677: static void AddGCPrologueCallback(GCEpilogueCallback callback,
On 2010/03/23 09:20:22, Søren Gjesse wrote:
> Please align arguments.

Done.

http://codereview.chromium.org/1094002/diff/41001/42002#newcode681
src/heap.h:681: static void AddGCEpilogueCallback(GCEpilogueCallback callback,
On 2010/03/23 09:20:22, Søren Gjesse wrote:
> Ditto.

Done.

http://codereview.chromium.org/1094002/diff/41001/42002#newcode1056
src/heap.h:1056: GCPrologueCallbackPair(GCPrologueCallback callback, GCType
gc_type)
On 2010/03/23 09:20:22, Søren Gjesse wrote:
> 4 space indent of :.

Done.

http://codereview.chromium.org/1094002/diff/41001/42002#newcode1065
src/heap.h:1065: GCEpilogueCallbackPair(GCEpilogueCallback callback, GCType
gc_type)
On 2010/03/23 09:20:22, Søren Gjesse wrote:
> Ditto.

Done.

http://codereview.chromium.org/1094002/diff/41001/42005
File test/cctest/test-api.cc (right):

http://codereview.chromium.org/1094002/diff/41001/42005#newcode9985
test/cctest/test-api.cc:9985: i::Heap::CollectAllGarbage(false);
On 2010/03/23 09:01:05, Michail Naganov wrote:
> nit: for clarity, I'd add
> 
> CHECK_EQ(0, prologue_call_count);
> CHECK_EQ(0, epilogue_call_count);
> 
> prior to GC.

Done.

[Søren Thygesen Gjesse, 2010-03-23 11:20:11]

LGTM