Oilpan: Remove GC_INFO_{DEFINE,DECLARE} macros

Oilpan: Remove GC_INFO_{DEFINE,DECLARE} macros

This CL is a cherry-pick of
[oilpan] Make GC_INFO_{DEFINE,DECLARE} macros NOOP
https://codereview.chromium.org/171843002/

BUG=None

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=167743

[kouhei (in TOK), 2014-02-17 06:02:26]

I want to avoid writing GC_INFO_{DEFINE,DECLARE} manually. This is a
proof-of-concept patch to remove the macros. wdyt?

This patch do not have support for readable "markerType" string yet, but I think
it can be implemented by hacking symbol names like I did in InstanceCounter
impl, assuming they are only needed in DEBUG builds.

[kouhei (in TOK), 2014-02-17 06:02:55]

On 2014/02/17 06:02:26, kouhei wrote:
> I want to avoid writing GC_INFO_{DEFINE,DECLARE} manually. This is a
> proof-of-concept patch to remove the macros. wdyt?
> 
> This patch do not have support for readable "markerType" string yet, but I
think
> it can be implemented by hacking symbol names like I did in InstanceCounter
> impl, assuming they are only needed in DEBUG builds.

This patch passes blink_heap_unittests and content_shell compiles/runs.

[kouhei (in TOK), 2014-02-17 06:10:20]

+Slava

[haraken, 2014-02-17 06:12:06]

You're expected to add oilpan-reviews@ for oilpan CLs.

[Mads Ager (chromium), 2014-02-17 07:22:07]

I like this experiment but I think we should do it on the branch instead of on
trunk. On trunk we do not have much code that actually uses oilpan yet. I would
feel more confident if we do this on the branch first to check that it works
there. In particular, I think the friend declaration that this patch removes
will be needed in places. It would be nice to see how often that is needed on
the branch to evaluate if that is better than having the macros that you just
consistently use.

https://codereview.chromium.org/169293002/diff/1/Source/heap/Visitor.h
File Source/heap/Visitor.h (left):

https://codereview.chromium.org/169293002/diff/1/Source/heap/Visitor.h#oldcod...
Source/heap/Visitor.h:142: template<typename Any> friend struct FinalizerTrait; 
                    \
Here we are losing a friend declaration. Maybe that is not needed on trunk yes
but it is on the branch. That might make this less ideal since we would then
have to sometimes add a friend declaration and sometimes not. It might be easier
to just have the rule that you always have DECLARE_GC_INFO?

https://codereview.chromium.org/169293002/diff/1/Source/heap/Visitor.h
File Source/heap/Visitor.h (right):

