Oilpan: Forbid adoptRef(X*) where X is RefCountedGarbageCollected

Oilpan: Forbid adoptRef(X*) where X is RefCountedGarbageCollected

Once https://codereview.chromium.org/614373007/ lands, the following code doesn't work as expected (It confuses reference counting).

class X : public RefCountedGarbageCollected<X> {
  static PassRefPtr<X> create() {
    return adoptRef(new X);
  }
}

Instead we have to write:

class X : public RefCountedGarbageCollected<X> {
  static X* create() {
    return new X;
  }
}

This CL adds a compile-time verification to detect adoptRef(X*) where X is RefCountedGarbageCollected.

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

[haraken, 2014-10-12 15:16:44]

haraken@chromium.org changed reviewers:
+ oilpan-reviews@chromium.org, tkent@chromium.org

[haraken, 2014-10-12 15:16:45]

PTAL

https://codereview.chromium.org/647393002/diff/1/Source/wtf/PassRefPtr.h
File Source/wtf/PassRefPtr.h (right):

https://codereview.chromium.org/647393002/diff/1/Source/wtf/PassRefPtr.h#newc...
Source/wtf/PassRefPtr.h:186: template<typename T> class
RefCountedGarbageCollected;

I'm not sure if this forward declaration is allowed (RefCountedGarbageCollected
is defined in platform/).

[sof, 2014-10-13 12:11:30]

sigbjornf@opera.com changed reviewers:
+ sigbjornf@opera.com

[sof, 2014-10-13 12:11:31]

This change is incompatible with what surfaced via
https://codereview.chromium.org/650033002/

Is there a deeper reason why the SVGProperty objects are RefPtr<>-based?

https://codereview.chromium.org/647393002/diff/1/Source/wtf/PassRefPtr.h
File Source/wtf/PassRefPtr.h (right):

https://codereview.chromium.org/647393002/diff/1/Source/wtf/PassRefPtr.h#newc...
Source/wtf/PassRefPtr.h:186: template<typename T> class
RefCountedGarbageCollected;
On 2014/10/12 15:16:45, haraken wrote:
> 
> I'm not sure if this forward declaration is allowed
(RefCountedGarbageCollected
> is defined in platform/).

It doesn't quite set a precedent, TypeTraits.h already brings a blink name into
scope like this.

A compile-time name "use" entirely, so it seems reasonable to allow&enable
checks such as this.

https://codereview.chromium.org/647393002/diff/1/Source/wtf/PassRefPtr.h#newc...
Source/wtf/PassRefPtr.h:186: template<typename T> class
RefCountedGarbageCollected;
Don't think there is a "using" for it (or in scope), so move the forward decl up
a bit and wrap it in "namespace blink { ... }" (and refer to it as
blink::RefCountedGarbageCollected<> below)?

[haraken, 2014-10-13 15:28:55]

On 2014/10/13 12:11:31, sof wrote:
> This change is incompatible with what surfaced via
> https://codereview.chromium.org/650033002/

Yeah, thanks for catching this!

> Is there a deeper reason why the SVGProperty objects are RefPtr<>-based?

Not really... we just haven't yet moved the SVGProperty hierarchy to the heap. I
think we should do it :)


> https://codereview.chromium.org/647393002/diff/1/Source/wtf/PassRefPtr.h
> File Source/wtf/PassRefPtr.h (right):
> 
>
https://codereview.chromium.org/647393002/diff/1/Source/wtf/PassRefPtr.h#newc...
> Source/wtf/PassRefPtr.h:186: template<typename T> class
> RefCountedGarbageCollected;
> On 2014/10/12 15:16:45, haraken wrote:
> > 
> > I'm not sure if this forward declaration is allowed
> (RefCountedGarbageCollected
> > is defined in platform/).
> 
> It doesn't quite set a precedent, TypeTraits.h already brings a blink name
into
> scope like this.
> 
> A compile-time name "use" entirely, so it seems reasonable to allow&enable
> checks such as this.
> 
>
https://codereview.chromium.org/647393002/diff/1/Source/wtf/PassRefPtr.h#newc...
> Source/wtf/PassRefPtr.h:186: template<typename T> class
> RefCountedGarbageCollected;
> Don't think there is a "using" for it (or in scope), so move the forward decl
up
> a bit and wrap it in "namespace blink { ... }" (and refer to it as
> blink::RefCountedGarbageCollected<> below)?

Done.

[sof, 2014-10-14 09:47:01]

On 2014/10/13 15:28:55, haraken wrote:
> On 2014/10/13 12:11:31, sof wrote:
> > This change is incompatible with what surfaced via
> > https://codereview.chromium.org/650033002/
> 
> Yeah, thanks for catching this!
> 
> > Is there a deeper reason why the SVGProperty objects are RefPtr<>-based?
> 
> Not really... we just haven't yet moved the SVGProperty hierarchy to the heap.
I
> think we should do it :)
> 

Good, wasn't sure if earlier svg/ work backed off for some reason. That needs to
happen, then :)

[kouhei (in TOK), 2014-10-14 10:27:59]

On 2014/10/14 09:47:01, sof wrote:
> On 2014/10/13 15:28:55, haraken wrote:
> > On 2014/10/13 12:11:31, sof wrote:
> > > This change is incompatible with what surfaced via
> > > https://codereview.chromium.org/650033002/
> > 
> > Yeah, thanks for catching this!
> > 
> > > Is there a deeper reason why the SVGProperty objects are RefPtr<>-based?
> > 
> > Not really... we just haven't yet moved the SVGProperty hierarchy to the
heap.
> I
> > think we should do it :)
> > 
> 
> Good, wasn't sure if earlier svg/ work backed off for some reason. That needs
to
> happen, then :)

Sorry this is paused in intermediate state. I can either revert the change in
SVG property hierarchy or go forward.
I thought this was blocked on core/ Node decision, as this will introduce a lot
of Persistents to Node hierarchy thus may regress performance.
haraken: Any thoughts on this?

[haraken, 2014-10-14 10:48:01]

On 2014/10/14 10:27:59, kouhei wrote:
> On 2014/10/14 09:47:01, sof wrote:
> > On 2014/10/13 15:28:55, haraken wrote:
> > > On 2014/10/13 12:11:31, sof wrote:
> > > > This change is incompatible with what surfaced via
> > > > https://codereview.chromium.org/650033002/
> > > 
> > > Yeah, thanks for catching this!
> > > 
> > > > Is there a deeper reason why the SVGProperty objects are RefPtr<>-based?
> > > 
> > > Not really... we just haven't yet moved the SVGProperty hierarchy to the
> heap.
> > I
> > > think we should do it :)
> > > 
> > 
> > Good, wasn't sure if earlier svg/ work backed off for some reason. That
needs
> to
> > happen, then :)
> 
> Sorry this is paused in intermediate state. I can either revert the change in
> SVG property hierarchy or go forward.
> I thought this was blocked on core/ Node decision, as this will introduce a
lot
> of Persistents to Node hierarchy thus may regress performance.
> haraken: Any thoughts on this?

I'm not sure of this. What's your concern of moving the SVGProperty hierarchy to
the heap only in oilpan builds? In my understanding, the SVGProperty hierarchy
is tightly coupled with the Node hierarchy, so it makes sense to move it to the
heap with the Node hierarchy.

If I remember things correctly, the reason we didn't move the SVGProperty
hierarchy to the heap was just that we didn't need to move it to the heap to
move the Node hierarchy to the heap. If we could move it to the heap, that would
be better I think.

[Mikhail, 2014-10-14 11:05:09]

mikhail.pozdnyakov@intel.com changed reviewers:
+ mikhail.pozdnyakov@intel.com

[Mikhail, 2014-10-14 11:05:10]

https://codereview.chromium.org/647393002/diff/80001/Source/wtf/PassRefPtr.h
File Source/wtf/PassRefPtr.h (right):

https://codereview.chromium.org/647393002/diff/80001/Source/wtf/PassRefPtr.h#...
Source/wtf/PassRefPtr.h:193: COMPILE_ASSERT(notRefCountedGarbageCollected,
adoptRefIsNotAllowedForRefCountedGarbageCollected);
wouldn't smth like 

