Turn ViewportAnchors into stack allocated objects.

Turn ViewportAnchors into stack allocated objects.

The viewport anchors have stack lifetimes, so rephrase their
implementation and use slightly so as to be able to allocate
them on the stack.

The main benefit of doing so is that the references they
contain to Oilpan heap objects will then be correctly traced
should an Oilpan conservative GC be needed while resizing
the view.

R=bokan,haraken
BUG=340522
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=192586

[sof, 2015-03-25 18:57:24]

sigbjornf@opera.com changed reviewers:
+ bokan@chromium.org, oilpan-reviews@chromium.org

[sof, 2015-03-25 18:57:24]

Please take a look.

Following up on https://codereview.chromium.org/1002423003/#msg6

bokan@: this CL may look a bit frivolous in nature; in case it does, let me try
to motivate it a bit: as it is on trunk,

 - ViewportAnchor contains a pair of references.
 - With Oilpan, these point to objects both on its GCed heap. 
 - ViewportAnchors are OwnPtr<> allocated in resize().
 - Should a conservative GC happen while resizing, the garbage collector will
walk the stack and pick out references that point into the heap being GCed.
 - The OwnPtr<> reference will only be followed as far as seeing that it refers
to something not on the Oilpan heap & what it points to will not be traced and
accounted for (*).
 - We risk not identifying those two ViewportAnchors as alive and may finalize
them as part of the GC. Which would be bad.
 - In https://codereview.chromium.org/1002423003/ we worked around that by
