[oilpan] Remove GC_INFO_{DEFINE,DECLARE} macros

[oilpan] Make GC_INFO_{DEFINE,DECLARE} macros NOOP

BUG=None
R=ager@chromium.org, vegorov@chromium.org, zerny@chromium.org

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

[kouhei (in TOK), 2014-02-19 02:02:53]

Would you take a look?

This passes webkit_unit_tests

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

https://codereview.chromium.org/171843002/diff/1/Source/heap/Heap.h#newcode925
Source/heap/Heap.h:925: template<typename Any> friend struct FinalizerTrait;
Maybe this can be made public? all override of this method is declared public.

[haraken, 2014-02-19 02:51:40]

Overall, this looks good to me. Denmark guys should take a look.

Nit: We use an "[oilpan]" prefix for CLs that go to the experimental branch.

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

https://codereview.chromium.org/171843002/diff/1/Source/heap/Visitor.h#newcod...
Source/heap/Visitor.h:521: static char foo = 'a'; // FIXME

foo => type (or className)

Add more description.

[kouhei (in TOK), 2014-02-19 03:08:20]

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

https://codereview.chromium.org/171843002/diff/1/Source/heap/Visitor.h#newcod...
Source/heap/Visitor.h:521: static char foo = 'a'; // FIXME
On 2014/02/19 02:51:40, haraken wrote:
> 
> foo => type (or className)
> 
> Add more description.

@Slava
I want your suggestions here. I'm not sure how we should handle |m_markerType|
here.

This passes tests, but breaks some debug outputs relying on the fact
m_typeMarker == #T.

Some options are:
1. Remove |m_markerType|, and replace its comparisons with &gcInfo comparisons.
2. Introduce something like |extractNameFromFunctionName()| on DEBUG builds. Use
some random unique address on RELEASE.

[zerny-chromium, 2014-02-19 09:44:47]

lgtm

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

https://codereview.chromium.org/171843002/diff/1/Source/heap/Heap.h#newcode925
Source/heap/Heap.h:925: template<typename Any> friend struct FinalizerTrait;
On 2014/02/19 02:02:53, kouhei wrote:
> Maybe this can be made public? all override of this method is declared public.

If this is the only friend declaration we need (ie, none are needed in client
code), then let's keep it protected.

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

https://codereview.chromium.org/171843002/diff/1/Source/heap/Visitor.h#newcod...
Source/heap/Visitor.h:521: static char foo = 'a'; // FIXME
On 2014/02/19 03:08:20, kouhei wrote:
> On 2014/02/19 02:51:40, haraken wrote:
> > 
> > foo => type (or className)
> > 
> > Add more description.
> 
> @Slava
> I want your suggestions here. I'm not sure how we should handle |m_markerType|
> here.
> 
> This passes tests, but breaks some debug outputs relying on the fact
> m_typeMarker == #T.
> 
> Some options are:
> 1. Remove |m_markerType|, and replace its comparisons with &gcInfo
comparisons.
> 2. Introduce something like |extractNameFromFunctionName()| on DEBUG builds.
Use
> some random unique address on RELEASE.

We only need the type marker on debug builds so we can simply remove it
completely from the gcInfo struct on release builds.

For now we need the type marker for at least debug builds with gcc and clang. Do
you know if we can get an |extractNameFromFunctionName()| working for debug on
windows builds too? Would be nice, but it's not as important as the other two.

[kouhei (in TOK), 2014-02-19 11:57:27]

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

https://codereview.chromium.org/171843002/diff/1/Source/heap/Visitor.h#newcod...
Source/heap/Visitor.h:521: static char foo = 'a'; // FIXME
On 2014/02/19 09:44:48, zerny-chromium wrote:
> On 2014/02/19 03:08:20, kouhei wrote:
> > On 2014/02/19 02:51:40, haraken wrote:
> > > 
> > > foo => type (or className)
> > > 
> > > Add more description.
> > 
> > @Slava
> > I want your suggestions here. I'm not sure how we should handle
|m_markerType|
> > here.
> > 
> > This passes tests, but breaks some debug outputs relying on the fact
> > m_typeMarker == #T.
> > 
> > Some options are:
> > 1. Remove |m_markerType|, and replace its comparisons with &gcInfo
> comparisons.
> > 2. Introduce something like |extractNameFromFunctionName()| on DEBUG builds.
> Use
> > some random unique address on RELEASE.
> 
> We only need the type marker on debug builds so we can simply remove it
> completely from the gcInfo struct on release builds.
> 
> For now we need the type marker for at least debug builds with gcc and clang.
Do
> you know if we can get an |extractNameFromFunctionName()| working for debug on
> windows builds too? Would be nice, but it's not as important as the other two.

Thanks for your comment.

Just to clarify, the above &foo will also work on debug, and pass all ASSERTs.
It just breaks some debug message outputs.

I'm not sure why CLASSOF_FUNC(T) and #T are both needed.

[Vyacheslav Egorov (Chromium), 2014-02-19 15:03:31]

LGTM!

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

https://codereview.chromium.org/171843002/diff/1/Source/heap/Visitor.h#newcod...
Source/heap/Visitor.h:521: static char foo = 'a'; // FIXME
CLASSOF_FUNC could delegate to a virtual classOf method that could return a
result more precise than #T (e.g. for Node #T is always Node as a root, but
classOf can return more precise node type).