template<T> void adoptRef(const RefCountedGarbageCollected<T>*)
{
  static_assert(false, ...);
}

be better? 
(btw this can be put together with the RefCountedGarbageCollected declaration)

[sof, 2014-10-28 21:41:32]

On 2014/10/14 10:48:01, haraken wrote:
> On 2014/10/14 10:27:59, kouhei wrote:
> > On 2014/10/14 09:47:01, sof wrote:
> > > On 2014/10/13 15:28:55, haraken wrote:
> > > > On 2014/10/13 12:11:31, sof wrote:
> > > > > This change is incompatible with what surfaced via
> > > > > https://codereview.chromium.org/650033002/
> > > > 
> > > > Yeah, thanks for catching this!
> > > > 
> > > > > Is there a deeper reason why the SVGProperty objects are
RefPtr<>-based?
> > > > 
> > > > Not really... we just haven't yet moved the SVGProperty hierarchy to the
> > heap.
> > > I
> > > > think we should do it :)
> > > > 
> > > 
> > > Good, wasn't sure if earlier svg/ work backed off for some reason. That
> needs
> > to
> > > happen, then :)
> > 
> > Sorry this is paused in intermediate state. I can either revert the change
in
> > SVG property hierarchy or go forward.
> > I thought this was blocked on core/ Node decision, as this will introduce a
> lot
> > of Persistents to Node hierarchy thus may regress performance.
> > haraken: Any thoughts on this?
> 
> I'm not sure of this. What's your concern of moving the SVGProperty hierarchy
to
> the heap only in oilpan builds? In my understanding, the SVGProperty hierarchy
> is tightly coupled with the Node hierarchy, so it makes sense to move it to
the
> heap with the Node hierarchy.
> 
> If I remember things correctly, the reason we didn't move the SVGProperty
> hierarchy to the heap was just that we didn't need to move it to the heap to
> move the Node hierarchy to the heap. If we could move it to the heap, that
would
> be better I think.

That is a rather large undertaking ( https://codereview.chromium.org/678163002/
), which I'm not so sure we want/need to do at this time.

How about reverting SVGPropertyBase back to deriving from RefCounted<> ?

[haraken, 2014-10-29 00:49:48]

On 2014/10/28 21:41:32, sof wrote:
> On 2014/10/14 10:48:01, haraken wrote:
> > On 2014/10/14 10:27:59, kouhei wrote:
> > > On 2014/10/14 09:47:01, sof wrote:
> > > > On 2014/10/13 15:28:55, haraken wrote:
> > > > > On 2014/10/13 12:11:31, sof wrote:
> > > > > > This change is incompatible with what surfaced via
> > > > > > https://codereview.chromium.org/650033002/
> > > > > 
> > > > > Yeah, thanks for catching this!
> > > > > 
> > > > > > Is there a deeper reason why the SVGProperty objects are
> RefPtr<>-based?
> > > > > 
> > > > > Not really... we just haven't yet moved the SVGProperty hierarchy to
the
> > > heap.
> > > > I
> > > > > think we should do it :)
> > > > > 
> > > > 
> > > > Good, wasn't sure if earlier svg/ work backed off for some reason. That
> > needs
> > > to
> > > > happen, then :)
> > > 
> > > Sorry this is paused in intermediate state. I can either revert the change
> in
> > > SVG property hierarchy or go forward.
> > > I thought this was blocked on core/ Node decision, as this will introduce
a
> > lot
> > > of Persistents to Node hierarchy thus may regress performance.
> > > haraken: Any thoughts on this?
> > 
> > I'm not sure of this. What's your concern of moving the SVGProperty
hierarchy
> to
> > the heap only in oilpan builds? In my understanding, the SVGProperty
hierarchy
> > is tightly coupled with the Node hierarchy, so it makes sense to move it to
> the
> > heap with the Node hierarchy.
> > 
> > If I remember things correctly, the reason we didn't move the SVGProperty
> > hierarchy to the heap was just that we didn't need to move it to the heap to
> > move the Node hierarchy to the heap. If we could move it to the heap, that
> would
> > be better I think.
> 
> That is a rather large undertaking (
https://codereview.chromium.org/678163002/
> ), which I'm not so sure we want/need to do at this time.

That is a great CL btw :)

> How about reverting SVGPropertyBase back to deriving from RefCounted<> ?

Either is fine with me. We need to move the SVGProperty hierarchy to the heap at
some point anyway, so I guess it's just an issue of the order in which we've
done our work.

Would you measure performance of blink_perf.svg (i.e., PerformanceTests/SVG/)
with your CL? If they don't regress, I'm fine with landing the CL now.

[sof, 2014-10-29 06:22:11]

On 2014/10/29 00:49:48, haraken wrote:
> On 2014/10/28 21:41:32, sof wrote:
> > On 2014/10/14 10:48:01, haraken wrote:
> > > On 2014/10/14 10:27:59, kouhei wrote:
> > > > On 2014/10/14 09:47:01, sof wrote:
> > > > > On 2014/10/13 15:28:55, haraken wrote:
> > > > > > On 2014/10/13 12:11:31, sof wrote:
> > > > > > > This change is incompatible with what surfaced via
> > > > > > > https://codereview.chromium.org/650033002/
> > > > > > 
> > > > > > Yeah, thanks for catching this!
> > > > > > 
> > > > > > > Is there a deeper reason why the SVGProperty objects are
> > RefPtr<>-based?
> > > > > > 
> > > > > > Not really... we just haven't yet moved the SVGProperty hierarchy to
> the
> > > > heap.
> > > > > I
> > > > > > think we should do it :)
> > > > > > 
> > > > > 
> > > > > Good, wasn't sure if earlier svg/ work backed off for some reason.
That
> > > needs
> > > > to
> > > > > happen, then :)
> > > > 
> > > > Sorry this is paused in intermediate state. I can either revert the
change
> > in
> > > > SVG property hierarchy or go forward.
> > > > I thought this was blocked on core/ Node decision, as this will
introduce
> a
> > > lot
> > > > of Persistents to Node hierarchy thus may regress performance.
> > > > haraken: Any thoughts on this?
> > > 
> > > I'm not sure of this. What's your concern of moving the SVGProperty
> hierarchy
> > to
> > > the heap only in oilpan builds? In my understanding, the SVGProperty
> hierarchy
> > > is tightly coupled with the Node hierarchy, so it makes sense to move it
to
> > the
> > > heap with the Node hierarchy.
> > > 
> > > If I remember things correctly, the reason we didn't move the SVGProperty
> > > hierarchy to the heap was just that we didn't need to move it to the heap
to
> > > move the Node hierarchy to the heap. If we could move it to the heap, that
> > would
> > > be better I think.
> > 
> > That is a rather large undertaking (
> https://codereview.chromium.org/678163002/
> > ), which I'm not so sure we want/need to do at this time.
> 
> That is a great CL btw :)
> 
> > How about reverting SVGPropertyBase back to deriving from RefCounted<> ?
> 
> Either is fine with me. We need to move the SVGProperty hierarchy to the heap
at
> some point anyway, so I guess it's just an issue of the order in which we've
> done our work.
> 
> Would you measure performance of blink_perf.svg (i.e., PerformanceTests/SVG/)
> with your CL? If they don't regress, I'm fine with landing the CL now.

I can try doing both; i.e., the mild slowdowns for blink_perf.svg might be due
to the use of Persistent<>s for SVGPropertyBase.

[sof, 2014-10-29 09:24:27]