wrapping up another Node reference in a Persistent<> reference -- Persistent<>s
reside off heap, but the object they refer to on the Oilpan heap will be kept
alive by them (i.e., it's a persistent, strong reference.)
 - Persistent<> have overhead (and requires sep. allocation) and should be used
with some care, so adding another two Persistent<>s to ViewportAnchor is
something we should try to avoid.
 - ViewportAnchor usage have stack lifetime here, and the use of OwnPtr<>s is
really a code structuring device for how they're instantiated & used in
WebViewImpl::resize().

By putting the ViewportAnchor entirely on the stack, we can avoid the above
unsafety and have the garbage collector be able to locate all references that
the anchors contain.

* - you may argue that the objects will be reachable by other references that
are on the stack and thus this isn't a practical problem; they'll remain alive.
Maybe so, but that kind of reasoning just gets complex and unsatisfactory as the
code evolves. _And_ by not accounting for all object references, we limit
ourselves to only certain kinds of GC algorithms / systems.

[bokan, 2015-03-25 19:30:33]

On 2015/03/25 18:57:24, sof wrote:
> Please take a look.
> 
> Following up on https://codereview.chromium.org/1002423003/#msg6
> 
> bokan@: this CL may look a bit frivolous in nature; in case it does, let me
try
> to motivate it a bit: as it is on trunk,
> 
>  - ViewportAnchor contains a pair of references.
>  - With Oilpan, these point to objects both on its GCed heap. 
>  - ViewportAnchors are OwnPtr<> allocated in resize().
>  - Should a conservative GC happen while resizing, the garbage collector will
> walk the stack and pick out references that point into the heap being GCed.
>  - The OwnPtr<> reference will only be followed as far as seeing that it
refers
> to something not on the Oilpan heap & what it points to will not be traced and
> accounted for (*).
>  - We risk not identifying those two ViewportAnchors as alive and may finalize
> them as part of the GC. Which would be bad.
>  - In https://codereview.chromium.org/1002423003/ we worked around that by
> wrapping up another Node reference in a Persistent<> reference --
Persistent<>s
> reside off heap, but the object they refer to on the Oilpan heap will be kept
> alive by them (i.e., it's a persistent, strong reference.)
>  - Persistent<> have overhead (and requires sep. allocation) and should be
used
> with some care, so adding another two Persistent<>s to ViewportAnchor is
> something we should try to avoid.
>  - ViewportAnchor usage have stack lifetime here, and the use of OwnPtr<>s is
> really a code structuring device for how they're instantiated & used in
> WebViewImpl::resize().
> 
> By putting the ViewportAnchor entirely on the stack, we can avoid the above
> unsafety and have the garbage collector be able to locate all references that
> the anchors contain.
> 
> * - you may argue that the objects will be reachable by other references that
> are on the stack and thus this isn't a practical problem; they'll remain
alive.
> Maybe so, but that kind of reasoning just gets complex and unsatisfactory as
the
> code evolves. _And_ by not accounting for all object references, we limit
> ourselves to only certain kinds of GC algorithms / systems.

Hi Sigbjorn,

Thanks for the detailed explanation! I originally tried to keep these stack
allocated but, as the small heap allocation in a non-hot code path was cleaner,
it was deemed a premature optimization in review. The Oilpan case makes sense to
me though (and this is clearer than what I had...) so lgtm! Though I'm not an
OWNER.

[sof, 2015-03-25 19:52:03]

On 2015/03/25 19:30:33, bokan wrote:
> On 2015/03/25 18:57:24, sof wrote:
> > Please take a look.
> > 
> > Following up on https://codereview.chromium.org/1002423003/#msg6
> > 
> > bokan@: this CL may look a bit frivolous in nature; in case it does, let me
> try
> > to motivate it a bit: as it is on trunk,
> > 
> >  - ViewportAnchor contains a pair of references.
> >  - With Oilpan, these point to objects both on its GCed heap. 
> >  - ViewportAnchors are OwnPtr<> allocated in resize().
> >  - Should a conservative GC happen while resizing, the garbage collector
will
> > walk the stack and pick out references that point into the heap being GCed.
> >  - The OwnPtr<> reference will only be followed as far as seeing that it
> refers
> > to something not on the Oilpan heap & what it points to will not be traced
and
> > accounted for (*).
> >  - We risk not identifying those two ViewportAnchors as alive and may
finalize
> > them as part of the GC. Which would be bad.
> >  - In https://codereview.chromium.org/1002423003/ we worked around that by
> > wrapping up another Node reference in a Persistent<> reference --
> Persistent<>s
> > reside off heap, but the object they refer to on the Oilpan heap will be
kept
> > alive by them (i.e., it's a persistent, strong reference.)
> >  - Persistent<> have overhead (and requires sep. allocation) and should be
> used
> > with some care, so adding another two Persistent<>s to ViewportAnchor is
> > something we should try to avoid.
> >  - ViewportAnchor usage have stack lifetime here, and the use of OwnPtr<>s
is
> > really a code structuring device for how they're instantiated & used in
> > WebViewImpl::resize().
> > 
> > By putting the ViewportAnchor entirely on the stack, we can avoid the above
> > unsafety and have the garbage collector be able to locate all references
that
> > the anchors contain.
> > 
> > * - you may argue that the objects will be reachable by other references
that
> > are on the stack and thus this isn't a practical problem; they'll remain
> alive.
> > Maybe so, but that kind of reasoning just gets complex and unsatisfactory as
> the
> > code evolves. _And_ by not accounting for all object references, we limit
> > ourselves to only certain kinds of GC algorithms / systems.
> 
> Hi Sigbjorn,
> 
> Thanks for the detailed explanation! I originally tried to keep these stack
> allocated but, as the small heap allocation in a non-hot code path was
cleaner,
> it was deemed a premature optimization in review. The Oilpan case makes sense
to
> me though (and this is clearer than what I had...) so lgtm! Though I'm not an
> OWNER.

Thanks for the review, that's good feedback to have. Don't want to mutate well
working code without at least trying to explain why :)

[haraken, 2015-03-26 01:53:40]

LGTM, nice change.

[sof, 2015-03-26 06:59:28]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2015-03-26 06:59:44]

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

[commit-bot: I haz the power, 2015-03-26 07:03:14]

Committed patchset #1 (id:1) as
https://src.chromium.org/viewvc/blink?view=rev&revision=192586