Extend destructor checks to account for eagerly finalized class types.

Extend destructor checks to account for eagerly finalized class types.

Classes annotated as being eagerly finalized are allowed to touch other
heap objects in their destructors, provided these are not also eagerly
finalized. Extend the CheckFinalizerVisitor implementation to take that
into account

R=haraken
BUG=491488
Committed: https://crrev.com/0cb71ff110ab4c8f7b06e70879c0b0e4a426c318
Cr-Commit-Position: refs/heads/master@{#331331}

[haraken, 2015-05-25 00:19:21]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2015-05-25 00:19:21]

LGTM

You might want to replace all existing USE_PRE_FINALIZER()s with
EAGERLY_FINALIZED() and confirm that this CL works for them (because it takes
time to roll a GC plugin so it seems better to confirm it in advance).

[haraken, 2015-05-25 00:19:32]

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

[sof, 2015-05-25 05:52:48]

On 2015/05/25 00:19:21, haraken wrote:
> LGTM
> 
> You might want to replace all existing USE_PRE_FINALIZER()s with
> EAGERLY_FINALIZED() and confirm that this CL works for them (because it takes
> time to roll a GC plugin so it seems better to confirm it in advance).

Yes, doing that along with checking ASan status is the next step to validate
this switch in approach.

(Public holiday here today so things will be slower, but will do it in the
background.)

[sof, 2015-05-26 06:12:22]

On 2015/05/25 05:52:48, sof wrote:
> On 2015/05/25 00:19:21, haraken wrote:
> > LGTM
> > 
> > You might want to replace all existing USE_PRE_FINALIZER()s with
> > EAGERLY_FINALIZED() and confirm that this CL works for them (because it
takes
> > time to roll a GC plugin so it seems better to confirm it in advance).
> 
> Yes, doing that along with checking ASan status is the next step to validate
> this switch in approach.
> 
> (Public holiday here today so things will be slower, but will do it in the
> background.)

ASan and other testing only turned up a problem with switching AudioNode to be
eagerly finalized, others can be recast without trouble.

AudioNode cannot as it depends on accessing other eagerly finalized objects
while finalizing. However, those objects (LifecycleObserver instances) will no
longer have to be eagerly finalized when LifecycleObserver is always on the
heap. AudioNode can become an eagerly finalized object then & its prefinalizer
removed.

Hence, will go forward with this and other changes related to eager
finalization.

[haraken, 2015-05-26 06:18:07]

On 2015/05/26 06:12:22, sof wrote:
> On 2015/05/25 05:52:48, sof wrote:
> > On 2015/05/25 00:19:21, haraken wrote:
> > > LGTM
> > > 
> > > You might want to replace all existing USE_PRE_FINALIZER()s with
> > > EAGERLY_FINALIZED() and confirm that this CL works for them (because it
> takes
> > > time to roll a GC plugin so it seems better to confirm it in advance).
> > 
> > Yes, doing that along with checking ASan status is the next step to validate
> > this switch in approach.
> > 
> > (Public holiday here today so things will be slower, but will do it in the
> > background.)
> 
> ASan and other testing only turned up a problem with switching AudioNode to be
> eagerly finalized, others can be recast without trouble.
> 
> AudioNode cannot as it depends on accessing other eagerly finalized objects
> while finalizing. However, those objects (LifecycleObserver instances) will no
> longer have to be eagerly finalized when LifecycleObserver is always on the
> heap. AudioNode can become an eagerly finalized object then & its prefinalizer
> removed.
> 
> Hence, will go forward with this and other changes related to eager
> finalization.

Great!

[sof, 2015-05-26 06:18:11]

The CQ bit was checked by sigbjornf@opera.com

[sof, 2015-05-26 06:18:12]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1158623002/#ps20001
(title: "updated to look for IsEagerlyFinalizedMarker instead")

[commit-bot: I haz the power, 2015-05-26 06:18:19]

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

[sof, 2015-05-26 06:28:16]

hans@, thakis@: getting this GC plugin change rolled out would unblock work on
Oilpan stabilization - could you queue it up to be included in the next
iteration, please? tia.

[commit-bot: I haz the power, 2015-05-26 07:08:46]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-05-26 07:10:43]