On 2014/10/29 06:22:11, sof wrote:
> On 2014/10/29 00:49:48, haraken wrote:
> > On 2014/10/28 21:41:32, sof wrote:
> > > On 2014/10/14 10:48:01, haraken wrote:
> > > > On 2014/10/14 10:27:59, kouhei wrote:
> > > > > On 2014/10/14 09:47:01, sof wrote:
> > > > > > On 2014/10/13 15:28:55, haraken wrote:
> > > > > > > On 2014/10/13 12:11:31, sof wrote:
> > > > > > > > This change is incompatible with what surfaced via
> > > > > > > > https://codereview.chromium.org/650033002/
> > > > > > > 
> > > > > > > Yeah, thanks for catching this!
> > > > > > > 
> > > > > > > > Is there a deeper reason why the SVGProperty objects are
> > > RefPtr<>-based?
> > > > > > > 
> > > > > > > Not really... we just haven't yet moved the SVGProperty hierarchy
to
> > the
> > > > > heap.
> > > > > > I
> > > > > > > think we should do it :)
> > > > > > > 
> > > > > > 
> > > > > > Good, wasn't sure if earlier svg/ work backed off for some reason.
> That
> > > > needs
> > > > > to
> > > > > > happen, then :)
> > > > > 
> > > > > Sorry this is paused in intermediate state. I can either revert the
> change
> > > in
> > > > > SVG property hierarchy or go forward.
> > > > > I thought this was blocked on core/ Node decision, as this will
> introduce
> > a
> > > > lot
> > > > > of Persistents to Node hierarchy thus may regress performance.
> > > > > haraken: Any thoughts on this?
> > > > 
> > > > I'm not sure of this. What's your concern of moving the SVGProperty
> > hierarchy
> > > to
> > > > the heap only in oilpan builds? In my understanding, the SVGProperty
> > hierarchy
> > > > is tightly coupled with the Node hierarchy, so it makes sense to move it
> to
> > > the
> > > > heap with the Node hierarchy.
> > > > 
> > > > If I remember things correctly, the reason we didn't move the
SVGProperty
> > > > hierarchy to the heap was just that we didn't need to move it to the
heap
> to
> > > > move the Node hierarchy to the heap. If we could move it to the heap,
that
> > > would
> > > > be better I think.
> > > 
> > > That is a rather large undertaking (
> > https://codereview.chromium.org/678163002/
> > > ), which I'm not so sure we want/need to do at this time.
> > 
> > That is a great CL btw :)
> > 
> > > How about reverting SVGPropertyBase back to deriving from RefCounted<> ?
> > 
> > Either is fine with me. We need to move the SVGProperty hierarchy to the
heap
> at
> > some point anyway, so I guess it's just an issue of the order in which we've
> > done our work.
> > 
> > Would you measure performance of blink_perf.svg (i.e.,
PerformanceTests/SVG/)
> > with your CL? If they don't regress, I'm fine with landing the CL now.
> 
> I can try doing both; i.e., the mild slowdowns for blink_perf.svg might be due
> to the use of Persistent<>s for SVGPropertyBase.

http://pastebin.com/0G8TfYuY has the numbers.

[haraken, 2014-10-29 09:30:33]

On 2014/10/29 09:24:27, sof wrote:
> On 2014/10/29 06:22:11, sof wrote:
> > On 2014/10/29 00:49:48, haraken wrote:
> > > On 2014/10/28 21:41:32, sof wrote:
> > > > On 2014/10/14 10:48:01, haraken wrote:
> > > > > On 2014/10/14 10:27:59, kouhei wrote:
> > > > > > On 2014/10/14 09:47:01, sof wrote:
> > > > > > > On 2014/10/13 15:28:55, haraken wrote:
> > > > > > > > On 2014/10/13 12:11:31, sof wrote:
> > > > > > > > > This change is incompatible with what surfaced via
> > > > > > > > > https://codereview.chromium.org/650033002/
> > > > > > > > 
> > > > > > > > Yeah, thanks for catching this!
> > > > > > > > 
> > > > > > > > > Is there a deeper reason why the SVGProperty objects are
> > > > RefPtr<>-based?
> > > > > > > > 
> > > > > > > > Not really... we just haven't yet moved the SVGProperty
hierarchy
> to
> > > the
> > > > > > heap.
> > > > > > > I
> > > > > > > > think we should do it :)
> > > > > > > > 
> > > > > > > 
> > > > > > > Good, wasn't sure if earlier svg/ work backed off for some reason.
> > That
> > > > > needs
> > > > > > to
> > > > > > > happen, then :)
> > > > > > 
> > > > > > Sorry this is paused in intermediate state. I can either revert the
> > change
> > > > in
> > > > > > SVG property hierarchy or go forward.
> > > > > > I thought this was blocked on core/ Node decision, as this will
> > introduce
> > > a
> > > > > lot
> > > > > > of Persistents to Node hierarchy thus may regress performance.
> > > > > > haraken: Any thoughts on this?
> > > > > 
> > > > > I'm not sure of this. What's your concern of moving the SVGProperty
> > > hierarchy
> > > > to
> > > > > the heap only in oilpan builds? In my understanding, the SVGProperty
> > > hierarchy
> > > > > is tightly coupled with the Node hierarchy, so it makes sense to move
it
> > to
> > > > the
> > > > > heap with the Node hierarchy.
> > > > > 
> > > > > If I remember things correctly, the reason we didn't move the
> SVGProperty
> > > > > hierarchy to the heap was just that we didn't need to move it to the
> heap
> > to
> > > > > move the Node hierarchy to the heap. If we could move it to the heap,
> that
> > > > would
> > > > > be better I think.
> > > > 
> > > > That is a rather large undertaking (
> > > https://codereview.chromium.org/678163002/
> > > > ), which I'm not so sure we want/need to do at this time.
> > > 
> > > That is a great CL btw :)
> > > 
> > > > How about reverting SVGPropertyBase back to deriving from RefCounted<> ?
> > > 
> > > Either is fine with me. We need to move the SVGProperty hierarchy to the
> heap
> > at
> > > some point anyway, so I guess it's just an issue of the order in which
we've
> > > done our work.
> > > 
> > > Would you measure performance of blink_perf.svg (i.e.,
> PerformanceTests/SVG/)
> > > with your CL? If they don't regress, I'm fine with landing the CL now.
> > 
> > I can try doing both; i.e., the mild slowdowns for blink_perf.svg might be
due
> > to the use of Persistent<>s for SVGPropertyBase.
> 
> http://pastebin.com/0G8TfYuY has the numbers.

Thanks for collecting the numbers!

Does the result mean that moving the SVGProperty hierarchy to the heap won't
affect performance (not better and not worse)?

In that case, I'd prefer moving the SVGProperty hierarchy to the heap now but
what do you think?

[sof, 2014-10-29 09:53:55]