Given that we ripped off the GC tracing on the trunk branch we should ignore the
issue for now and be satisfied with a random address.

Strictly speaking I would go as far as saying that we should be able to kill
typeMarker entirely because address of the gcInfo itself works as a type marker.
Type markers were introduced prior to introduction of gcInfo and served as a way
to verify consistency between dynamic types and static types of handles. Now
gcInfo address can be used for the same purpose.

Now if we ever want to resurrect GC tracing we can extract type name from
__PRETTY_FUNCTION_ on both gcc and clang and from __FUNCSIG__ on MS CL compiler,
so I think we will be fine.



On 2014/02/19 11:57:27, kouhei wrote:
> On 2014/02/19 09:44:48, zerny-chromium wrote:
> > On 2014/02/19 03:08:20, kouhei wrote:
> > > On 2014/02/19 02:51:40, haraken wrote:
> > > > 
> > > > foo => type (or className)
> > > > 
> > > > Add more description.
> > > 
> > > @Slava
> > > I want your suggestions here. I'm not sure how we should handle
> |m_markerType|
> > > here.
> > > 
> > > This passes tests, but breaks some debug outputs relying on the fact
> > > m_typeMarker == #T.
> > > 
> > > Some options are:
> > > 1. Remove |m_markerType|, and replace its comparisons with &gcInfo
> > comparisons.
> > > 2. Introduce something like |extractNameFromFunctionName()| on DEBUG
builds.
> > Use
> > > some random unique address on RELEASE.
> > 
> > We only need the type marker on debug builds so we can simply remove it
> > completely from the gcInfo struct on release builds.
> > 
> > For now we need the type marker for at least debug builds with gcc and
clang.
> Do
> > you know if we can get an |extractNameFromFunctionName()| working for debug
on
> > windows builds too? Would be nice, but it's not as important as the other
two.
> 
> Thanks for your comment.
> 
> Just to clarify, the above &foo will also work on debug, and pass all ASSERTs.
> It just breaks some debug message outputs.
> 
> I'm not sure why CLASSOF_FUNC(T) and #T are both needed.

[Mads Ager (chromium), 2014-02-19 15:04:56]

LGTM, I like it, thanks Kouhei! :-)

[kouhei (in TOK), 2014-02-20 01:07:41]

Thanks for your feedback!

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

https://codereview.chromium.org/171843002/diff/1/Source/heap/Visitor.h#newcod...
Source/heap/Visitor.h:521: static char foo = 'a'; // FIXME
On 2014/02/19 15:03:31, Vyacheslav Egorov wrote:
> CLASSOF_FUNC could delegate to a virtual classOf method that could return a
> result more precise than #T (e.g. for Node #T is always Node as a root, but
> classOf can return more precise node type).
> 
> Given that we ripped off the GC tracing on the trunk branch we should ignore
the
> issue for now and be satisfied with a random address.
> 
> Strictly speaking I would go as far as saying that we should be able to kill
> typeMarker entirely because address of the gcInfo itself works as a type
marker.
> Type markers were introduced prior to introduction of gcInfo and served as a
way
> to verify consistency between dynamic types and static types of handles. Now
> gcInfo address can be used for the same purpose.
> 
> Now if we ever want to resurrect GC tracing we can extract type name from
> __PRETTY_FUNCTION_ on both gcc and clang and from __FUNCSIG__ on MS CL
compiler,
> so I think we will be fine.
> 
> 
> 
> On 2014/02/19 11:57:27, kouhei wrote:
> > On 2014/02/19 09:44:48, zerny-chromium wrote:
> > > On 2014/02/19 03:08:20, kouhei wrote:
> > > > On 2014/02/19 02:51:40, haraken wrote:
> > > > > 
> > > > > foo => type (or className)
> > > > > 
> > > > > Add more description.
> > > > 
> > > > @Slava
> > > > I want your suggestions here. I'm not sure how we should handle
> > |m_markerType|
> > > > here.
> > > > 
> > > > This passes tests, but breaks some debug outputs relying on the fact
> > > > m_typeMarker == #T.
> > > > 
> > > > Some options are:
> > > > 1. Remove |m_markerType|, and replace its comparisons with &gcInfo
> > > comparisons.
> > > > 2. Introduce something like |extractNameFromFunctionName()| on DEBUG
> builds.
> > > Use
> > > > some random unique address on RELEASE.
> > > 
> > > We only need the type marker on debug builds so we can simply remove it
> > > completely from the gcInfo struct on release builds.
> > > 
> > > For now we need the type marker for at least debug builds with gcc and
> clang.
> > Do
> > > you know if we can get an |extractNameFromFunctionName()| working for
debug
> on
> > > windows builds too? Would be nice, but it's not as important as the other
> two.
> > 
> > Thanks for your comment.
> > 
> > Just to clarify, the above &foo will also work on debug, and pass all
ASSERTs.
> > It just breaks some debug message outputs.
> > 
> > I'm not sure why CLASSOF_FUNC(T) and #T are both needed.
> 

Thanks, Slava!
I'll split work and do the following
1. Land this CL, with |foo| renamed to |pseudoTypeMarker|.
2. Remove all macros CL
3. Remove type marker and use &gcInfo CL.

[kouhei (in TOK), 2014-02-20 06:58:12]

Committed patchset #2 manually as r167456 (presubmit successful).