Patchset 2 (id:??) landed as
https://crrev.com/0cb71ff110ab4c8f7b06e70879c0b0e4a426c318
Cr-Commit-Position: refs/heads/master@{#331331}

[hans, 2015-05-26 17:30:39]

On 2015/05/26 06:28:16, sof wrote:
> hans@, thakis@: getting this GC plugin change rolled out would unblock work on
> Oilpan stabilization - could you queue it up to be included in the next
> iteration, please? tia.

We build the plugins from tip-of-tree Chromium when rolling out new Clang
versions, so this entered the "queue" when it got committed.

Feel free to ping me if the next Clang roll drags out and you need this sooner.

[sof, 2015-05-26 18:35:52]

On 2015/05/26 17:30:39, hans wrote:
> On 2015/05/26 06:28:16, sof wrote:
> > hans@, thakis@: getting this GC plugin change rolled out would unblock work
on
> > Oilpan stabilization - could you queue it up to be included in the next
> > iteration, please? tia.
> 
> We build the plugins from tip-of-tree Chromium when rolling out new Clang
> versions, so this entered the "queue" when it got committed.
> 
> Feel free to ping me if the next Clang roll drags out and you need this
sooner.

Thanks. 

If https://codereview.chromium.org/1145043003 doesn't stick, could you consider
including this one? Low risk & blocks stabilization work.

[Nico, 2015-05-26 18:47:09]

it'll totally stick it'll totally stick it'll totally stick lalalalala

(more seriously, it's already attempt 3 or 4 – enough that I can't count
them anymore. So it's at least somewhat likely to stick)

On Tue, May 26, 2015 at 11:35 AM, <sigbjornf@opera.com> wrote:

> On 2015/05/26 17:30:39, hans wrote:
>
>> On 2015/05/26 06:28:16, sof wrote:
>> > hans@, thakis@: getting this GC plugin change rolled out would unblock
>> work
>>
> on
>
>> > Oilpan stabilization - could you queue it up to be included in the next
>> > iteration, please? tia.
>>
>
>  We build the plugins from tip-of-tree Chromium when rolling out new Clang
>> versions, so this entered the "queue" when it got committed.
>>
>
>  Feel free to ping me if the next Clang roll drags out and you need this
>>
> sooner.
>
> Thanks.
>
> If https://codereview.chromium.org/1145043003 doesn't stick, could you
> consider
> including this one? Low risk & blocks stabilization work.
>
>
>
> https://codereview.chromium.org/1158623002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sof, 2015-05-28 07:08:01]

On 2015/05/26 18:47:09, Nico wrote:
> it'll totally stick it'll totally stick it'll totally stick lalalalala
> 
> (more seriously, it's already attempt 3 or 4 – enough that I can't count
> them anymore. So it's at least somewhat likely to stick)
> 

Looks like that incantation did the trick? :)

We're a bit stuck without that GC plugin rolled, so if it could happen sooner,
that'd be helpful.

[Nico, 2015-05-28 07:21:48]

On Thu, May 28, 2015 at 12:08 AM, <sigbjornf@opera.com> wrote:

> On 2015/05/26 18:47:09, Nico wrote:
>
>> it'll totally stick it'll totally stick it'll totally stick lalalalala
>>
>
>  (more seriously, it's already attempt 3 or 4 – enough that I can't count
>> them anymore. So it's at least somewhat likely to stick)
>>
>
>
> Looks like that incantation did the trick? :)
>
> We're a bit stuck without that GC plugin rolled, so if it could happen
> sooner,
> that'd be helpful.
>

How stuck? Is next week enough? In that case, star
http://code.google.com/p/chromium/issues/detail?id=492898 . Else, I can
push just a new plugin on Friday (and in return something other thing won't
happen).


>
>
> https://codereview.chromium.org/1158623002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sof, 2015-05-28 07:38:26]

On 2015/05/28 07:21:48, Nico wrote:
> On Thu, May 28, 2015 at 12:08 AM, <mailto:sigbjornf@opera.com> wrote:
> 
> > On 2015/05/26 18:47:09, Nico wrote:
> >
> >> it'll totally stick it'll totally stick it'll totally stick lalalalala
> >>
> >
> >  (more seriously, it's already attempt 3 or 4 – enough that I can't count
> >> them anymore. So it's at least somewhat likely to stick)
> >>
> >
> >
> > Looks like that incantation did the trick? :)
> >
> > We're a bit stuck without that GC plugin rolled, so if it could happen
> > sooner,
> > that'd be helpful.
> >
> 
> How stuck? Is next week enough? In that case, star
> http://code.google.com/p/chromium/issues/detail?id=492898 . Else, I can
> push just a new plugin on Friday (and in return something other thing won't
> happen).
> 

Sometime next week would work out great for Oilpan -- we're reworking some
aspects of finalization, which is needed before Oilpan can ship "more fully."

(Thanks for the quick followups, sorry to come a-begging for help each time.)

[haraken, 2015-06-02 00:36:07]

On 2015/05/28 07:38:26, sof wrote:
> On 2015/05/28 07:21:48, Nico wrote:
> > On Thu, May 28, 2015 at 12:08 AM, <mailto:sigbjornf@opera.com> wrote:
> > 
> > > On 2015/05/26 18:47:09, Nico wrote:
> > >
> > >> it'll totally stick it'll totally stick it'll totally stick lalalalala
> > >>
> > >
> > >  (more seriously, it's already attempt 3 or 4 – enough that I can't count
> > >> them anymore. So it's at least somewhat likely to stick)
> > >>
> > >
> > >
> > > Looks like that incantation did the trick? :)
> > >
> > > We're a bit stuck without that GC plugin rolled, so if it could happen
> > > sooner,
> > > that'd be helpful.
> > >
> > 
> > How stuck? Is next week enough? In that case, star
> > http://code.google.com/p/chromium/issues/detail?id=492898 . Else, I can
> > push just a new plugin on Friday (and in return something other thing won't
> > happen).
> > 
> 
> Sometime next week would work out great for Oilpan -- we're reworking some
> aspects of finalization, which is needed before Oilpan can ship "more fully."
> 
> (Thanks for the quick followups, sorry to come a-begging for help each time.)

Any update on this?

[Nico, 2015-06-02 00:45:32]

Hans prepared a new clang package last Friday, but the script to upload it
to goma seems to be broken at the moment. We're waiting for goma team to
get back to us – maybe you can walk by their desks and ask :-)
On Jun 1, 2015 5:36 PM, <haraken@chromium.org> wrote:

> On 2015/05/28 07:38:26, sof wrote:
>
>> On 2015/05/28 07:21:48, Nico wrote:
>> > On Thu, May 28, 2015 at 12:08 AM, <mailto:sigbjornf@opera.com> wrote:
>> >
>> > > On 2015/05/26 18:47:09, Nico wrote:
>> > >
>> > >> it'll totally stick it'll totally stick it'll totally stick
>> lalalalala
>> > >>
>> > >
>> > >  (more seriously, it's already attempt 3 or 4 – enough that I can't
>> count
>> > >> them anymore. So it's at least somewhat likely to stick)
>> > >>
>> > >
>> > >
>> > > Looks like that incantation did the trick? :)
>> > >
>> > > We're a bit stuck without that GC plugin rolled, so if it could happen
>> > > sooner,
>> > > that'd be helpful.
>> > >
>> >
>> > How stuck? Is next week enough? In that case, star
>> > http://code.google.com/p/chromium/issues/detail?id=492898 . Else, I can
>> > push just a new plugin on Friday (and in return something other thing
>> won't
>> > happen).
>> >
>>
>
>  Sometime next week would work out great for Oilpan -- we're reworking some
>> aspects of finalization, which is needed before Oilpan can ship "more
>> fully."
>>
>
>  (Thanks for the quick followups, sorry to come a-begging for help each
>> time.)
>>
>
> Any update on this?
>
>
> https://codereview.chromium.org/1158623002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Nico, 2015-06-03 20:09:27]

We landed a compiler with this plugin change yesterday night, you should be
unblocked now.

On Mon, Jun 1, 2015 at 5:45 PM, Nico Weber <thakis@chromium.org> wrote:

> Hans prepared a new clang package last Friday, but the script to upload it
> to goma seems to be broken at the moment. We're waiting for goma team to
> get back to us – maybe you can walk by their desks and ask :-)
> On Jun 1, 2015 5:36 PM, <haraken@chromium.org> wrote:
>
>> On 2015/05/28 07:38:26, sof wrote:
>>
>>> On 2015/05/28 07:21:48, Nico wrote:
>>> > On Thu, May 28, 2015 at 12:08 AM, <mailto:sigbjornf@opera.com> wrote:
>>> >
>>> > > On 2015/05/26 18:47:09, Nico wrote:
>>> > >
>>> > >> it'll totally stick it'll totally stick it'll totally stick
>>> lalalalala
>>> > >>
>>> > >
>>> > >  (more seriously, it's already attempt 3 or 4 – enough that I can't
>>> count
>>> > >> them anymore. So it's at least somewhat likely to stick)
>>> > >>
>>> > >
>>> > >
>>> > > Looks like that incantation did the trick? :)
>>> > >
>>> > > We're a bit stuck without that GC plugin rolled, so if it could
>>> happen
>>> > > sooner,
>>> > > that'd be helpful.
>>> > >
>>> >
>>> > How stuck? Is next week enough? In that case, star
>>> > http://code.google.com/p/chromium/issues/detail?id=492898 . Else, I
>>> can
>>> > push just a new plugin on Friday (and in return something other thing
>>> won't
>>> > happen).
>>> >
>>>
>>
>>  Sometime next week would work out great for Oilpan -- we're reworking
>>> some
>>> aspects of finalization, which is needed before Oilpan can ship "more
>>> fully."
>>>
>>
>>  (Thanks for the quick followups, sorry to come a-begging for help each
>>> time.)
>>>
>>
>> Any update on this?
>>
>>
>> https://codereview.chromium.org/1158623002/
>>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sof, 2015-06-03 20:57:12]

On 2015/06/03 20:09:27, Nico wrote:
> We landed a compiler with this plugin change yesterday night, you should be
> unblocked now.
> 

Oh, yes :) Thanks for the heads up, I failed to notice that earlier today.
Great; that unblocks a few things by now.

https://codereview.chromium.org/1151163002/#ps20001 's linux_blink_oilpan_dbg
build is one place where the updated GC plugin is being put to good use.