On 2014/10/29 09:30:33, haraken wrote:
> On 2014/10/29 09:24:27, sof wrote:
> > On 2014/10/29 06:22:11, sof wrote:
> > > On 2014/10/29 00:49:48, haraken wrote:
> > > > On 2014/10/28 21:41:32, sof wrote:
> > > > > On 2014/10/14 10:48:01, haraken wrote:
> > > > > > On 2014/10/14 10:27:59, kouhei wrote:
> > > > > > > On 2014/10/14 09:47:01, sof wrote:
> > > > > > > > On 2014/10/13 15:28:55, haraken wrote:
> > > > > > > > > On 2014/10/13 12:11:31, sof wrote:
> > > > > > > > > > This change is incompatible with what surfaced via
> > > > > > > > > > https://codereview.chromium.org/650033002/
> > > > > > > > > 
> > > > > > > > > Yeah, thanks for catching this!
> > > > > > > > > 
> > > > > > > > > > Is there a deeper reason why the SVGProperty objects are
> > > > > RefPtr<>-based?
> > > > > > > > > 
> > > > > > > > > Not really... we just haven't yet moved the SVGProperty
> hierarchy
> > to
> > > > the
> > > > > > > heap.
> > > > > > > > I
> > > > > > > > > think we should do it :)
> > > > > > > > > 
> > > > > > > > 
> > > > > > > > Good, wasn't sure if earlier svg/ work backed off for some
reason.
> > > That
> > > > > > needs
> > > > > > > to
> > > > > > > > happen, then :)
> > > > > > > 
> > > > > > > Sorry this is paused in intermediate state. I can either revert
the
> > > change
> > > > > in
> > > > > > > SVG property hierarchy or go forward.
> > > > > > > I thought this was blocked on core/ Node decision, as this will
> > > introduce
> > > > a
> > > > > > lot
> > > > > > > of Persistents to Node hierarchy thus may regress performance.
> > > > > > > haraken: Any thoughts on this?
> > > > > > 
> > > > > > I'm not sure of this. What's your concern of moving the SVGProperty
> > > > hierarchy
> > > > > to
> > > > > > the heap only in oilpan builds? In my understanding, the SVGProperty
> > > > hierarchy
> > > > > > is tightly coupled with the Node hierarchy, so it makes sense to
move
> it
> > > to
> > > > > the
> > > > > > heap with the Node hierarchy.
> > > > > > 
> > > > > > If I remember things correctly, the reason we didn't move the
> > SVGProperty
> > > > > > hierarchy to the heap was just that we didn't need to move it to the
> > heap
> > > to
> > > > > > move the Node hierarchy to the heap. If we could move it to the
heap,
> > that
> > > > > would
> > > > > > be better I think.
> > > > > 
> > > > > That is a rather large undertaking (
> > > > https://codereview.chromium.org/678163002/
> > > > > ), which I'm not so sure we want/need to do at this time.
> > > > 
> > > > That is a great CL btw :)
> > > > 
> > > > > How about reverting SVGPropertyBase back to deriving from RefCounted<>
?
> > > > 
> > > > Either is fine with me. We need to move the SVGProperty hierarchy to the
> > heap
> > > at
> > > > some point anyway, so I guess it's just an issue of the order in which
> we've
> > > > done our work.
> > > > 
> > > > Would you measure performance of blink_perf.svg (i.e.,
> > PerformanceTests/SVG/)
> > > > with your CL? If they don't regress, I'm fine with landing the CL now.
> > > 
> > > I can try doing both; i.e., the mild slowdowns for blink_perf.svg might be
> due
> > > to the use of Persistent<>s for SVGPropertyBase.
> > 
> > http://pastebin.com/0G8TfYuY has the numbers.
> 
> Thanks for collecting the numbers!
> 
> Does the result mean that moving the SVGProperty hierarchy to the heap won't
> affect performance (not better and not worse)?
> 
> In that case, I'd prefer moving the SVGProperty hierarchy to the heap now but
> what do you think?

Either option will bring an improvement, so preferable to make some change.

I can't afford to spend a lot of time into getting that (terribly large) CL
landed this week & next, at least. It doesn't have harder parts, but still.

[haraken, 2014-10-29 11:19:39]

On 2014/10/29 09:53:55, sof wrote:
> On 2014/10/29 09:30:33, haraken wrote:
> > On 2014/10/29 09:24:27, sof wrote:
> > > On 2014/10/29 06:22:11, sof wrote:
> > > > On 2014/10/29 00:49:48, haraken wrote:
> > > > > On 2014/10/28 21:41:32, sof wrote:
> > > > > > On 2014/10/14 10:48:01, haraken wrote:
> > > > > > > On 2014/10/14 10:27:59, kouhei wrote:
> > > > > > > > On 2014/10/14 09:47:01, sof wrote:
> > > > > > > > > On 2014/10/13 15:28:55, haraken wrote:
> > > > > > > > > > On 2014/10/13 12:11:31, sof wrote:
> > > > > > > > > > > This change is incompatible with what surfaced via
> > > > > > > > > > > https://codereview.chromium.org/650033002/
> > > > > > > > > > 
> > > > > > > > > > Yeah, thanks for catching this!
> > > > > > > > > > 
> > > > > > > > > > > Is there a deeper reason why the SVGProperty objects are
> > > > > > RefPtr<>-based?
> > > > > > > > > > 
> > > > > > > > > > Not really... we just haven't yet moved the SVGProperty
> > hierarchy
> > > to
> > > > > the
> > > > > > > > heap.
> > > > > > > > > I
> > > > > > > > > > think we should do it :)
> > > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > Good, wasn't sure if earlier svg/ work backed off for some
> reason.
> > > > That
> > > > > > > needs
> > > > > > > > to
> > > > > > > > > happen, then :)
> > > > > > > > 
> > > > > > > > Sorry this is paused in intermediate state. I can either revert
> the
> > > > change
> > > > > > in
> > > > > > > > SVG property hierarchy or go forward.
> > > > > > > > I thought this was blocked on core/ Node decision, as this will
> > > > introduce
> > > > > a
> > > > > > > lot
> > > > > > > > of Persistents to Node hierarchy thus may regress performance.
> > > > > > > > haraken: Any thoughts on this?
> > > > > > > 
> > > > > > > I'm not sure of this. What's your concern of moving the
SVGProperty
> > > > > hierarchy
> > > > > > to
> > > > > > > the heap only in oilpan builds? In my understanding, the
SVGProperty
> > > > > hierarchy
> > > > > > > is tightly coupled with the Node hierarchy, so it makes sense to
> move
> > it
> > > > to
> > > > > > the
> > > > > > > heap with the Node hierarchy.
> > > > > > > 
> > > > > > > If I remember things correctly, the reason we didn't move the
> > > SVGProperty
> > > > > > > hierarchy to the heap was just that we didn't need to move it to
the
> > > heap
> > > > to
> > > > > > > move the Node hierarchy to the heap. If we could move it to the
> heap,
> > > that
> > > > > > would
> > > > > > > be better I think.
> > > > > > 
> > > > > > That is a rather large undertaking (
> > > > > https://codereview.chromium.org/678163002/
> > > > > > ), which I'm not so sure we want/need to do at this time.
> > > > > 
> > > > > That is a great CL btw :)
> > > > > 
> > > > > > How about reverting SVGPropertyBase back to deriving from
RefCounted<>
> ?
> > > > > 
> > > > > Either is fine with me. We need to move the SVGProperty hierarchy to
the
> > > heap
> > > > at
> > > > > some point anyway, so I guess it's just an issue of the order in which
> > we've
> > > > > done our work.
> > > > > 
> > > > > Would you measure performance of blink_perf.svg (i.e.,
> > > PerformanceTests/SVG/)
> > > > > with your CL? If they don't regress, I'm fine with landing the CL now.
> > > > 
> > > > I can try doing both; i.e., the mild slowdowns for blink_perf.svg might
be
> > due
> > > > to the use of Persistent<>s for SVGPropertyBase.
> > > 
> > > http://pastebin.com/0G8TfYuY has the numbers.
> > 
> > Thanks for collecting the numbers!
> > 
> > Does the result mean that moving the SVGProperty hierarchy to the heap won't
> > affect performance (not better and not worse)?
> > 
> > In that case, I'd prefer moving the SVGProperty hierarchy to the heap now
but
> > what do you think?
> 
> Either option will bring an improvement, so preferable to make some change.
> 
> I can't afford to spend a lot of time into getting that (terribly large) CL
> landed this week & next, at least. It doesn't have harder parts, but still.

Great, then let's move the SVGProperty hierarchy to the heap.

I don't have much time to review it either this week & next though :-)

[sof, 2014-10-29 14:58:57]