https://codereview.chromium.org/169293002/diff/1/Source/heap/Visitor.h#newcod...
Source/heap/Visitor.h:539: static const GCInfo gcInfo = {
The other difference with this is that these structures are now local statics
and therefore setup on first call to the get method instead of at startup. That
could be a good thing.

[kouhei (in TOK), 2014-02-17 08:13:48]

Thanks for your feedback! I'll try this on the branch oilpan.

[kouhei (in TOK), 2014-02-17 08:35:52]

https://codereview.chromium.org/169293002/diff/1/Source/heap/Visitor.h
File Source/heap/Visitor.h (left):

https://codereview.chromium.org/169293002/diff/1/Source/heap/Visitor.h#oldcod...
Source/heap/Visitor.h:142: template<typename Any> friend struct FinalizerTrait; 
                    \
On 2014/02/17 07:22:07, Mads Ager (chromium) wrote:
> Here we are losing a friend declaration. Maybe that is not needed on trunk yes
> but it is on the branch. That might make this less ideal since we would then
> have to sometimes add a friend declaration and sometimes not. It might be
easier
> to just have the rule that you always have DECLARE_GC_INFO?

This friend stmt is for calling |finalize()| method declared private. However,
there is only a few places where finalize() is declared, so I think it is better
to have friend declaration at there. I'll write a follow up patch for this.

https://codereview.chromium.org/169293002/diff/1/Source/heap/Visitor.h
File Source/heap/Visitor.h (right):

https://codereview.chromium.org/169293002/diff/1/Source/heap/Visitor.h#newcod...
Source/heap/Visitor.h:539: static const GCInfo gcInfo = {
On 2014/02/17 07:22:07, Mads Ager (chromium) wrote:
> The other difference with this is that these structures are now local statics
> and therefore setup on first call to the get method instead of at startup.
That
> could be a good thing.

There is no difference in generated code.
I tested this on g++, but this don't setup on first call, but the struct is
statically embedded to the binary ".rodata" section.

[wibling-chromium, 2014-02-17 08:44:15]

Very cool. It would be nice if we could get rid of the manually added
DECLARE/DEFINE.

https://codereview.chromium.org/169293002/diff/1/Source/heap/Visitor.h
File Source/heap/Visitor.h (left):

https://codereview.chromium.org/169293002/diff/1/Source/heap/Visitor.h#oldcod...
Source/heap/Visitor.h:142: template<typename Any> friend struct FinalizerTrait; 
                    \
On 2014/02/17 08:35:53, kouhei wrote:
> On 2014/02/17 07:22:07, Mads Ager (chromium) wrote:
> > Here we are losing a friend declaration. Maybe that is not needed on trunk
yes
> > but it is on the branch. That might make this less ideal since we would then
> > have to sometimes add a friend declaration and sometimes not. It might be
> easier
> > to just have the rule that you always have DECLARE_GC_INFO?
> 
> This friend stmt is for calling |finalize()| method declared private. However,
> there is only a few places where finalize() is declared, so I think it is
better
> to have friend declaration at there. I'll write a follow up patch for this.

Can't we add the friend declaration to GarbageCollectedFinalized instead? Or are
you talking about the case where it is overriden by the specific classes?

[kouhei (in TOK), 2014-02-17 08:48:18]

https://codereview.chromium.org/169293002/diff/1/Source/heap/Visitor.h
File Source/heap/Visitor.h (left):

https://codereview.chromium.org/169293002/diff/1/Source/heap/Visitor.h#oldcod...
Source/heap/Visitor.h:142: template<typename Any> friend struct FinalizerTrait; 
                    \
On 2014/02/17 08:44:15, wibling-chromium wrote:
> On 2014/02/17 08:35:53, kouhei wrote:
> > On 2014/02/17 07:22:07, Mads Ager (chromium) wrote:
> > > Here we are losing a friend declaration. Maybe that is not needed on trunk
> yes
> > > but it is on the branch. That might make this less ideal since we would
then
> > > have to sometimes add a friend declaration and sometimes not. It might be
> > easier
> > > to just have the rule that you always have DECLARE_GC_INFO?
> > 
> > This friend stmt is for calling |finalize()| method declared private.
However,
> > there is only a few places where finalize() is declared, so I think it is
> better
> > to have friend declaration at there. I'll write a follow up patch for this.
> 
> Can't we add the friend declaration to GarbageCollectedFinalized instead? Or
are
> you talking about the case where it is overriden by the specific classes?
Yes. I think we can add the friend decl to GarbageCollectedFinalized and other
specific classes where finalized() is manually overriden.

[Vyacheslav Egorov (Chromium), 2014-02-17 10:40:44]

Very nice simplification!

[kouhei (in TOK), 2014-02-20 08:23:33]

PTAL. This is already landed on the branch, so now this is a cherry-pick of the
CL.

[Mads Ager (chromium), 2014-02-20 08:42:09]

Could you remove the macros completely and get rid of all uses as well?

[kouhei (in TOK), 2014-02-20 08:43:27]

On 2014/02/20 08:42:09, Mads Ager (chromium) wrote:
> Could you remove the macros completely and get rid of all uses as well?

Got it.

[haraken, 2014-02-20 08:46:35]

On 2014/02/20 08:43:27, kouhei wrote:
> On 2014/02/20 08:42:09, Mads Ager (chromium) wrote:
> > Could you remove the macros completely and get rid of all uses as well?
> 
> Got it.

Probably you might want to do the removal in the experimental branch first to
make sure that it works as expected.

[kouhei (in TOK), 2014-02-24 01:51:12]

PTAL. Removed all macros.

[haraken, 2014-02-24 01:55:08]

The side-by-side diff is broken. Would you reupload?

[kouhei (in TOK), 2014-02-24 01:58:14]

On 2014/02/24 01:55:08, haraken wrote:
> The side-by-side diff is broken. Would you reupload?

done

[haraken, 2014-02-24 02:10:48]

Please remove "empty" files.

https://codereview.chromium.org/169293002/diff/360001/Source/modules/geolocat...
File Source/modules/geolocation/Geoposition.cpp (left):

https://codereview.chromium.org/169293002/diff/360001/Source/modules/geolocat...
Source/modules/geolocation/Geoposition.cpp:12: void Geoposition::trace(Visitor*
visitor)

You can move trace() to the header and remove this file.

Basically it wouldn't be worth adding a new file just for trace().

https://codereview.chromium.org/169293002/diff/360001/Source/modules/geolocat...
File Source/modules/geolocation/PositionError.cpp (left):

https://codereview.chromium.org/169293002/diff/360001/Source/modules/geolocat...
Source/modules/geolocation/PositionError.cpp:1: // Copyright 2014 The Chromium
Authors. All rights reserved.

You can remove this file.

[haraken, 2014-02-24 02:11:43]

On 2014/02/24 02:10:48, haraken wrote:
> Please remove "empty" files.
> 
>
https://codereview.chromium.org/169293002/diff/360001/Source/modules/geolocat...
> File Source/modules/geolocation/Geoposition.cpp (left):
> 
>
https://codereview.chromium.org/169293002/diff/360001/Source/modules/geolocat...
> Source/modules/geolocation/Geoposition.cpp:12: void
Geoposition::trace(Visitor*
> visitor)
> 
> You can move trace() to the header and remove this file.
> 
> Basically it wouldn't be worth adding a new file just for trace().
> 
>
https://codereview.chromium.org/169293002/diff/360001/Source/modules/geolocat...
> File Source/modules/geolocation/PositionError.cpp (left):
> 
>
https://codereview.chromium.org/169293002/diff/360001/Source/modules/geolocat...
> Source/modules/geolocation/PositionError.cpp:1: // Copyright 2014 The Chromium
> Authors. All rights reserved.
> 
> You can remove this file.

I didn't check all files, so I'd be happy if you could find empty files and
remove :)

[kouhei (in TOK), 2014-02-24 03:06:17]

PTAL. removed "empty" files..

[haraken, 2014-02-24 03:57:16]

LGTM

[kouhei (in TOK), 2014-02-24 03:58:34]

The CQ bit was checked by kouhei@chromium.org

[commit-bot: I haz the power, 2014-02-24 03:58:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kouhei@chromium.org/169293002/390002

[commit-bot: I haz the power, 2014-02-24 05:03:28]

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

[commit-bot: I haz the power, 2014-02-24 05:03:28]

Retried try job too often on mac_layout for step(s) webkit_lint
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout...

[kouhei (in TOK), 2014-02-25 02:56:51]

Source/heap/Visitor.h implementation changed to support css/.
haraken: PTAL

[haraken, 2014-02-25 03:19:32]

LGTM

[kouhei (in TOK), 2014-02-25 03:20:50]

The CQ bit was checked by kouhei@chromium.org

[commit-bot: I haz the power, 2014-02-25 03:21:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kouhei@chromium.org/169293002/570001

[kouhei (in TOK), 2014-02-25 03:44:57]

On 2014/02/25 03:21:02, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
> https://chromium-status.appspot.com/cq/kouhei%40chromium.org/169293002/570001

looks like this fails heap unittests. looking.

[kouhei (in TOK), 2014-02-25 03:56:47]

The CQ bit was checked by kouhei@chromium.org

[kouhei (in TOK), 2014-02-25 03:56:52]

The CQ bit was unchecked by kouhei@chromium.org

[kouhei (in TOK), 2014-02-25 03:56:52]

The CQ bit was checked by kouhei@chromium.org

[kouhei (in TOK), 2014-02-25 03:56:52]

The CQ bit was unchecked by kouhei@chromium.org

[kouhei (in TOK), 2014-02-25 03:56:53]

The CQ bit was checked by kouhei@chromium.org

[kouhei (in TOK), 2014-02-25 04:27:57]

The CQ bit was checked by kouhei@chromium.org

[commit-bot: I haz the power, 2014-02-25 04:28:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kouhei@chromium.org/169293002/590001

[commit-bot: I haz the power, 2014-02-25 05:20:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kouhei@chromium.org/169293002/590001

[commit-bot: I haz the power, 2014-02-25 07:35:06]

Change committed as 167743