Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2322253002:
Use WTF::WeakPtr in WTF::makeCancellable (Closed)
DescriptionUse WTF::WeakPtr in WTF::makeCancellable
This CL modifies the cancellation mechanism of makeCancellable to use
WeakPtr, instead of custom impl, so that base::Callback::IsCancelled
work with the cancellation.
Patch Set 1 #Patch Set 2 : +test #
Depends on Patchset: Messages
Total messages: 39 (9 generated)
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Use WTF::WeakPtr in WTF::makeCancellable BUG= ========== to ========== Use WTF::WeakPtr in WTF::makeCancellable This CL modifies the cancellation mechanism of makeCancellable to use WeakPtr, instead of custom impl, so that base::Callback::IsCancelled work with the cancellation. ==========
tzik@chromium.org changed reviewers: + alexclarke@chromium.org, dcheng@chromium.org
Alex, Daniel: We've decided to use just WeakPtr for the task cancellation, rather than postCancellableTask(). Then, I think makeCancellable is still useful as a wrapper of WeakPtr-based cancellation. WDYT? Can we reactivate it? FYI: here is an example usage of it: https://codereview.chromium.org/2328683003/
One of the canonical examples is this: auto result = makeCancellable(bind(&Foo::bar, wrapPersistent(foo))); But that could just be written as: auto result = bind(&Foo::bar, wrapWeakPersistent(foo))); right? Which is shorter and also avoids an additional heap allocations for the extra state. In the linked example, the same thing could be accomplished by using WeakPtrFactory directly and also results in fewer object allocations, unless I'm missing something. In Chromium, just using WeakPtrFactory has worked pretty well: while there is a CancelableClosure helper in //base... looking at those, I think most of those could probably just use WeakPtrFactory.
On 2016/09/09 14:53:39, tzik wrote: > Alex, Daniel: We've decided to use just WeakPtr for the task cancellation, > rather than postCancellableTask(). > Then, I think makeCancellable is still useful as a wrapper of WeakPtr-based > cancellation. > > WDYT? Can we reactivate it? > FYI: here is an example usage of it: https://codereview.chromium.org/2328683003/ I'm not sure it makes sense to have that and https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/sched... Is there some use case CancellableTaskFactory doesn't support? It was intended to be a drop in replacement for timers.
On 2016/09/09 19:47:43, alex clarke wrote: > On 2016/09/09 14:53:39, tzik wrote: > > Alex, Daniel: We've decided to use just WeakPtr for the task cancellation, > > rather than postCancellableTask(). > > Then, I think makeCancellable is still useful as a wrapper of WeakPtr-based > > cancellation. > > > > WDYT? Can we reactivate it? > > FYI: here is an example usage of it: > https://codereview.chromium.org/2328683003/ > > I'm not sure it makes sense to have that and > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/sched... > > Is there some use case CancellableTaskFactory doesn't support? It was intended > to be a drop in replacement for timers. Of course times have changed since CancellableTaskFactory was written. If we choose to keep it, the inner CancellableTask should go away and instead we would post a closure binding CancellableTaskFactory::run, m_weakPtrFactory.createWeakPtr().
On 2016/09/09 19:22:19, dcheng wrote:
> One of the canonical examples is this:
>
> auto result = makeCancellable(bind(&Foo::bar, wrapPersistent(foo)));
>
> But that could just be written as:
>
> auto result = bind(&Foo::bar, wrapWeakPersistent(foo)));
>
> right? Which is shorter and also avoids an additional heap allocations for the
> extra state.
>
> In the linked example, the same thing could be accomplished by using
> WeakPtrFactory directly and also results in fewer object allocations, unless
I'm
> missing something.
>
> In Chromium, just using WeakPtrFactory has worked pretty well: while there is
a
> CancelableClosure helper in //base... looking at those, I think most of those
> could probably just use WeakPtrFactory.
I have several concern about cancellation by the direct use of WeakPersistent or
WeakPtr.
1.
On the cancellation by WeakPersistent, we have no way to invalidate and cancel
the task before its lifetime, some of ActiveDOMObjects need to cancel their
pending tasks on ActiveDOMObject::stop().
2.
The direct use of WeakPtrFactory on GCed object is probably unsafe due to the
lazy sweep. After the object gets unreachable and before it gets sweeped, there
is a period that we can't touch the object and WeakPtr is still valid.
3.
Also, some classes have multiple cancellable tasks. (e.g. Document has 6
Timer<Document> as its member). They will have multiple WeakPtrFactory for each,
that looks confusing to me.
4.
Another is the lifetime of the bound objects. After the cancellation on raw
Weak{Ptr,Persistent}, bound objects keeps alive until the Callback itself is
destroyed. Not sure it matters a lot, but existing cancellation mechanisms seem
to clear objects depended by the task on the cancellation. To preserve the
previous behavior, we should clear the bound objects on the cancellation.
5.
The receiver retention and the cancellation are orthogonal, IMO. We might need a
cancellable task with strong reference to the object, or a cancellable task on a
non-member function.
# Though I don't find the example for this. :p
On 2016/09/12 07:45:51, tzik wrote: > On 2016/09/09 19:22:19, dcheng wrote: > > One of the canonical examples is this: > > > > auto result = makeCancellable(bind(&Foo::bar, wrapPersistent(foo))); > > > > But that could just be written as: > > > > auto result = bind(&Foo::bar, wrapWeakPersistent(foo))); > > > > right? Which is shorter and also avoids an additional heap allocations for the > > extra state. > > > > In the linked example, the same thing could be accomplished by using > > WeakPtrFactory directly and also results in fewer object allocations, unless > I'm > > missing something. > > > > In Chromium, just using WeakPtrFactory has worked pretty well: while there is > a > > CancelableClosure helper in //base... looking at those, I think most of those > > could probably just use WeakPtrFactory. > > I have several concern about cancellation by the direct use of WeakPersistent or > WeakPtr. > > 1. > On the cancellation by WeakPersistent, we have no way to invalidate and cancel > the task before its lifetime, some of ActiveDOMObjects need to cancel their > pending tasks on ActiveDOMObject::stop(). > > 2. > The direct use of WeakPtrFactory on GCed object is probably unsafe due to the > lazy sweep. After the object gets unreachable and before it gets sweeped, there > is a period that we can't touch the object and WeakPtr is still valid. TimerBase::runInternal has some logic to work around this. It checks * TimerIsObjectAliveTrait<TimerFiredClass>::isHeapObjectAlive(m_object) to see if the object is going away and doesn't fire. A possibly bad idea: Get WTF::bind or CancellableTaskFactory or FunctionCancellerImpl::run or to perform the same check. * https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/Timer... > > 3. > Also, some classes have multiple cancellable tasks. (e.g. Document has 6 > Timer<Document> as its member). They will have multiple WeakPtrFactory for each, > that looks confusing to me. > > 4. > Another is the lifetime of the bound objects. After the cancellation on raw > Weak{Ptr,Persistent}, bound objects keeps alive until the Callback itself is > destroyed. Not sure it matters a lot, but existing cancellation mechanisms seem > to clear objects depended by the task on the cancellation. To preserve the > previous behavior, we should clear the bound objects on the cancellation. > > 5. > The receiver retention and the cancellation are orthogonal, IMO. We might need a > cancellable task with strong reference to the object, or a cancellable task on a > non-member function. > # Though I don't find the example for this. :p
On 2016/09/12 15:00:06, alex clarke wrote: > On 2016/09/12 07:45:51, tzik wrote: > > On 2016/09/09 19:22:19, dcheng wrote: > > > One of the canonical examples is this: > > > > > > auto result = makeCancellable(bind(&Foo::bar, wrapPersistent(foo))); > > > > > > But that could just be written as: > > > > > > auto result = bind(&Foo::bar, wrapWeakPersistent(foo))); > > > > > > right? Which is shorter and also avoids an additional heap allocations for > the > > > extra state. > > > > > > In the linked example, the same thing could be accomplished by using > > > WeakPtrFactory directly and also results in fewer object allocations, unless > > I'm > > > missing something. > > > > > > In Chromium, just using WeakPtrFactory has worked pretty well: while there > is > > a > > > CancelableClosure helper in //base... looking at those, I think most of > those > > > could probably just use WeakPtrFactory. > > > > I have several concern about cancellation by the direct use of WeakPersistent > or > > WeakPtr. > > > > 1. > > On the cancellation by WeakPersistent, we have no way to invalidate and cancel > > the task before its lifetime, some of ActiveDOMObjects need to cancel their > > pending tasks on ActiveDOMObject::stop(). > > > > 2. > > The direct use of WeakPtrFactory on GCed object is probably unsafe due to the > > lazy sweep. After the object gets unreachable and before it gets sweeped, > there > > is a period that we can't touch the object and WeakPtr is still valid. > > TimerBase::runInternal has some logic to work around this. It checks > * TimerIsObjectAliveTrait<TimerFiredClass>::isHeapObjectAlive(m_object) to see > if > the object is going away and doesn't fire. > > A possibly bad idea: Get WTF::bind or CancellableTaskFactory or > FunctionCancellerImpl::run or to perform the same check. > > * > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/Timer... The isHeapObject() check is a terrible hack :) CancellableTaskFactory shouldn't need that hack because it's using WeakPersistent to the owning object. If the owning object is dead, the WeakPersistent is cleared and thus the callback doesn't run; i.e., the existing CancellableTaskFactory doesn't have the lazy-sweeping problem. > > > > > > > 3. > > Also, some classes have multiple cancellable tasks. (e.g. Document has 6 > > Timer<Document> as its member). They will have multiple WeakPtrFactory for > each, > > that looks confusing to me. > > > > 4. > > Another is the lifetime of the bound objects. After the cancellation on raw > > Weak{Ptr,Persistent}, bound objects keeps alive until the Callback itself is > > destroyed. Not sure it matters a lot, but existing cancellation mechanisms > seem > > to clear objects depended by the task on the cancellation. To preserve the > > previous behavior, we should clear the bound objects on the cancellation. > > > > 5. > > The receiver retention and the cancellation are orthogonal, IMO. We might need > a > > cancellable task with strong reference to the object, or a cancellable task on > a > > non-member function. > > # Though I don't find the example for this. :p
On 2016/09/12 07:45:51, tzik wrote:
> On 2016/09/09 19:22:19, dcheng wrote:
> > One of the canonical examples is this:
> >
> > auto result = makeCancellable(bind(&Foo::bar, wrapPersistent(foo)));
> >
> > But that could just be written as:
> >
> > auto result = bind(&Foo::bar, wrapWeakPersistent(foo)));
> >
> > right? Which is shorter and also avoids an additional heap allocations for
the
> > extra state.
> >
> > In the linked example, the same thing could be accomplished by using
> > WeakPtrFactory directly and also results in fewer object allocations, unless
> I'm
> > missing something.
> >
> > In Chromium, just using WeakPtrFactory has worked pretty well: while there
is
> a
> > CancelableClosure helper in //base... looking at those, I think most of
those
> > could probably just use WeakPtrFactory.
>
> I have several concern about cancellation by the direct use of WeakPersistent
or
> WeakPtr.
>
> 1.
> On the cancellation by WeakPersistent, we have no way to invalidate and cancel
> the task before its lifetime, some of ActiveDOMObjects need to cancel their
> pending tasks on ActiveDOMObject::stop().
>
> 2.
> The direct use of WeakPtrFactory on GCed object is probably unsafe due to the
> lazy sweep. After the object gets unreachable and before it gets sweeped,
there
> is a period that we can't touch the object and WeakPtr is still valid.
Yeah, I agree this would be a bit weird, but it's not clear to me how
makeCancellable helps with that: it has the same problem unless there's an
explicit invalidate() somewhere in an explicit dispose(), right?
>
> 3.
> Also, some classes have multiple cancellable tasks. (e.g. Document has 6
> Timer<Document> as its member). They will have multiple WeakPtrFactory for
each,
> that looks confusing to me.
It's a pretty common pattern in Chrome actually...
>
> 4.
> Another is the lifetime of the bound objects. After the cancellation on raw
> Weak{Ptr,Persistent}, bound objects keeps alive until the Callback itself is
> destroyed. Not sure it matters a lot, but existing cancellation mechanisms
seem
> to clear objects depended by the task on the cancellation. To preserve the
> previous behavior, we should clear the bound objects on the cancellation.
>
> 5.
> The receiver retention and the cancellation are orthogonal, IMO. We might need
a
> cancellable task with strong reference to the object, or a cancellable task on
a
> non-member function.
> # Though I don't find the example for this. :p
Right, if there were a good use case for this, then perhaps, but I don't know of
any either.
On 2016/09/21 06:13:38, dcheng wrote:
> On 2016/09/12 07:45:51, tzik wrote:
> > On 2016/09/09 19:22:19, dcheng wrote:
> > > One of the canonical examples is this:
> > >
> > > auto result = makeCancellable(bind(&Foo::bar, wrapPersistent(foo)));
> > >
> > > But that could just be written as:
> > >
> > > auto result = bind(&Foo::bar, wrapWeakPersistent(foo)));
> > >
> > > right? Which is shorter and also avoids an additional heap allocations for
> the
> > > extra state.
> > >
> > > In the linked example, the same thing could be accomplished by using
> > > WeakPtrFactory directly and also results in fewer object allocations,
unless
> > I'm
> > > missing something.
> > >
> > > In Chromium, just using WeakPtrFactory has worked pretty well: while there
> is
> > a
> > > CancelableClosure helper in //base... looking at those, I think most of
> those
> > > could probably just use WeakPtrFactory.
> >
> > I have several concern about cancellation by the direct use of
WeakPersistent
> or
> > WeakPtr.
> >
> > 1.
> > On the cancellation by WeakPersistent, we have no way to invalidate and
cancel
> > the task before its lifetime, some of ActiveDOMObjects need to cancel their
> > pending tasks on ActiveDOMObject::stop().
> >
> > 2.
> > The direct use of WeakPtrFactory on GCed object is probably unsafe due to
the
> > lazy sweep. After the object gets unreachable and before it gets sweeped,
> there
> > is a period that we can't touch the object and WeakPtr is still valid.
>
> Yeah, I agree this would be a bit weird, but it's not clear to me how
> makeCancellable helps with that: it has the same problem unless there's an
> explicit invalidate() somewhere in an explicit dispose(), right?
Since the inner WTF::Function will have a managed smart pointer, we'll not hit
the unreachable object access on that case.
E.g.:
WeakPersistent<Foo> foo;
auto innerFunction = bind(&Foo::bar, foo);
auto result = makeCancellable(innerFunction);
// |*foo| got unreachable here, and that implies |foo| is cleared.
(*result.function)(); // |innerFunction| is cancelled by |foo| invalidation.
>
> >
> > 3.
> > Also, some classes have multiple cancellable tasks. (e.g. Document has 6
> > Timer<Document> as its member). They will have multiple WeakPtrFactory for
> each,
> > that looks confusing to me.
>
> It's a pretty common pattern in Chrome actually...
>
Hmm, I didn't see them before, but found 4 case of multiple WeakPtrFactory
member.
> >
> > 4.
> > Another is the lifetime of the bound objects. After the cancellation on raw
> > Weak{Ptr,Persistent}, bound objects keeps alive until the Callback itself is
> > destroyed. Not sure it matters a lot, but existing cancellation mechanisms
> seem
> > to clear objects depended by the task on the cancellation. To preserve the
> > previous behavior, we should clear the bound objects on the cancellation.
> >
> > 5.
> > The receiver retention and the cancellation are orthogonal, IMO. We might
need
> a
> > cancellable task with strong reference to the object, or a cancellable task
on
> a
> > non-member function.
> > # Though I don't find the example for this. :p
>
> Right, if there were a good use case for this, then perhaps, but I don't know
of
> any either.
tzik@ has been going back and forth for a long time. Let's try to collect all thoughts on this thread and build consensus :) On 2016/09/21 06:59:16, tzik wrote: > On 2016/09/21 06:13:38, dcheng wrote: > > On 2016/09/12 07:45:51, tzik wrote: > > > On 2016/09/09 19:22:19, dcheng wrote: > > > > One of the canonical examples is this: > > > > > > > > auto result = makeCancellable(bind(&Foo::bar, wrapPersistent(foo))); > > > > > > > > But that could just be written as: > > > > > > > > auto result = bind(&Foo::bar, wrapWeakPersistent(foo))); > > > > > > > > right? Which is shorter and also avoids an additional heap allocations for > > the > > > > extra state. > > > > > > > > In the linked example, the same thing could be accomplished by using > > > > WeakPtrFactory directly and also results in fewer object allocations, > unless > > > I'm > > > > missing something. > > > > > > > > In Chromium, just using WeakPtrFactory has worked pretty well: while there > > is > > > a > > > > CancelableClosure helper in //base... looking at those, I think most of > > those > > > > could probably just use WeakPtrFactory. > > > > > > I have several concern about cancellation by the direct use of > WeakPersistent > > or > > > WeakPtr. > > > > > > 1. > > > On the cancellation by WeakPersistent, we have no way to invalidate and > cancel > > > the task before its lifetime, some of ActiveDOMObjects need to cancel their > > > pending tasks on ActiveDOMObject::stop(). > > > > > > 2. > > > The direct use of WeakPtrFactory on GCed object is probably unsafe due to > the > > > lazy sweep. After the object gets unreachable and before it gets sweeped, > > there > > > is a period that we can't touch the object and WeakPtr is still valid. > > > > Yeah, I agree this would be a bit weird, but it's not clear to me how > > makeCancellable helps with that: it has the same problem unless there's an > > explicit invalidate() somewhere in an explicit dispose(), right? > > Since the inner WTF::Function will have a managed smart pointer, we'll not hit > the unreachable object access on that case. > E.g.: > WeakPersistent<Foo> foo; > auto innerFunction = bind(&Foo::bar, foo); > auto result = makeCancellable(innerFunction); > > // |*foo| got unreachable here, and that implies |foo| is cleared. > > (*result.function)(); // |innerFunction| is cancelled by |foo| invalidation. Right. And the WebTaskRunner::postCancellableTask API (https://codereview.chromium.org/2353913005/) is a thing that tries to make the API shape nicer. (As far as I was observing the previous thread, it seems people liked the WebTaskRunner::postCancellableTask API, although we have decided not to use the task-handle implementation internally.) > > > > > > > > > 3. > > > Also, some classes have multiple cancellable tasks. (e.g. Document has 6 > > > Timer<Document> as its member). They will have multiple WeakPtrFactory for > > each, > > > that looks confusing to me. > > > > It's a pretty common pattern in Chrome actually... > > > > Hmm, I didn't see them before, but found 4 case of multiple WeakPtrFactory > member. > > > > > > > 4. > > > Another is the lifetime of the bound objects. After the cancellation on raw > > > Weak{Ptr,Persistent}, bound objects keeps alive until the Callback itself is > > > destroyed. Not sure it matters a lot, but existing cancellation mechanisms > > seem > > > to clear objects depended by the task on the cancellation. To preserve the > > > previous behavior, we should clear the bound objects on the cancellation. > > > > > > 5. > > > The receiver retention and the cancellation are orthogonal, IMO. We might > need > > a > > > cancellable task with strong reference to the object, or a cancellable task > on > > a > > > non-member function. > > > # Though I don't find the example for this. :p > > > > Right, if there were a good use case for this, then perhaps, but I don't know > of > > any either.
On 2016/09/21 07:50:42, haraken wrote: > tzik@ has been going back and forth for a long time. Let's try to collect all > thoughts on this thread and build consensus :) Yeah, sorry. This is a tricky problem and not very easy to solve. Some thoughts: - Is it possible to disallow normal WeakPtrFactory from getting used for Oilpan objects? Would it make sense to? For example, I know that Document has a WeakPtrFactory right now. - Alternatively, would it make sense to use registerWeakMembers to force invalidation of a WeakPtrFactory? Maybe that way we wouldn't have to rely on sweep? > > On 2016/09/21 06:59:16, tzik wrote: > > On 2016/09/21 06:13:38, dcheng wrote: > > > On 2016/09/12 07:45:51, tzik wrote: > > > > On 2016/09/09 19:22:19, dcheng wrote: > > > > > One of the canonical examples is this: > > > > > > > > > > auto result = makeCancellable(bind(&Foo::bar, wrapPersistent(foo))); > > > > > > > > > > But that could just be written as: > > > > > > > > > > auto result = bind(&Foo::bar, wrapWeakPersistent(foo))); > > > > > > > > > > right? Which is shorter and also avoids an additional heap allocations > for > > > the > > > > > extra state. > > > > > > > > > > In the linked example, the same thing could be accomplished by using > > > > > WeakPtrFactory directly and also results in fewer object allocations, > > unless > > > > I'm > > > > > missing something. > > > > > > > > > > In Chromium, just using WeakPtrFactory has worked pretty well: while > there > > > is > > > > a > > > > > CancelableClosure helper in //base... looking at those, I think most of > > > those > > > > > could probably just use WeakPtrFactory. > > > > > > > > I have several concern about cancellation by the direct use of > > WeakPersistent > > > or > > > > WeakPtr. > > > > > > > > 1. > > > > On the cancellation by WeakPersistent, we have no way to invalidate and > > cancel > > > > the task before its lifetime, some of ActiveDOMObjects need to cancel > their > > > > pending tasks on ActiveDOMObject::stop(). > > > > > > > > 2. > > > > The direct use of WeakPtrFactory on GCed object is probably unsafe due to > > the > > > > lazy sweep. After the object gets unreachable and before it gets sweeped, > > > there > > > > is a period that we can't touch the object and WeakPtr is still valid. > > > > > > Yeah, I agree this would be a bit weird, but it's not clear to me how > > > makeCancellable helps with that: it has the same problem unless there's an > > > explicit invalidate() somewhere in an explicit dispose(), right? > > > > Since the inner WTF::Function will have a managed smart pointer, we'll not hit > > the unreachable object access on that case. > > E.g.: > > WeakPersistent<Foo> foo; > > auto innerFunction = bind(&Foo::bar, foo); > > auto result = makeCancellable(innerFunction); > > > > // |*foo| got unreachable here, and that implies |foo| is cleared. > > > > (*result.function)(); // |innerFunction| is cancelled by |foo| > invalidation. > > Right. And the WebTaskRunner::postCancellableTask API > (https://codereview.chromium.org/2353913005/) is a thing that tries to make the > API shape nicer. (As far as I was observing the previous thread, it seems people > liked the WebTaskRunner::postCancellableTask API, although we have decided not > to use the task-handle implementation internally.) > > > > > > > > > > > > > > > 3. > > > > Also, some classes have multiple cancellable tasks. (e.g. Document has 6 > > > > Timer<Document> as its member). They will have multiple WeakPtrFactory for > > > each, > > > > that looks confusing to me. > > > > > > It's a pretty common pattern in Chrome actually... > > > > > > > Hmm, I didn't see them before, but found 4 case of multiple WeakPtrFactory > > member. > > > > > > > > > > 4. > > > > Another is the lifetime of the bound objects. After the cancellation on > raw > > > > Weak{Ptr,Persistent}, bound objects keeps alive until the Callback itself > is > > > > destroyed. Not sure it matters a lot, but existing cancellation mechanisms > > > seem > > > > to clear objects depended by the task on the cancellation. To preserve the > > > > previous behavior, we should clear the bound objects on the cancellation. > > > > > > > > 5. > > > > The receiver retention and the cancellation are orthogonal, IMO. We might > > need > > > a > > > > cancellable task with strong reference to the object, or a cancellable > task > > on > > > a > > > > non-member function. > > > > # Though I don't find the example for this. :p > > > > > > Right, if there were a good use case for this, then perhaps, but I don't > know > > of > > > any either.
(Sorry, I didn't notice your reply -- I was kicked out from the cc list for some reason.) On 2016/09/22 06:01:16, dcheng wrote: > On 2016/09/21 07:50:42, haraken wrote: > > tzik@ has been going back and forth for a long time. Let's try to collect all > > thoughts on this thread and build consensus :) > > > Yeah, sorry. This is a tricky problem and not very easy to solve. Some thoughts: > - Is it possible to disallow normal WeakPtrFactory from getting used for Oilpan > objects? Would it make sense to? For example, I know that Document has a > WeakPtrFactory right now. > - Alternatively, would it make sense to use registerWeakMembers to force > invalidation of a WeakPtrFactory? Maybe that way we wouldn't have to rely on > sweep? Hmm, I'm not sure how it works. registerWeakMembers (or default weak processing of WeakPersistent/WeakMember) is a way to take some action when the pointing object is collected in a garbage collection. It doesn't provide a way for developers to explicitly revoke all WeakMembers. That is the functionality WeakPtrFactory provides, and that's what we need to cancel posted tasks. What do you think? > > > > On 2016/09/21 06:59:16, tzik wrote: > > > On 2016/09/21 06:13:38, dcheng wrote: > > > > On 2016/09/12 07:45:51, tzik wrote: > > > > > On 2016/09/09 19:22:19, dcheng wrote: > > > > > > One of the canonical examples is this: > > > > > > > > > > > > auto result = makeCancellable(bind(&Foo::bar, wrapPersistent(foo))); > > > > > > > > > > > > But that could just be written as: > > > > > > > > > > > > auto result = bind(&Foo::bar, wrapWeakPersistent(foo))); > > > > > > > > > > > > right? Which is shorter and also avoids an additional heap allocations > > for > > > > the > > > > > > extra state. > > > > > > > > > > > > In the linked example, the same thing could be accomplished by using > > > > > > WeakPtrFactory directly and also results in fewer object allocations, > > > unless > > > > > I'm > > > > > > missing something. > > > > > > > > > > > > In Chromium, just using WeakPtrFactory has worked pretty well: while > > there > > > > is > > > > > a > > > > > > CancelableClosure helper in //base... looking at those, I think most > of > > > > those > > > > > > could probably just use WeakPtrFactory. > > > > > > > > > > I have several concern about cancellation by the direct use of > > > WeakPersistent > > > > or > > > > > WeakPtr. > > > > > > > > > > 1. > > > > > On the cancellation by WeakPersistent, we have no way to invalidate and > > > cancel > > > > > the task before its lifetime, some of ActiveDOMObjects need to cancel > > their > > > > > pending tasks on ActiveDOMObject::stop(). > > > > > > > > > > 2. > > > > > The direct use of WeakPtrFactory on GCed object is probably unsafe due > to > > > the > > > > > lazy sweep. After the object gets unreachable and before it gets > sweeped, > > > > there > > > > > is a period that we can't touch the object and WeakPtr is still valid. > > > > > > > > Yeah, I agree this would be a bit weird, but it's not clear to me how > > > > makeCancellable helps with that: it has the same problem unless there's an > > > > explicit invalidate() somewhere in an explicit dispose(), right? > > > > > > Since the inner WTF::Function will have a managed smart pointer, we'll not > hit > > > the unreachable object access on that case. > > > E.g.: > > > WeakPersistent<Foo> foo; > > > auto innerFunction = bind(&Foo::bar, foo); > > > auto result = makeCancellable(innerFunction); > > > > > > // |*foo| got unreachable here, and that implies |foo| is cleared. > > > > > > (*result.function)(); // |innerFunction| is cancelled by |foo| > > invalidation. > > > > Right. And the WebTaskRunner::postCancellableTask API > > (https://codereview.chromium.org/2353913005/) is a thing that tries to make > the > > API shape nicer. (As far as I was observing the previous thread, it seems > people > > liked the WebTaskRunner::postCancellableTask API, although we have decided not > > to use the task-handle implementation internally.) > > > > > > > > > > > > > > > > > > > > > 3. > > > > > Also, some classes have multiple cancellable tasks. (e.g. Document has 6 > > > > > Timer<Document> as its member). They will have multiple WeakPtrFactory > for > > > > each, > > > > > that looks confusing to me. > > > > > > > > It's a pretty common pattern in Chrome actually... > > > > > > > > > > Hmm, I didn't see them before, but found 4 case of multiple WeakPtrFactory > > > member. > > > > > > > > > > > > > 4. > > > > > Another is the lifetime of the bound objects. After the cancellation on > > raw > > > > > Weak{Ptr,Persistent}, bound objects keeps alive until the Callback > itself > > is > > > > > destroyed. Not sure it matters a lot, but existing cancellation > mechanisms > > > > seem > > > > > to clear objects depended by the task on the cancellation. To preserve > the > > > > > previous behavior, we should clear the bound objects on the > cancellation. > > > > > > > > > > 5. > > > > > The receiver retention and the cancellation are orthogonal, IMO. We > might > > > need > > > > a > > > > > cancellable task with strong reference to the object, or a cancellable > > task > > > on > > > > a > > > > > non-member function. > > > > > # Though I don't find the example for this. :p > > > > > > > > Right, if there were a good use case for this, then perhaps, but I don't > > know > > > of > > > > any either.
On 2016/09/23 07:56:50, haraken wrote: > (Sorry, I didn't notice your reply -- I was kicked out from the cc list for some > reason.) > > On 2016/09/22 06:01:16, dcheng wrote: > > On 2016/09/21 07:50:42, haraken wrote: > > > tzik@ has been going back and forth for a long time. Let's try to collect > all > > > thoughts on this thread and build consensus :) > > > > > > Yeah, sorry. This is a tricky problem and not very easy to solve. Some > thoughts: > > - Is it possible to disallow normal WeakPtrFactory from getting used for > Oilpan > > objects? Would it make sense to? For example, I know that Document has a > > WeakPtrFactory right now. > > - Alternatively, would it make sense to use registerWeakMembers to force > > invalidation of a WeakPtrFactory? Maybe that way we wouldn't have to rely on > > sweep? > > Hmm, I'm not sure how it works. > > registerWeakMembers (or default weak processing of WeakPersistent/WeakMember) is > a way to take some action when the pointing object is collected in a garbage > collection. It doesn't provide a way for developers to explicitly revoke all > WeakMembers. That is the functionality WeakPtrFactory provides, and that's what > we need to cancel posted tasks. > > What do you think? Ah, I think I was thinking about registerWeakMembers backwards: unfortunately, I agree that it won't help here. One other thing I am thinking is if it's possible to have a custom WeakPtrFactory that stores a WeakPersistent / WeakMember inside, but I'm not sure that's actually a good idea. > > > > > > > On 2016/09/21 06:59:16, tzik wrote: > > > > On 2016/09/21 06:13:38, dcheng wrote: > > > > > On 2016/09/12 07:45:51, tzik wrote: > > > > > > On 2016/09/09 19:22:19, dcheng wrote: > > > > > > > One of the canonical examples is this: > > > > > > > > > > > > > > auto result = makeCancellable(bind(&Foo::bar, wrapPersistent(foo))); > > > > > > > > > > > > > > But that could just be written as: > > > > > > > > > > > > > > auto result = bind(&Foo::bar, wrapWeakPersistent(foo))); > > > > > > > > > > > > > > right? Which is shorter and also avoids an additional heap > allocations > > > for > > > > > the > > > > > > > extra state. > > > > > > > > > > > > > > In the linked example, the same thing could be accomplished by using > > > > > > > WeakPtrFactory directly and also results in fewer object > allocations, > > > > unless > > > > > > I'm > > > > > > > missing something. > > > > > > > > > > > > > > In Chromium, just using WeakPtrFactory has worked pretty well: while > > > there > > > > > is > > > > > > a > > > > > > > CancelableClosure helper in //base... looking at those, I think most > > of > > > > > those > > > > > > > could probably just use WeakPtrFactory. > > > > > > > > > > > > I have several concern about cancellation by the direct use of > > > > WeakPersistent > > > > > or > > > > > > WeakPtr. > > > > > > > > > > > > 1. > > > > > > On the cancellation by WeakPersistent, we have no way to invalidate > and > > > > cancel > > > > > > the task before its lifetime, some of ActiveDOMObjects need to cancel > > > their > > > > > > pending tasks on ActiveDOMObject::stop(). > > > > > > > > > > > > 2. > > > > > > The direct use of WeakPtrFactory on GCed object is probably unsafe due > > to > > > > the > > > > > > lazy sweep. After the object gets unreachable and before it gets > > sweeped, > > > > > there > > > > > > is a period that we can't touch the object and WeakPtr is still valid. > > > > > > > > > > Yeah, I agree this would be a bit weird, but it's not clear to me how > > > > > makeCancellable helps with that: it has the same problem unless there's > an > > > > > explicit invalidate() somewhere in an explicit dispose(), right? > > > > > > > > Since the inner WTF::Function will have a managed smart pointer, we'll not > > hit > > > > the unreachable object access on that case. > > > > E.g.: > > > > WeakPersistent<Foo> foo; > > > > auto innerFunction = bind(&Foo::bar, foo); > > > > auto result = makeCancellable(innerFunction); > > > > > > > > // |*foo| got unreachable here, and that implies |foo| is cleared. > > > > > > > > (*result.function)(); // |innerFunction| is cancelled by |foo| > > > invalidation. > > > > > > Right. And the WebTaskRunner::postCancellableTask API > > > (https://codereview.chromium.org/2353913005/) is a thing that tries to make > > the > > > API shape nicer. (As far as I was observing the previous thread, it seems > > people > > > liked the WebTaskRunner::postCancellableTask API, although we have decided > not > > > to use the task-handle implementation internally.) > > > > > > > > > > > > > > > > > > > > > > > > > > > 3. > > > > > > Also, some classes have multiple cancellable tasks. (e.g. Document has > 6 > > > > > > Timer<Document> as its member). They will have multiple WeakPtrFactory > > for > > > > > each, > > > > > > that looks confusing to me. > > > > > > > > > > It's a pretty common pattern in Chrome actually... > > > > > > > > > > > > > Hmm, I didn't see them before, but found 4 case of multiple WeakPtrFactory > > > > member. > > > > > > > > > > > > > > > > 4. > > > > > > Another is the lifetime of the bound objects. After the cancellation > on > > > raw > > > > > > Weak{Ptr,Persistent}, bound objects keeps alive until the Callback > > itself > > > is > > > > > > destroyed. Not sure it matters a lot, but existing cancellation > > mechanisms > > > > > seem > > > > > > to clear objects depended by the task on the cancellation. To preserve > > the > > > > > > previous behavior, we should clear the bound objects on the > > cancellation. > > > > > > > > > > > > 5. > > > > > > The receiver retention and the cancellation are orthogonal, IMO. We > > might > > > > need > > > > > a > > > > > > cancellable task with strong reference to the object, or a cancellable > > > task > > > > on > > > > > a > > > > > > non-member function. > > > > > > # Though I don't find the example for this. :p > > > > > > > > > > Right, if there were a good use case for this, then perhaps, but I don't > > > know > > > > of > > > > > any either.
On 2016/09/23 08:02:40, dcheng wrote: > On 2016/09/23 07:56:50, haraken wrote: > > (Sorry, I didn't notice your reply -- I was kicked out from the cc list for > some > > reason.) > > > > On 2016/09/22 06:01:16, dcheng wrote: > > > On 2016/09/21 07:50:42, haraken wrote: > > > > tzik@ has been going back and forth for a long time. Let's try to collect > > all > > > > thoughts on this thread and build consensus :) > > > > > > > > > Yeah, sorry. This is a tricky problem and not very easy to solve. Some > > thoughts: > > > - Is it possible to disallow normal WeakPtrFactory from getting used for > > Oilpan > > > objects? Would it make sense to? For example, I know that Document has a > > > WeakPtrFactory right now. > > > - Alternatively, would it make sense to use registerWeakMembers to force > > > invalidation of a WeakPtrFactory? Maybe that way we wouldn't have to rely on > > > sweep? > > > > Hmm, I'm not sure how it works. > > > > registerWeakMembers (or default weak processing of WeakPersistent/WeakMember) > is > > a way to take some action when the pointing object is collected in a garbage > > collection. It doesn't provide a way for developers to explicitly revoke all > > WeakMembers. That is the functionality WeakPtrFactory provides, and that's > what > > we need to cancel posted tasks. > > > > What do you think? > > Ah, I think I was thinking about registerWeakMembers backwards: unfortunately, I > agree that it won't help here. > One other thing I am thinking is if it's possible to have a custom > WeakPtrFactory that stores a WeakPersistent / WeakMember inside, but I'm not > sure that's actually a good idea. Isn't it exactly what CancellableTaskFactory is doing? BTW, this issue is blocking https://codereview.chromium.org/2341043002/ (I don't know why but it seems I'm always kicked out from the cc list on this thread. Please make sure that haraken@ is cced :-)
dcheng@chromium.org changed reviewers: + haraken@chromium.org
On 2016/09/25 23:21:32, haraken wrote: > On 2016/09/23 08:02:40, dcheng wrote: > > On 2016/09/23 07:56:50, haraken wrote: > > > (Sorry, I didn't notice your reply -- I was kicked out from the cc list for > > some > > > reason.) > > > > > > On 2016/09/22 06:01:16, dcheng wrote: > > > > On 2016/09/21 07:50:42, haraken wrote: > > > > > tzik@ has been going back and forth for a long time. Let's try to > collect > > > all > > > > > thoughts on this thread and build consensus :) > > > > > > > > > > > > Yeah, sorry. This is a tricky problem and not very easy to solve. Some > > > thoughts: > > > > - Is it possible to disallow normal WeakPtrFactory from getting used for > > > Oilpan > > > > objects? Would it make sense to? For example, I know that Document has a > > > > WeakPtrFactory right now. > > > > - Alternatively, would it make sense to use registerWeakMembers to force > > > > invalidation of a WeakPtrFactory? Maybe that way we wouldn't have to rely > on > > > > sweep? > > > > > > Hmm, I'm not sure how it works. > > > > > > registerWeakMembers (or default weak processing of > WeakPersistent/WeakMember) > > is > > > a way to take some action when the pointing object is collected in a garbage > > > collection. It doesn't provide a way for developers to explicitly revoke all > > > WeakMembers. That is the functionality WeakPtrFactory provides, and that's > > what > > > we need to cancel posted tasks. > > > > > > What do you think? > > > > Ah, I think I was thinking about registerWeakMembers backwards: unfortunately, > I > > agree that it won't help here. > > One other thing I am thinking is if it's possible to have a custom > > WeakPtrFactory that stores a WeakPersistent / WeakMember inside, but I'm not > > sure that's actually a good idea. > > Isn't it exactly what CancellableTaskFactory is doing? > Yes, but it's with an additional indirection: we have to wrap a callback in another callback. > BTW, this issue is blocking https://codereview.chromium.org/2341043002/ > > (I don't know why but it seems I'm always kicked out from the cc list on this > thread. Please make sure that haraken@ is cced :-) base/memory/weak_ptr.h stores a T* in the WeakPtr right now, but if it was configurable by a template parameter, we could just make WeakPtr hold a WeakPersistent<T> for Oilpan objects, right? Then we would just get the correct behavior.
On 2016/09/25 23:41:31, dcheng wrote: > On 2016/09/25 23:21:32, haraken wrote: > > On 2016/09/23 08:02:40, dcheng wrote: > > > On 2016/09/23 07:56:50, haraken wrote: > > > > (Sorry, I didn't notice your reply -- I was kicked out from the cc list > for > > > some > > > > reason.) > > > > > > > > On 2016/09/22 06:01:16, dcheng wrote: > > > > > On 2016/09/21 07:50:42, haraken wrote: > > > > > > tzik@ has been going back and forth for a long time. Let's try to > > collect > > > > all > > > > > > thoughts on this thread and build consensus :) > > > > > > > > > > > > > > > Yeah, sorry. This is a tricky problem and not very easy to solve. Some > > > > thoughts: > > > > > - Is it possible to disallow normal WeakPtrFactory from getting used for > > > > Oilpan > > > > > objects? Would it make sense to? For example, I know that Document has a > > > > > WeakPtrFactory right now. > > > > > - Alternatively, would it make sense to use registerWeakMembers to force > > > > > invalidation of a WeakPtrFactory? Maybe that way we wouldn't have to > rely > > on > > > > > sweep? > > > > > > > > Hmm, I'm not sure how it works. > > > > > > > > registerWeakMembers (or default weak processing of > > WeakPersistent/WeakMember) > > > is > > > > a way to take some action when the pointing object is collected in a > garbage > > > > collection. It doesn't provide a way for developers to explicitly revoke > all > > > > WeakMembers. That is the functionality WeakPtrFactory provides, and that's > > > what > > > > we need to cancel posted tasks. > > > > > > > > What do you think? > > > > > > Ah, I think I was thinking about registerWeakMembers backwards: > unfortunately, > > I > > > agree that it won't help here. > > > One other thing I am thinking is if it's possible to have a custom > > > WeakPtrFactory that stores a WeakPersistent / WeakMember inside, but I'm not > > > sure that's actually a good idea. > > > > Isn't it exactly what CancellableTaskFactory is doing? > > > > Yes, but it's with an additional indirection: we have to wrap a callback in > another callback. > > > BTW, this issue is blocking https://codereview.chromium.org/2341043002/ > > > > (I don't know why but it seems I'm always kicked out from the cc list on this > > thread. Please make sure that haraken@ is cced :-) > > base/memory/weak_ptr.h stores a T* in the WeakPtr right now, but if it was > configurable by a template parameter, we could just make WeakPtr hold a > WeakPersistent<T> for Oilpan objects, right? Then we would just get the correct > behavior. Yeah, I think that's an option. tzik@: What do you think?
On 2016/09/25 23:41:31, dcheng wrote: > On 2016/09/25 23:21:32, haraken wrote: > > On 2016/09/23 08:02:40, dcheng wrote: > > > On 2016/09/23 07:56:50, haraken wrote: > > > > (Sorry, I didn't notice your reply -- I was kicked out from the cc list > for > > > some > > > > reason.) > > > > > > > > On 2016/09/22 06:01:16, dcheng wrote: > > > > > On 2016/09/21 07:50:42, haraken wrote: > > > > > > tzik@ has been going back and forth for a long time. Let's try to > > collect > > > > all > > > > > > thoughts on this thread and build consensus :) > > > > > > > > > > > > > > > Yeah, sorry. This is a tricky problem and not very easy to solve. Some > > > > thoughts: > > > > > - Is it possible to disallow normal WeakPtrFactory from getting used for > > > > Oilpan > > > > > objects? Would it make sense to? For example, I know that Document has a > > > > > WeakPtrFactory right now. > > > > > - Alternatively, would it make sense to use registerWeakMembers to force > > > > > invalidation of a WeakPtrFactory? Maybe that way we wouldn't have to > rely > > on > > > > > sweep? > > > > > > > > Hmm, I'm not sure how it works. > > > > > > > > registerWeakMembers (or default weak processing of > > WeakPersistent/WeakMember) > > > is > > > > a way to take some action when the pointing object is collected in a > garbage > > > > collection. It doesn't provide a way for developers to explicitly revoke > all > > > > WeakMembers. That is the functionality WeakPtrFactory provides, and that's > > > what > > > > we need to cancel posted tasks. > > > > > > > > What do you think? > > > > > > Ah, I think I was thinking about registerWeakMembers backwards: > unfortunately, > > I > > > agree that it won't help here. > > > One other thing I am thinking is if it's possible to have a custom > > > WeakPtrFactory that stores a WeakPersistent / WeakMember inside, but I'm not > > > sure that's actually a good idea. > > > > Isn't it exactly what CancellableTaskFactory is doing? > > > > Yes, but it's with an additional indirection: we have to wrap a callback in > another callback. > > > BTW, this issue is blocking https://codereview.chromium.org/2341043002/ > > > > (I don't know why but it seems I'm always kicked out from the cc list on this > > thread. Please make sure that haraken@ is cced :-) > > base/memory/weak_ptr.h stores a T* in the WeakPtr right now, but if it was > configurable by a template parameter, we could just make WeakPtr hold a > WeakPersistent<T> for Oilpan objects, right? Then we would just get the correct > behavior. That implies WeakPtr will require T be a complete type, that requires a massive rewrite, and also a class will not able to hold a WeakPtr to itself. struct Foo { WeakPtr<Foo> weak_foo; // Foo is an incomplete type the compiler reaches "}". };
On 2016/09/26 04:51:52, tzik wrote: > On 2016/09/25 23:41:31, dcheng wrote: > > On 2016/09/25 23:21:32, haraken wrote: > > > On 2016/09/23 08:02:40, dcheng wrote: > > > > On 2016/09/23 07:56:50, haraken wrote: > > > > > (Sorry, I didn't notice your reply -- I was kicked out from the cc list > > for > > > > some > > > > > reason.) > > > > > > > > > > On 2016/09/22 06:01:16, dcheng wrote: > > > > > > On 2016/09/21 07:50:42, haraken wrote: > > > > > > > tzik@ has been going back and forth for a long time. Let's try to > > > collect > > > > > all > > > > > > > thoughts on this thread and build consensus :) > > > > > > > > > > > > > > > > > > Yeah, sorry. This is a tricky problem and not very easy to solve. Some > > > > > thoughts: > > > > > > - Is it possible to disallow normal WeakPtrFactory from getting used > for > > > > > Oilpan > > > > > > objects? Would it make sense to? For example, I know that Document has > a > > > > > > WeakPtrFactory right now. > > > > > > - Alternatively, would it make sense to use registerWeakMembers to > force > > > > > > invalidation of a WeakPtrFactory? Maybe that way we wouldn't have to > > rely > > > on > > > > > > sweep? > > > > > > > > > > Hmm, I'm not sure how it works. > > > > > > > > > > registerWeakMembers (or default weak processing of > > > WeakPersistent/WeakMember) > > > > is > > > > > a way to take some action when the pointing object is collected in a > > garbage > > > > > collection. It doesn't provide a way for developers to explicitly revoke > > all > > > > > WeakMembers. That is the functionality WeakPtrFactory provides, and > that's > > > > what > > > > > we need to cancel posted tasks. > > > > > > > > > > What do you think? > > > > > > > > Ah, I think I was thinking about registerWeakMembers backwards: > > unfortunately, > > > I > > > > agree that it won't help here. > > > > One other thing I am thinking is if it's possible to have a custom > > > > WeakPtrFactory that stores a WeakPersistent / WeakMember inside, but I'm > not > > > > sure that's actually a good idea. > > > > > > Isn't it exactly what CancellableTaskFactory is doing? > > > > > > > Yes, but it's with an additional indirection: we have to wrap a callback in > > another callback. > > > > > BTW, this issue is blocking https://codereview.chromium.org/2341043002/ > > > > > > (I don't know why but it seems I'm always kicked out from the cc list on > this > > > thread. Please make sure that haraken@ is cced :-) > > > > base/memory/weak_ptr.h stores a T* in the WeakPtr right now, but if it was > > configurable by a template parameter, we could just make WeakPtr hold a > > WeakPersistent<T> for Oilpan objects, right? Then we would just get the > correct > > behavior. > > That implies WeakPtr will require T be a complete type, that requires a massive > rewrite, and also a class will not able to hold a WeakPtr to itself. > > struct Foo { > WeakPtr<Foo> weak_foo; // Foo is an incomplete type the compiler reaches "}". > }; Tried to add a traits to configure the internal pointer type: https://codereview.chromium.org/2371663002/
tzik@ has been trying to invent a way to wrap an Oilpan pointer in WeakPtr<T> but hasn't yet succeeded at the attempt for some complicated reasons (which tzik@ can explain in details). dcheng@: Is there any option to add a helper function inside Blink that provides the postCancellableTask-style API? I'm curious whether your concern is on adding the postCancellableTask-style API to the public layer or on adding the API to the code base. If your concern is the former one, we can add a helper function only to Blink (not the public layer).
On 2016/09/28 09:03:41, haraken wrote: > tzik@ has been trying to invent a way to wrap an Oilpan pointer in WeakPtr<T> > but hasn't yet succeeded at the attempt for some complicated reasons (which > tzik@ can explain in details). > > dcheng@: Is there any option to add a helper function inside Blink that provides > the postCancellableTask-style API? I'm curious whether your concern is on adding > the postCancellableTask-style API to the public layer or on adding the API to > the code base. If your concern is the former one, we can add a helper function > only to Blink (not the public layer). It turned out even adding traits is confusing to match with the coding convention of Oilpan. We have to have 3 new variants of WeakPtrFactory for each WeakPersistent, CrossThreadWeakPersistent, and WeakMember, so that the owner of WeakPtrFactory can be GCed object or not. And we need 4 variants of WeakPtr for raw pointer, WP, CTWP, WM. Also, WeakMember variants of WeakPtr and WeakPtrFactory need traceable.
On 2016/09/28 13:59:15, tzik wrote: > On 2016/09/28 09:03:41, haraken wrote: > > tzik@ has been trying to invent a way to wrap an Oilpan pointer in WeakPtr<T> > > but hasn't yet succeeded at the attempt for some complicated reasons (which > > tzik@ can explain in details). > > > > dcheng@: Is there any option to add a helper function inside Blink that > provides > > the postCancellableTask-style API? I'm curious whether your concern is on > adding > > the postCancellableTask-style API to the public layer or on adding the API to > > the code base. If your concern is the former one, we can add a helper function > > only to Blink (not the public layer). > > It turned out even adding traits is confusing to match with the coding > convention of Oilpan. > We have to have 3 new variants of WeakPtrFactory for each WeakPersistent, > CrossThreadWeakPersistent, and WeakMember, so that the owner of WeakPtrFactory > can be GCed object or not. And we need 4 variants of WeakPtr for raw pointer, > WP, CTWP, WM. > Also, WeakMember variants of WeakPtr and WeakPtrFactory need traceable. WeakPtrFactory isn't thread-safe, so CTWP isn't an option anyway. Is just using WeakPersistent a big problem?
On 2016/09/29 01:30:14, dcheng wrote: > On 2016/09/28 13:59:15, tzik wrote: > > On 2016/09/28 09:03:41, haraken wrote: > > > tzik@ has been trying to invent a way to wrap an Oilpan pointer in > WeakPtr<T> > > > but hasn't yet succeeded at the attempt for some complicated reasons (which > > > tzik@ can explain in details). > > > > > > dcheng@: Is there any option to add a helper function inside Blink that > > provides > > > the postCancellableTask-style API? I'm curious whether your concern is on > > adding > > > the postCancellableTask-style API to the public layer or on adding the API > to > > > the code base. If your concern is the former one, we can add a helper > function > > > only to Blink (not the public layer). > > > > It turned out even adding traits is confusing to match with the coding > > convention of Oilpan. > > We have to have 3 new variants of WeakPtrFactory for each WeakPersistent, > > CrossThreadWeakPersistent, and WeakMember, so that the owner of WeakPtrFactory > > can be GCed object or not. And we need 4 variants of WeakPtr for raw pointer, > > WP, CTWP, WM. > > Also, WeakMember variants of WeakPtr and WeakPtrFactory need traceable. > > WeakPtrFactory isn't thread-safe, so CTWP isn't an option anyway. Is just using > WeakPersistent a big problem? Btw, to unblock things, I'm OK with proceeding with something like this if we can limit to Oilpan types. To me, it's not as bad if Oilpan has special rules, since it's memory model is not quite the same. In the long-term, I would still like to converge these, but we can figure that out without blocking further timer work. WDYT?
On 2016/09/29 02:34:22, dcheng wrote: > On 2016/09/29 01:30:14, dcheng wrote: > > On 2016/09/28 13:59:15, tzik wrote: > > > On 2016/09/28 09:03:41, haraken wrote: > > > > tzik@ has been trying to invent a way to wrap an Oilpan pointer in > > WeakPtr<T> > > > > but hasn't yet succeeded at the attempt for some complicated reasons > (which > > > > tzik@ can explain in details). > > > > > > > > dcheng@: Is there any option to add a helper function inside Blink that > > > provides > > > > the postCancellableTask-style API? I'm curious whether your concern is on > > > adding > > > > the postCancellableTask-style API to the public layer or on adding the API > > to > > > > the code base. If your concern is the former one, we can add a helper > > function > > > > only to Blink (not the public layer). > > > > > > It turned out even adding traits is confusing to match with the coding > > > convention of Oilpan. > > > We have to have 3 new variants of WeakPtrFactory for each WeakPersistent, > > > CrossThreadWeakPersistent, and WeakMember, so that the owner of > WeakPtrFactory > > > can be GCed object or not. And we need 4 variants of WeakPtr for raw > pointer, > > > WP, CTWP, WM. > > > Also, WeakMember variants of WeakPtr and WeakPtrFactory need traceable. > > > > WeakPtrFactory isn't thread-safe, so CTWP isn't an option anyway. Is just > using > > WeakPersistent a big problem? > > Btw, to unblock things, I'm OK with proceeding with something like this if we > can limit to Oilpan types. To me, it's not as bad if Oilpan has special rules, > since it's memory model is not quite the same. > > In the long-term, I would still like to converge these, but we can figure that > out without blocking further timer work. WDYT? Sounds reasonable to me.
On 2016/09/29 01:30:14, dcheng wrote: > On 2016/09/28 13:59:15, tzik wrote: > > On 2016/09/28 09:03:41, haraken wrote: > > > tzik@ has been trying to invent a way to wrap an Oilpan pointer in > WeakPtr<T> > > > but hasn't yet succeeded at the attempt for some complicated reasons (which > > > tzik@ can explain in details). > > > > > > dcheng@: Is there any option to add a helper function inside Blink that > > provides > > > the postCancellableTask-style API? I'm curious whether your concern is on > > adding > > > the postCancellableTask-style API to the public layer or on adding the API > to > > > the code base. If your concern is the former one, we can add a helper > function > > > only to Blink (not the public layer). > > > > It turned out even adding traits is confusing to match with the coding > > convention of Oilpan. > > We have to have 3 new variants of WeakPtrFactory for each WeakPersistent, > > CrossThreadWeakPersistent, and WeakMember, so that the owner of WeakPtrFactory > > can be GCed object or not. And we need 4 variants of WeakPtr for raw pointer, > > WP, CTWP, WM. > > Also, WeakMember variants of WeakPtr and WeakPtrFactory need traceable. > > WeakPtrFactory isn't thread-safe, so CTWP isn't an option anyway. Is just using > WeakPersistent a big problem? The construction and destruction of WPF can be happen on the different threads, though WP creation and revocation can't. It's banned by the oilpan clang plugin to hold WeakPersistent on a GCed object directly or indirectly. So, if we unconditionally hold the object as WeakPersistent, GCed objects will not able to hold WeakPtrFactory.
On 2016/09/29 02:34:22, dcheng wrote: > On 2016/09/29 01:30:14, dcheng wrote: > > On 2016/09/28 13:59:15, tzik wrote: > > > On 2016/09/28 09:03:41, haraken wrote: > > > > tzik@ has been trying to invent a way to wrap an Oilpan pointer in > > WeakPtr<T> > > > > but hasn't yet succeeded at the attempt for some complicated reasons > (which > > > > tzik@ can explain in details). > > > > > > > > dcheng@: Is there any option to add a helper function inside Blink that > > > provides > > > > the postCancellableTask-style API? I'm curious whether your concern is on > > > adding > > > > the postCancellableTask-style API to the public layer or on adding the API > > to > > > > the code base. If your concern is the former one, we can add a helper > > function > > > > only to Blink (not the public layer). > > > > > > It turned out even adding traits is confusing to match with the coding > > > convention of Oilpan. > > > We have to have 3 new variants of WeakPtrFactory for each WeakPersistent, > > > CrossThreadWeakPersistent, and WeakMember, so that the owner of > WeakPtrFactory > > > can be GCed object or not. And we need 4 variants of WeakPtr for raw > pointer, > > > WP, CTWP, WM. > > > Also, WeakMember variants of WeakPtr and WeakPtrFactory need traceable. > > > > WeakPtrFactory isn't thread-safe, so CTWP isn't an option anyway. Is just > using > > WeakPersistent a big problem? > > Btw, to unblock things, I'm OK with proceeding with something like this if we > can limit to Oilpan types. To me, it's not as bad if Oilpan has special rules, > since it's memory model is not quite the same. > > In the long-term, I would still like to converge these, but we can figure that > out without blocking further timer work. WDYT? dcheng@: Just to confirm, what do you mean by 'something like this'? Do you mean the postCancellableTask API not exposed to the public layer? Or do you mean supporting many traits in WeakPtrFactory?
On 2016/09/30 02:01:56, haraken wrote: > On 2016/09/29 02:34:22, dcheng wrote: > > On 2016/09/29 01:30:14, dcheng wrote: > > > On 2016/09/28 13:59:15, tzik wrote: > > > > On 2016/09/28 09:03:41, haraken wrote: > > > > > tzik@ has been trying to invent a way to wrap an Oilpan pointer in > > > WeakPtr<T> > > > > > but hasn't yet succeeded at the attempt for some complicated reasons > > (which > > > > > tzik@ can explain in details). > > > > > > > > > > dcheng@: Is there any option to add a helper function inside Blink that > > > > provides > > > > > the postCancellableTask-style API? I'm curious whether your concern is > on > > > > adding > > > > > the postCancellableTask-style API to the public layer or on adding the > API > > > to > > > > > the code base. If your concern is the former one, we can add a helper > > > function > > > > > only to Blink (not the public layer). > > > > > > > > It turned out even adding traits is confusing to match with the coding > > > > convention of Oilpan. > > > > We have to have 3 new variants of WeakPtrFactory for each WeakPersistent, > > > > CrossThreadWeakPersistent, and WeakMember, so that the owner of > > WeakPtrFactory > > > > can be GCed object or not. And we need 4 variants of WeakPtr for raw > > pointer, > > > > WP, CTWP, WM. > > > > Also, WeakMember variants of WeakPtr and WeakPtrFactory need traceable. > > > > > > WeakPtrFactory isn't thread-safe, so CTWP isn't an option anyway. Is just > > using > > > WeakPersistent a big problem? > > > > Btw, to unblock things, I'm OK with proceeding with something like this if we > > can limit to Oilpan types. To me, it's not as bad if Oilpan has special rules, > > since it's memory model is not quite the same. > > > > In the long-term, I would still like to converge these, but we can figure that > > out without blocking further timer work. WDYT? > > dcheng@: Just to confirm, what do you mean by 'something like this'? Do you mean > the postCancellableTask API not exposed to the public layer? Or do you mean > supporting many traits in WeakPtrFactory? Sorry, I was not clear. I mean the current approach in this CL to add a new API in Blink. I don't have a strong preference where we expose it; I would just prefer that we static_assert that it's used for Oilpan for now. WeakPtrFactory has too many problems and we shouldn't block this work anymore.
On 2016/09/30 02:31:52, dcheng wrote: > On 2016/09/30 02:01:56, haraken wrote: > > On 2016/09/29 02:34:22, dcheng wrote: > > > On 2016/09/29 01:30:14, dcheng wrote: > > > > On 2016/09/28 13:59:15, tzik wrote: > > > > > On 2016/09/28 09:03:41, haraken wrote: > > > > > > tzik@ has been trying to invent a way to wrap an Oilpan pointer in > > > > WeakPtr<T> > > > > > > but hasn't yet succeeded at the attempt for some complicated reasons > > > (which > > > > > > tzik@ can explain in details). > > > > > > > > > > > > dcheng@: Is there any option to add a helper function inside Blink > that > > > > > provides > > > > > > the postCancellableTask-style API? I'm curious whether your concern is > > on > > > > > adding > > > > > > the postCancellableTask-style API to the public layer or on adding the > > API > > > > to > > > > > > the code base. If your concern is the former one, we can add a helper > > > > function > > > > > > only to Blink (not the public layer). > > > > > > > > > > It turned out even adding traits is confusing to match with the coding > > > > > convention of Oilpan. > > > > > We have to have 3 new variants of WeakPtrFactory for each > WeakPersistent, > > > > > CrossThreadWeakPersistent, and WeakMember, so that the owner of > > > WeakPtrFactory > > > > > can be GCed object or not. And we need 4 variants of WeakPtr for raw > > > pointer, > > > > > WP, CTWP, WM. > > > > > Also, WeakMember variants of WeakPtr and WeakPtrFactory need traceable. > > > > > > > > WeakPtrFactory isn't thread-safe, so CTWP isn't an option anyway. Is just > > > using > > > > WeakPersistent a big problem? > > > > > > Btw, to unblock things, I'm OK with proceeding with something like this if > we > > > can limit to Oilpan types. To me, it's not as bad if Oilpan has special > rules, > > > since it's memory model is not quite the same. > > > > > > In the long-term, I would still like to converge these, but we can figure > that > > > out without blocking further timer work. WDYT? > > > > dcheng@: Just to confirm, what do you mean by 'something like this'? Do you > mean > > the postCancellableTask API not exposed to the public layer? Or do you mean > > supporting many traits in WeakPtrFactory? > > Sorry, I was not clear. I mean the current approach in this CL to add a new API > in Blink. I don't have a strong preference where we expose it; I would just > prefer that we static_assert that it's used for Oilpan for now. WeakPtrFactory > has too many problems and we shouldn't block this work anymore. Thanks for the clarification; makes sense!
On 2016/09/30 02:32:54, haraken wrote: > On 2016/09/30 02:31:52, dcheng wrote: > > On 2016/09/30 02:01:56, haraken wrote: > > > On 2016/09/29 02:34:22, dcheng wrote: > > > > On 2016/09/29 01:30:14, dcheng wrote: > > > > > On 2016/09/28 13:59:15, tzik wrote: > > > > > > On 2016/09/28 09:03:41, haraken wrote: > > > > > > > tzik@ has been trying to invent a way to wrap an Oilpan pointer in > > > > > WeakPtr<T> > > > > > > > but hasn't yet succeeded at the attempt for some complicated reasons > > > > (which > > > > > > > tzik@ can explain in details). > > > > > > > > > > > > > > dcheng@: Is there any option to add a helper function inside Blink > > that > > > > > > provides > > > > > > > the postCancellableTask-style API? I'm curious whether your concern > is > > > on > > > > > > adding > > > > > > > the postCancellableTask-style API to the public layer or on adding > the > > > API > > > > > to > > > > > > > the code base. If your concern is the former one, we can add a > helper > > > > > function > > > > > > > only to Blink (not the public layer). > > > > > > > > > > > > It turned out even adding traits is confusing to match with the coding > > > > > > convention of Oilpan. > > > > > > We have to have 3 new variants of WeakPtrFactory for each > > WeakPersistent, > > > > > > CrossThreadWeakPersistent, and WeakMember, so that the owner of > > > > WeakPtrFactory > > > > > > can be GCed object or not. And we need 4 variants of WeakPtr for raw > > > > pointer, > > > > > > WP, CTWP, WM. > > > > > > Also, WeakMember variants of WeakPtr and WeakPtrFactory need > traceable. > > > > > > > > > > WeakPtrFactory isn't thread-safe, so CTWP isn't an option anyway. Is > just > > > > using > > > > > WeakPersistent a big problem? > > > > > > > > Btw, to unblock things, I'm OK with proceeding with something like this if > > we > > > > can limit to Oilpan types. To me, it's not as bad if Oilpan has special > > rules, > > > > since it's memory model is not quite the same. > > > > > > > > In the long-term, I would still like to converge these, but we can figure > > that > > > > out without blocking further timer work. WDYT? > > > > > > dcheng@: Just to confirm, what do you mean by 'something like this'? Do you > > mean > > > the postCancellableTask API not exposed to the public layer? Or do you mean > > > supporting many traits in WeakPtrFactory? > > > > Sorry, I was not clear. I mean the current approach in this CL to add a new > API > > in Blink. I don't have a strong preference where we expose it; I would just > > prefer that we static_assert that it's used for Oilpan for now. WeakPtrFactory > > has too many problems and we shouldn't block this work anymore. > > Thanks for the clarification; makes sense! Why is this actually better than CancellableTaskFactory?
On 2016/10/04 09:43:59, alex clarke wrote: > On 2016/09/30 02:32:54, haraken wrote: > > On 2016/09/30 02:31:52, dcheng wrote: > > > On 2016/09/30 02:01:56, haraken wrote: > > > > On 2016/09/29 02:34:22, dcheng wrote: > > > > > On 2016/09/29 01:30:14, dcheng wrote: > > > > > > On 2016/09/28 13:59:15, tzik wrote: > > > > > > > On 2016/09/28 09:03:41, haraken wrote: > > > > > > > > tzik@ has been trying to invent a way to wrap an Oilpan pointer in > > > > > > WeakPtr<T> > > > > > > > > but hasn't yet succeeded at the attempt for some complicated > reasons > > > > > (which > > > > > > > > tzik@ can explain in details). > > > > > > > > > > > > > > > > dcheng@: Is there any option to add a helper function inside Blink > > > that > > > > > > > provides > > > > > > > > the postCancellableTask-style API? I'm curious whether your > concern > > is > > > > on > > > > > > > adding > > > > > > > > the postCancellableTask-style API to the public layer or on adding > > the > > > > API > > > > > > to > > > > > > > > the code base. If your concern is the former one, we can add a > > helper > > > > > > function > > > > > > > > only to Blink (not the public layer). > > > > > > > > > > > > > > It turned out even adding traits is confusing to match with the > coding > > > > > > > convention of Oilpan. > > > > > > > We have to have 3 new variants of WeakPtrFactory for each > > > WeakPersistent, > > > > > > > CrossThreadWeakPersistent, and WeakMember, so that the owner of > > > > > WeakPtrFactory > > > > > > > can be GCed object or not. And we need 4 variants of WeakPtr for raw > > > > > pointer, > > > > > > > WP, CTWP, WM. > > > > > > > Also, WeakMember variants of WeakPtr and WeakPtrFactory need > > traceable. > > > > > > > > > > > > WeakPtrFactory isn't thread-safe, so CTWP isn't an option anyway. Is > > just > > > > > using > > > > > > WeakPersistent a big problem? > > > > > > > > > > Btw, to unblock things, I'm OK with proceeding with something like this > if > > > we > > > > > can limit to Oilpan types. To me, it's not as bad if Oilpan has special > > > rules, > > > > > since it's memory model is not quite the same. > > > > > > > > > > In the long-term, I would still like to converge these, but we can > figure > > > that > > > > > out without blocking further timer work. WDYT? > > > > > > > > dcheng@: Just to confirm, what do you mean by 'something like this'? Do > you > > > mean > > > > the postCancellableTask API not exposed to the public layer? Or do you > mean > > > > supporting many traits in WeakPtrFactory? > > > > > > Sorry, I was not clear. I mean the current approach in this CL to add a new > > API > > > in Blink. I don't have a strong preference where we expose it; I would just > > > prefer that we static_assert that it's used for Oilpan for now. > WeakPtrFactory > > > has too many problems and we shouldn't block this work anymore. > > > > Thanks for the clarification; makes sense! > > Why is this actually better than CancellableTaskFactory? See this thread for why the current CancellableTaskFactory is not nice: https://groups.google.com/a/chromium.org/d/topic/platform-architecture-dev/f1... You suggested the postCancellableTask API (although in the context of the task-handle-based implementation at that point), and as far as I read the thread, people liked the API. That's why we've tried to use the postCancellableTask API (on top of the weakptr-based implementation). In any case, I'm getting tired of repeating the same discussion again and again for one month. If you have any concerns, please raise all of your concerns and your suggestions on this thread. I'm happy to VC.
On 2016/10/04 11:46:23, haraken wrote: > On 2016/10/04 09:43:59, alex clarke wrote: > > On 2016/09/30 02:32:54, haraken wrote: > > > On 2016/09/30 02:31:52, dcheng wrote: > > > > On 2016/09/30 02:01:56, haraken wrote: > > > > > On 2016/09/29 02:34:22, dcheng wrote: > > > > > > On 2016/09/29 01:30:14, dcheng wrote: > > > > > > > On 2016/09/28 13:59:15, tzik wrote: > > > > > > > > On 2016/09/28 09:03:41, haraken wrote: > > > > > > > > > tzik@ has been trying to invent a way to wrap an Oilpan pointer > in > > > > > > > WeakPtr<T> > > > > > > > > > but hasn't yet succeeded at the attempt for some complicated > > reasons > > > > > > (which > > > > > > > > > tzik@ can explain in details). > > > > > > > > > > > > > > > > > > dcheng@: Is there any option to add a helper function inside > Blink > > > > that > > > > > > > > provides > > > > > > > > > the postCancellableTask-style API? I'm curious whether your > > concern > > > is > > > > > on > > > > > > > > adding > > > > > > > > > the postCancellableTask-style API to the public layer or on > adding > > > the > > > > > API > > > > > > > to > > > > > > > > > the code base. If your concern is the former one, we can add a > > > helper > > > > > > > function > > > > > > > > > only to Blink (not the public layer). > > > > > > > > > > > > > > > > It turned out even adding traits is confusing to match with the > > coding > > > > > > > > convention of Oilpan. > > > > > > > > We have to have 3 new variants of WeakPtrFactory for each > > > > WeakPersistent, > > > > > > > > CrossThreadWeakPersistent, and WeakMember, so that the owner of > > > > > > WeakPtrFactory > > > > > > > > can be GCed object or not. And we need 4 variants of WeakPtr for > raw > > > > > > pointer, > > > > > > > > WP, CTWP, WM. > > > > > > > > Also, WeakMember variants of WeakPtr and WeakPtrFactory need > > > traceable. > > > > > > > > > > > > > > WeakPtrFactory isn't thread-safe, so CTWP isn't an option anyway. Is > > > just > > > > > > using > > > > > > > WeakPersistent a big problem? > > > > > > > > > > > > Btw, to unblock things, I'm OK with proceeding with something like > this > > if > > > > we > > > > > > can limit to Oilpan types. To me, it's not as bad if Oilpan has > special > > > > rules, > > > > > > since it's memory model is not quite the same. > > > > > > > > > > > > In the long-term, I would still like to converge these, but we can > > figure > > > > that > > > > > > out without blocking further timer work. WDYT? > > > > > > > > > > dcheng@: Just to confirm, what do you mean by 'something like this'? Do > > you > > > > mean > > > > > the postCancellableTask API not exposed to the public layer? Or do you > > mean > > > > > supporting many traits in WeakPtrFactory? > > > > > > > > Sorry, I was not clear. I mean the current approach in this CL to add a > new > > > API > > > > in Blink. I don't have a strong preference where we expose it; I would > just > > > > prefer that we static_assert that it's used for Oilpan for now. > > WeakPtrFactory > > > > has too many problems and we shouldn't block this work anymore. > > > > > > Thanks for the clarification; makes sense! > > > > Why is this actually better than CancellableTaskFactory? > > See this thread for why the current CancellableTaskFactory is not nice: > https://groups.google.com/a/chromium.org/d/topic/platform-architecture-dev/f1... Sorry I forgot about that thread, yes getting rid of CancellableTaskFactory makes sense now. > > You suggested the postCancellableTask API (although in the context of the > task-handle-based implementation at that point), and as far as I read the > thread, people liked the API. That's why we've tried to use the > postCancellableTask API (on top of the weakptr-based implementation). > > In any case, I'm getting tired of repeating the same discussion again and again > for one month. If you have any concerns, please raise all of your concerns and > your suggestions on this thread. I'm happy to VC. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