On 2014/10/29 11:19:39, haraken wrote:
> On 2014/10/29 09:53:55, sof wrote:
> > On 2014/10/29 09:30:33, haraken wrote:
> > > On 2014/10/29 09:24:27, sof wrote:
> > > > On 2014/10/29 06:22:11, sof wrote:
> > > > > On 2014/10/29 00:49:48, haraken wrote:
> > > > > > On 2014/10/28 21:41:32, sof wrote:
> > > > > > > On 2014/10/14 10:48:01, haraken wrote:
> > > > > > > > On 2014/10/14 10:27:59, kouhei wrote:
> > > > > > > > > On 2014/10/14 09:47:01, sof wrote:
> > > > > > > > > > On 2014/10/13 15:28:55, haraken wrote:
> > > > > > > > > > > On 2014/10/13 12:11:31, sof wrote:
> > > > > > > > > > > > This change is incompatible with what surfaced via
> > > > > > > > > > > > https://codereview.chromium.org/650033002/
> > > > > > > > > > > 
> > > > > > > > > > > Yeah, thanks for catching this!
> > > > > > > > > > > 
> > > > > > > > > > > > Is there a deeper reason why the SVGProperty objects are
> > > > > > > RefPtr<>-based?
> > > > > > > > > > > 
> > > > > > > > > > > Not really... we just haven't yet moved the SVGProperty
> > > hierarchy
> > > > to
> > > > > > the
> > > > > > > > > heap.
> > > > > > > > > > I
> > > > > > > > > > > think we should do it :)
> > > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > Good, wasn't sure if earlier svg/ work backed off for some
> > reason.
> > > > > That
> > > > > > > > needs
> > > > > > > > > to
> > > > > > > > > > happen, then :)
> > > > > > > > > 
> > > > > > > > > Sorry this is paused in intermediate state. I can either
revert
> > the
> > > > > change
> > > > > > > in
> > > > > > > > > SVG property hierarchy or go forward.
> > > > > > > > > I thought this was blocked on core/ Node decision, as this
will
> > > > > introduce
> > > > > > a
> > > > > > > > lot
> > > > > > > > > of Persistents to Node hierarchy thus may regress performance.
> > > > > > > > > haraken: Any thoughts on this?
> > > > > > > > 
> > > > > > > > I'm not sure of this. What's your concern of moving the
> SVGProperty
> > > > > > hierarchy
> > > > > > > to
> > > > > > > > the heap only in oilpan builds? In my understanding, the
> SVGProperty
> > > > > > hierarchy
> > > > > > > > is tightly coupled with the Node hierarchy, so it makes sense to
> > move
> > > it
> > > > > to
> > > > > > > the
> > > > > > > > heap with the Node hierarchy.
> > > > > > > > 
> > > > > > > > If I remember things correctly, the reason we didn't move the
> > > > SVGProperty
> > > > > > > > hierarchy to the heap was just that we didn't need to move it to
> the
> > > > heap
> > > > > to
> > > > > > > > move the Node hierarchy to the heap. If we could move it to the
> > heap,
> > > > that
> > > > > > > would
> > > > > > > > be better I think.
> > > > > > > 
> > > > > > > That is a rather large undertaking (
> > > > > > https://codereview.chromium.org/678163002/
> > > > > > > ), which I'm not so sure we want/need to do at this time.
> > > > > > 
> > > > > > That is a great CL btw :)
> > > > > > 
> > > > > > > How about reverting SVGPropertyBase back to deriving from
> RefCounted<>
> > ?
> > > > > > 
> > > > > > Either is fine with me. We need to move the SVGProperty hierarchy to
> the
> > > > heap
> > > > > at
> > > > > > some point anyway, so I guess it's just an issue of the order in
which
> > > we've
> > > > > > done our work.
> > > > > > 
> > > > > > Would you measure performance of blink_perf.svg (i.e.,
> > > > PerformanceTests/SVG/)
> > > > > > with your CL? If they don't regress, I'm fine with landing the CL
now.
> > > > > 
> > > > > I can try doing both; i.e., the mild slowdowns for blink_perf.svg
might
> be
> > > due
> > > > > to the use of Persistent<>s for SVGPropertyBase.
> > > > 
> > > > http://pastebin.com/0G8TfYuY has the numbers.
> > > 
> > > Thanks for collecting the numbers!
> > > 
> > > Does the result mean that moving the SVGProperty hierarchy to the heap
won't
> > > affect performance (not better and not worse)?
> > > 
> > > In that case, I'd prefer moving the SVGProperty hierarchy to the heap now
> but
> > > what do you think?
> > 
> > Either option will bring an improvement, so preferable to make some change.
> > 
> > I can't afford to spend a lot of time into getting that (terribly large) CL
> > landed this week & next, at least. It doesn't have harder parts, but still.
> 
> Great, then let's move the SVGProperty hierarchy to the heap.
> 
> I don't have much time to review it either this week & next though :-)

Right. https://codereview.chromium.org/686163002/ switches back to RefCounted<>
until there is time; it'll improve blink_perf.svg numbers and allow this CL to
go ahead.

(Please ignore the Oilpan bot redness; ToT is busted atm.)

[haraken, 2014-10-29 15:41:00]

On 2014/10/29 14:58:57, sof wrote:
> On 2014/10/29 11:19:39, haraken wrote:
> > On 2014/10/29 09:53:55, sof wrote:
> > > On 2014/10/29 09:30:33, haraken wrote:
> > > > On 2014/10/29 09:24:27, sof wrote:
> > > > > On 2014/10/29 06:22:11, sof wrote:
> > > > > > On 2014/10/29 00:49:48, haraken wrote:
> > > > > > > On 2014/10/28 21:41:32, sof wrote:
> > > > > > > > On 2014/10/14 10:48:01, haraken wrote:
> > > > > > > > > On 2014/10/14 10:27:59, kouhei wrote:
> > > > > > > > > > On 2014/10/14 09:47:01, sof wrote:
> > > > > > > > > > > On 2014/10/13 15:28:55, haraken wrote:
> > > > > > > > > > > > On 2014/10/13 12:11:31, sof wrote:
> > > > > > > > > > > > > This change is incompatible with what surfaced via
> > > > > > > > > > > > > https://codereview.chromium.org/650033002/
> > > > > > > > > > > > 
> > > > > > > > > > > > Yeah, thanks for catching this!
> > > > > > > > > > > > 
> > > > > > > > > > > > > Is there a deeper reason why the SVGProperty objects
are
> > > > > > > > RefPtr<>-based?
> > > > > > > > > > > > 
> > > > > > > > > > > > Not really... we just haven't yet moved the SVGProperty
> > > > hierarchy
> > > > > to
> > > > > > > the
> > > > > > > > > > heap.
> > > > > > > > > > > I
> > > > > > > > > > > > think we should do it :)
> > > > > > > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > > > Good, wasn't sure if earlier svg/ work backed off for some
> > > reason.
> > > > > > That
> > > > > > > > > needs
> > > > > > > > > > to
> > > > > > > > > > > happen, then :)
> > > > > > > > > > 
> > > > > > > > > > Sorry this is paused in intermediate state. I can either
> revert
> > > the
> > > > > > change
> > > > > > > > in
> > > > > > > > > > SVG property hierarchy or go forward.
> > > > > > > > > > I thought this was blocked on core/ Node decision, as this
> will
> > > > > > introduce
> > > > > > > a
> > > > > > > > > lot
> > > > > > > > > > of Persistents to Node hierarchy thus may regress
performance.
> > > > > > > > > > haraken: Any thoughts on this?
> > > > > > > > > 
> > > > > > > > > I'm not sure of this. What's your concern of moving the
> > SVGProperty
> > > > > > > hierarchy
> > > > > > > > to
> > > > > > > > > the heap only in oilpan builds? In my understanding, the
> > SVGProperty
> > > > > > > hierarchy
> > > > > > > > > is tightly coupled with the Node hierarchy, so it makes sense
to
> > > move
> > > > it
> > > > > > to
> > > > > > > > the
> > > > > > > > > heap with the Node hierarchy.
> > > > > > > > > 
> > > > > > > > > If I remember things correctly, the reason we didn't move the
> > > > > SVGProperty
> > > > > > > > > hierarchy to the heap was just that we didn't need to move it
to
> > the
> > > > > heap
> > > > > > to
> > > > > > > > > move the Node hierarchy to the heap. If we could move it to
the
> > > heap,
> > > > > that
> > > > > > > > would
> > > > > > > > > be better I think.
> > > > > > > > 
> > > > > > > > That is a rather large undertaking (
> > > > > > > https://codereview.chromium.org/678163002/
> > > > > > > > ), which I'm not so sure we want/need to do at this time.
> > > > > > > 
> > > > > > > That is a great CL btw :)
> > > > > > > 
> > > > > > > > How about reverting SVGPropertyBase back to deriving from
> > RefCounted<>
> > > ?
> > > > > > > 
> > > > > > > Either is fine with me. We need to move the SVGProperty hierarchy
to
> > the
> > > > > heap
> > > > > > at
> > > > > > > some point anyway, so I guess it's just an issue of the order in
> which
> > > > we've
> > > > > > > done our work.
> > > > > > > 
> > > > > > > Would you measure performance of blink_perf.svg (i.e.,
> > > > > PerformanceTests/SVG/)
> > > > > > > with your CL? If they don't regress, I'm fine with landing the CL
> now.
> > > > > > 
> > > > > > I can try doing both; i.e., the mild slowdowns for blink_perf.svg
> might
> > be
> > > > due
> > > > > > to the use of Persistent<>s for SVGPropertyBase.
> > > > > 
> > > > > http://pastebin.com/0G8TfYuY has the numbers.
> > > > 
> > > > Thanks for collecting the numbers!
> > > > 
> > > > Does the result mean that moving the SVGProperty hierarchy to the heap
> won't
> > > > affect performance (not better and not worse)?
> > > > 
> > > > In that case, I'd prefer moving the SVGProperty hierarchy to the heap
now
> > but
> > > > what do you think?
> > > 
> > > Either option will bring an improvement, so preferable to make some
change.
> > > 
> > > I can't afford to spend a lot of time into getting that (terribly large)
CL
> > > landed this week & next, at least. It doesn't have harder parts, but
still.
> > 
> > Great, then let's move the SVGProperty hierarchy to the heap.
> > 
> > I don't have much time to review it either this week & next though :-)
> 
> Right. https://codereview.chromium.org/686163002/ switches back to
RefCounted<>
> until there is time; it'll improve blink_perf.svg numbers and allow this CL to
> go ahead.

That sounds like a better plan. LGed the CL :)

[sof, 2014-10-29 15:46:30]

On 2014/10/29 15:41:00, haraken wrote:
> On 2014/10/29 14:58:57, sof wrote:
> > On 2014/10/29 11:19:39, haraken wrote:
> > > On 2014/10/29 09:53:55, sof wrote:
> > > > On 2014/10/29 09:30:33, haraken wrote:
> > > > > On 2014/10/29 09:24:27, sof wrote:
> > > > > > On 2014/10/29 06:22:11, sof wrote:
> > > > > > > On 2014/10/29 00:49:48, haraken wrote:
> > > > > > > > On 2014/10/28 21:41:32, sof wrote:
> > > > > > > > > On 2014/10/14 10:48:01, haraken wrote:
> > > > > > > > > > On 2014/10/14 10:27:59, kouhei wrote:
> > > > > > > > > > > On 2014/10/14 09:47:01, sof wrote:
> > > > > > > > > > > > On 2014/10/13 15:28:55, haraken wrote:
> > > > > > > > > > > > > On 2014/10/13 12:11:31, sof wrote:
> > > > > > > > > > > > > > This change is incompatible with what surfaced via
> > > > > > > > > > > > > > https://codereview.chromium.org/650033002/
> > > > > > > > > > > > > 
> > > > > > > > > > > > > Yeah, thanks for catching this!
> > > > > > > > > > > > > 
> > > > > > > > > > > > > > Is there a deeper reason why the SVGProperty objects
> are
> > > > > > > > > RefPtr<>-based?
> > > > > > > > > > > > > 
> > > > > > > > > > > > > Not really... we just haven't yet moved the
SVGProperty
> > > > > hierarchy
> > > > > > to
> > > > > > > > the
> > > > > > > > > > > heap.
> > > > > > > > > > > > I
> > > > > > > > > > > > > think we should do it :)
> > > > > > > > > > > > > 
> > > > > > > > > > > > 
> > > > > > > > > > > > Good, wasn't sure if earlier svg/ work backed off for
some
> > > > reason.
> > > > > > > That
> > > > > > > > > > needs
> > > > > > > > > > > to
> > > > > > > > > > > > happen, then :)
> > > > > > > > > > > 
> > > > > > > > > > > Sorry this is paused in intermediate state. I can either
> > revert
> > > > the
> > > > > > > change
> > > > > > > > > in
> > > > > > > > > > > SVG property hierarchy or go forward.
> > > > > > > > > > > I thought this was blocked on core/ Node decision, as this
> > will
> > > > > > > introduce
> > > > > > > > a
> > > > > > > > > > lot
> > > > > > > > > > > of Persistents to Node hierarchy thus may regress
> performance.
> > > > > > > > > > > haraken: Any thoughts on this?
> > > > > > > > > > 
> > > > > > > > > > I'm not sure of this. What's your concern of moving the
> > > SVGProperty
> > > > > > > > hierarchy
> > > > > > > > > to
> > > > > > > > > > the heap only in oilpan builds? In my understanding, the
> > > SVGProperty
> > > > > > > > hierarchy
> > > > > > > > > > is tightly coupled with the Node hierarchy, so it makes
sense
> to
> > > > move
> > > > > it
> > > > > > > to
> > > > > > > > > the
> > > > > > > > > > heap with the Node hierarchy.
> > > > > > > > > > 
> > > > > > > > > > If I remember things correctly, the reason we didn't move
the
> > > > > > SVGProperty
> > > > > > > > > > hierarchy to the heap was just that we didn't need to move
it
> to
> > > the
> > > > > > heap
> > > > > > > to
> > > > > > > > > > move the Node hierarchy to the heap. If we could move it to
> the
> > > > heap,
> > > > > > that
> > > > > > > > > would
> > > > > > > > > > be better I think.
> > > > > > > > > 
> > > > > > > > > That is a rather large undertaking (
> > > > > > > > https://codereview.chromium.org/678163002/
> > > > > > > > > ), which I'm not so sure we want/need to do at this time.
> > > > > > > > 
> > > > > > > > That is a great CL btw :)
> > > > > > > > 
> > > > > > > > > How about reverting SVGPropertyBase back to deriving from
> > > RefCounted<>
> > > > ?
> > > > > > > > 
> > > > > > > > Either is fine with me. We need to move the SVGProperty
hierarchy
> to
> > > the
> > > > > > heap
> > > > > > > at
> > > > > > > > some point anyway, so I guess it's just an issue of the order in
> > which
> > > > > we've
> > > > > > > > done our work.
> > > > > > > > 
> > > > > > > > Would you measure performance of blink_perf.svg (i.e.,
> > > > > > PerformanceTests/SVG/)
> > > > > > > > with your CL? If they don't regress, I'm fine with landing the
CL
> > now.
> > > > > > > 
> > > > > > > I can try doing both; i.e., the mild slowdowns for blink_perf.svg
> > might
> > > be
> > > > > due
> > > > > > > to the use of Persistent<>s for SVGPropertyBase.
> > > > > > 
> > > > > > http://pastebin.com/0G8TfYuY has the numbers.
> > > > > 
> > > > > Thanks for collecting the numbers!
> > > > > 
> > > > > Does the result mean that moving the SVGProperty hierarchy to the heap
> > won't
> > > > > affect performance (not better and not worse)?
> > > > > 
> > > > > In that case, I'd prefer moving the SVGProperty hierarchy to the heap
> now
> > > but
> > > > > what do you think?
> > > > 
> > > > Either option will bring an improvement, so preferable to make some
> change.
> > > > 
> > > > I can't afford to spend a lot of time into getting that (terribly large)
> CL
> > > > landed this week & next, at least. It doesn't have harder parts, but
> still.
> > > 
> > > Great, then let's move the SVGProperty hierarchy to the heap.
> > > 
> > > I don't have much time to review it either this week & next though :-)
> > 
> > Right. https://codereview.chromium.org/686163002/ switches back to
> RefCounted<>
> > until there is time; it'll improve blink_perf.svg numbers and allow this CL
to
> > go ahead.
> 
> That sounds like a better plan. LGed the CL :)

great; once that CL lands, this one can hopefully get wtf/ approval and go
ahead.

[haraken, 2014-11-10 05:05:16]

tkent-san: Would you take a look at this CL?

[haraken, 2014-11-10 05:05:52]

On 2014/11/10 05:05:16, haraken wrote:
> tkent-san: Would you take a look at this CL?

sof's CL (https://codereview.chromium.org/686163002/) has been landed, so I
think it's safe to land this CL.

[tkent, 2014-11-10 06:06:59]

https://codereview.chromium.org/647393002/diff/80001/Source/wtf/PassRefPtr.h
File Source/wtf/PassRefPtr.h (right):

https://codereview.chromium.org/647393002/diff/80001/Source/wtf/PassRefPtr.h#...
Source/wtf/PassRefPtr.h:193: COMPILE_ASSERT(notRefCountedGarbageCollected,
adoptRefIsNotAllowedForRefCountedGarbageCollected);
On 2014/10/14 11:05:10, Mikhail wrote:
> wouldn't smth like 
> 
> template<T> void adoptRef(const RefCountedGarbageCollected<T>*)
> {
>   static_assert(false, ...);
> }
> 
> be better? 
> (btw this can be put together with the RefCountedGarbageCollected declaration)

Mikhail's idea looks better.
This patch has a layering violation.  WTF should have no knowledge of
blink::RefCountedGarbageCollected.

[haraken, 2014-11-10 06:41:57]

https://codereview.chromium.org/647393002/diff/80001/Source/wtf/PassRefPtr.h
File Source/wtf/PassRefPtr.h (right):

https://codereview.chromium.org/647393002/diff/80001/Source/wtf/PassRefPtr.h#...
Source/wtf/PassRefPtr.h:193: COMPILE_ASSERT(notRefCountedGarbageCollected,
adoptRefIsNotAllowedForRefCountedGarbageCollected);
On 2014/11/10 06:06:59, tkent wrote:
> On 2014/10/14 11:05:10, Mikhail wrote:
> > wouldn't smth like 
> > 
> > template<T> void adoptRef(const RefCountedGarbageCollected<T>*)
> > {
> >   static_assert(false, ...);
> > }
> > 
> > be better? 
> > (btw this can be put together with the RefCountedGarbageCollected
declaration)
> 
> Mikhail's idea looks better.
> This patch has a layering violation.  WTF should have no knowledge of
> blink::RefCountedGarbageCollected.
> 

Hmm, it looks like I need your help.

When I add:

  template<typename T>
  PassRefPtr<T> adoptRef(RefCountedGarbageCollected<T>*)
  {
    COMPILE_ASSERT(false, ...);
  }

to Handle.h, I always hit the assert when including Handle.h, like this:

In file included from
../../third_party/WebKit/Source/web/painting/ContinuousPainter.cpp:33:
In file included from
../../third_party/WebKit/Source/platform/graphics/GraphicsLayer.h:38:
In file included from
../../third_party/WebKit/Source/platform/graphics/GraphicsLayerDebugInfo.h:39:
In file included from
../../third_party/WebKit/public/platform/WebGraphicsLayerDebugInfo.h:34:
In file included from ../../third_party/WebKit/public/platform/WebString.h:35:
In file included from
../../third_party/WebKit/public/platform/WebPrivatePtr.h:37:
../../third_party/WebKit/Source/platform/heap/Handle.h:967:5: error:
static_assert failed "adoptRefIsNotAllowedForRefCountedGarbageCollected"
    COMPILE_ASSERT(false, adoptRefIsNotAllowedForRefCountedGarbageCollected);
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

How can I avoid hitting the assert?

[Mikhail, 2014-11-10 10:10:57]

https://codereview.chromium.org/647393002/diff/80001/Source/wtf/PassRefPtr.h
File Source/wtf/PassRefPtr.h (right):

https://codereview.chromium.org/647393002/diff/80001/Source/wtf/PassRefPtr.h#...
Source/wtf/PassRefPtr.h:193: COMPILE_ASSERT(notRefCountedGarbageCollected,
adoptRefIsNotAllowedForRefCountedGarbageCollected);
On 2014/11/10 06:41:56, haraken wrote:
> On 2014/11/10 06:06:59, tkent wrote:
> > On 2014/10/14 11:05:10, Mikhail wrote:
> > > wouldn't smth like 
> > > 
> > > template<T> void adoptRef(const RefCountedGarbageCollected<T>*)
> > > {
> > >   static_assert(false, ...);
> > > }
> > > 
> > > be better? 
> > > (btw this can be put together with the RefCountedGarbageCollected
> declaration)
> > 
> > Mikhail's idea looks better.
> > This patch has a layering violation.  WTF should have no knowledge of
> > blink::RefCountedGarbageCollected.
> > 
> 
> Hmm, it looks like I need your help.
> 
> When I add:
> 
>   template<typename T>
>   PassRefPtr<T> adoptRef(RefCountedGarbageCollected<T>*)
>   {
>     COMPILE_ASSERT(false, ...);
>   }
> 
> to Handle.h, I always hit the assert when including Handle.h, like this:
> 
> In file included from
> ../../third_party/WebKit/Source/web/painting/ContinuousPainter.cpp:33:
> In file included from
> ../../third_party/WebKit/Source/platform/graphics/GraphicsLayer.h:38:
> In file included from
> ../../third_party/WebKit/Source/platform/graphics/GraphicsLayerDebugInfo.h:39:
> In file included from
> ../../third_party/WebKit/public/platform/WebGraphicsLayerDebugInfo.h:34:
> In file included from ../../third_party/WebKit/public/platform/WebString.h:35:
> In file included from
> ../../third_party/WebKit/public/platform/WebPrivatePtr.h:37:
> ../../third_party/WebKit/Source/platform/heap/Handle.h:967:5: error:
> static_assert failed "adoptRefIsNotAllowedForRefCountedGarbageCollected"
>     COMPILE_ASSERT(false, adoptRefIsNotAllowedForRefCountedGarbageCollected);
>     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> How can I avoid hitting the assert?
yeah, sorry this trick does not work with functions, it's compiled even if it's
not used :( The easy solution which I've on top of my head is

template<typename T>
PassRefPtr<T> adoptRef(RefCountedGarbageCollected<T>*) = delete;

[haraken, 2014-11-10 11:40:51]

On 2014/11/10 10:10:57, Mikhail wrote:
> https://codereview.chromium.org/647393002/diff/80001/Source/wtf/PassRefPtr.h
> File Source/wtf/PassRefPtr.h (right):
> 
>
https://codereview.chromium.org/647393002/diff/80001/Source/wtf/PassRefPtr.h#...
> Source/wtf/PassRefPtr.h:193: COMPILE_ASSERT(notRefCountedGarbageCollected,
> adoptRefIsNotAllowedForRefCountedGarbageCollected);
> On 2014/11/10 06:41:56, haraken wrote:
> > On 2014/11/10 06:06:59, tkent wrote:
> > > On 2014/10/14 11:05:10, Mikhail wrote:
> > > > wouldn't smth like 
> > > > 
> > > > template<T> void adoptRef(const RefCountedGarbageCollected<T>*)
> > > > {
> > > >   static_assert(false, ...);
> > > > }
> > > > 
> > > > be better? 
> > > > (btw this can be put together with the RefCountedGarbageCollected
> > declaration)
> > > 
> > > Mikhail's idea looks better.
> > > This patch has a layering violation.  WTF should have no knowledge of
> > > blink::RefCountedGarbageCollected.
> > > 
> > 
> > Hmm, it looks like I need your help.
> > 
> > When I add:
> > 
> >   template<typename T>
> >   PassRefPtr<T> adoptRef(RefCountedGarbageCollected<T>*)
> >   {
> >     COMPILE_ASSERT(false, ...);
> >   }
> > 
> > to Handle.h, I always hit the assert when including Handle.h, like this:
> > 
> > In file included from
> > ../../third_party/WebKit/Source/web/painting/ContinuousPainter.cpp:33:
> > In file included from
> > ../../third_party/WebKit/Source/platform/graphics/GraphicsLayer.h:38:
> > In file included from
> >
../../third_party/WebKit/Source/platform/graphics/GraphicsLayerDebugInfo.h:39:
> > In file included from
> > ../../third_party/WebKit/public/platform/WebGraphicsLayerDebugInfo.h:34:
> > In file included from
../../third_party/WebKit/public/platform/WebString.h:35:
> > In file included from
> > ../../third_party/WebKit/public/platform/WebPrivatePtr.h:37:
> > ../../third_party/WebKit/Source/platform/heap/Handle.h:967:5: error:
> > static_assert failed "adoptRefIsNotAllowedForRefCountedGarbageCollected"
> >     COMPILE_ASSERT(false,
adoptRefIsNotAllowedForRefCountedGarbageCollected);
> >     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > 
> > How can I avoid hitting the assert?
> yeah, sorry this trick does not work with functions, it's compiled even if
it's
> not used :( The easy solution which I've on top of my head is
> 
> template<typename T>
> PassRefPtr<T> adoptRef(RefCountedGarbageCollected<T>*) = delete;

Thanks, done! PTAL.

[Mikhail, 2014-11-10 11:54:02]

lgtm

[haraken, 2014-11-10 12:13:07]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-11-10 12:13:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/647393002/130001

[commit-bot: I haz the power, 2014-11-10 12:32:07]

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

[commit-bot: I haz the power, 2014-11-10 12:32:09]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/3...)

[haraken, 2014-11-11 06:05:24]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-11-11 06:05:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/647393002/150001

[commit-bot: I haz the power, 2014-11-11 06:25:45]

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

[commit-bot: I haz the power, 2014-11-11 06:25:47]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/35748)

[haraken, 2014-11-11 08:55:41]

Mikhail: Hmm, the patch set 5 doesn't compile on Windows... Do you have any idea
on this?

[Mikhail, 2014-11-11 10:02:54]

On 2014/11/11 08:55:41, haraken wrote:
> Mikhail: Hmm, the patch set 5 doesn't compile on Windows... Do you have any
idea
> on this?

Unfortunately I do not have a Win machine to try things out.
The errors look like:

error C2784: 'WTF::PassRefPtr<T>
blink::adoptRef(blink::RefCountedGarbageCollected<T> *)' : could not deduce
template argument for 'blink::RefCountedGarbageCollected<T> *' from
'blink::Document *'
       
e:\b\build\slave\win_layout\build\src\third_party\webkit\source\platform\heap\handle.h(965)
: see declaration of 'blink::adoptRef'
       
e:\b\build\slave\win_layout\build\src\third_party\webkit\source\core\dom\document.h(226)
: see reference to function template instantiation 'WTF::PassRefPtr<T>
blink::adoptRefWillBeNoop<blink::Document>(T *)' being compiled
        with
        [
            T=blink::Document
        ]

but Document is not inherited from RefCountedGarbageCollected, right? so the
usual WTF::adoptRef should have been picked for it.

First I'd try fix the namespace for 
PassRefPtr<T> adoptRef(RefCountedGarbageCollected<T>*) -- it apparently should
not be declared within 'blink' namespace (sorry for not noticing it before!).

[haraken, 2014-11-11 11:22:50]

On 2014/11/11 10:02:54, Mikhail wrote:
> On 2014/11/11 08:55:41, haraken wrote:
> > Mikhail: Hmm, the patch set 5 doesn't compile on Windows... Do you have any
> idea
> > on this?
> 
> Unfortunately I do not have a Win machine to try things out.
> The errors look like:
> 
> error C2784: 'WTF::PassRefPtr<T>
> blink::adoptRef(blink::RefCountedGarbageCollected<T> *)' : could not deduce
> template argument for 'blink::RefCountedGarbageCollected<T> *' from
> 'blink::Document *'
>        
>
e:\b\build\slave\win_layout\build\src\third_party\webkit\source\platform\heap\handle.h(965)
> : see declaration of 'blink::adoptRef'
>        
>
e:\b\build\slave\win_layout\build\src\third_party\webkit\source\core\dom\document.h(226)
> : see reference to function template instantiation 'WTF::PassRefPtr<T>
> blink::adoptRefWillBeNoop<blink::Document>(T *)' being compiled
>         with
>         [
>             T=blink::Document
>         ]
> 
> but Document is not inherited from RefCountedGarbageCollected, right? so the
> usual WTF::adoptRef should have been picked for it.
> 
> First I'd try fix the namespace for 
> PassRefPtr<T> adoptRef(RefCountedGarbageCollected<T>*) -- it apparently should
> not be declared within 'blink' namespace (sorry for not noticing it before!).

Thanks for the advice! (Sorry I'm not familiar with C++ intrinsics...)

Trying win bots.

[Mikhail, 2014-11-11 12:41:28]

On 2014/11/11 11:22:50, haraken wrote:
> On 2014/11/11 10:02:54, Mikhail wrote:
> > On 2014/11/11 08:55:41, haraken wrote:
> > > Mikhail: Hmm, the patch set 5 doesn't compile on Windows... Do you have
any
> > idea
> > > on this?
> > 
> > Unfortunately I do not have a Win machine to try things out.
> > The errors look like:
> > 
> > error C2784: 'WTF::PassRefPtr<T>
> > blink::adoptRef(blink::RefCountedGarbageCollected<T> *)' : could not deduce
> > template argument for 'blink::RefCountedGarbageCollected<T> *' from
> > 'blink::Document *'
> >        
> >
>
e:\b\build\slave\win_layout\build\src\third_party\webkit\source\platform\heap\handle.h(965)
> > : see declaration of 'blink::adoptRef'
> >        
> >
>
e:\b\build\slave\win_layout\build\src\third_party\webkit\source\core\dom\document.h(226)
> > : see reference to function template instantiation 'WTF::PassRefPtr<T>
> > blink::adoptRefWillBeNoop<blink::Document>(T *)' being compiled
> >         with
> >         [
> >             T=blink::Document
> >         ]
> > 
> > but Document is not inherited from RefCountedGarbageCollected, right? so the
> > usual WTF::adoptRef should have been picked for it.
> > 
> > First I'd try fix the namespace for 
> > PassRefPtr<T> adoptRef(RefCountedGarbageCollected<T>*) -- it apparently
should
> > not be declared within 'blink' namespace (sorry for not noticing it
before!).
> 
> Thanks for the advice! (Sorry I'm not familiar with C++ intrinsics...)
> 
> Trying win bots.

Looks like it compiles now :)

[haraken, 2014-11-11 12:58:40]

The CQ bit was checked by haraken@chromium.org

[haraken, 2014-11-11 12:58:48]

On 2014/11/11 12:41:28, Mikhail wrote:
> On 2014/11/11 11:22:50, haraken wrote:
> > On 2014/11/11 10:02:54, Mikhail wrote:
> > > On 2014/11/11 08:55:41, haraken wrote:
> > > > Mikhail: Hmm, the patch set 5 doesn't compile on Windows... Do you have
> any
> > > idea
> > > > on this?
> > > 
> > > Unfortunately I do not have a Win machine to try things out.
> > > The errors look like:
> > > 
> > > error C2784: 'WTF::PassRefPtr<T>
> > > blink::adoptRef(blink::RefCountedGarbageCollected<T> *)' : could not
deduce
> > > template argument for 'blink::RefCountedGarbageCollected<T> *' from
> > > 'blink::Document *'
> > >        
> > >
> >
>
e:\b\build\slave\win_layout\build\src\third_party\webkit\source\platform\heap\handle.h(965)
> > > : see declaration of 'blink::adoptRef'
> > >        
> > >
> >
>
e:\b\build\slave\win_layout\build\src\third_party\webkit\source\core\dom\document.h(226)
> > > : see reference to function template instantiation 'WTF::PassRefPtr<T>
> > > blink::adoptRefWillBeNoop<blink::Document>(T *)' being compiled
> > >         with
> > >         [
> > >             T=blink::Document
> > >         ]
> > > 
> > > but Document is not inherited from RefCountedGarbageCollected, right? so
the
> > > usual WTF::adoptRef should have been picked for it.
> > > 
> > > First I'd try fix the namespace for 
> > > PassRefPtr<T> adoptRef(RefCountedGarbageCollected<T>*) -- it apparently
> should
> > > not be declared within 'blink' namespace (sorry for not noticing it
> before!).
> > 
> > Thanks for the advice! (Sorry I'm not familiar with C++ intrinsics...)
> > 
> > Trying win bots.
> 
> Looks like it compiles now :)

Thanks for the help!

[commit-bot: I haz the power, 2014-11-11 12:59:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/647393002/170001

[commit-bot: I haz the power, 2014-11-11 14:56:12]

Committed patchset #6 (id:170001) as 185124