Oilpan and the loss of unique ownership

[Daniel Cheng]

In Oilpan, one thing I find myself missing is the concept of unique ownership.

There are currently many classes in Blink where the ownership (pre-Oilpan) is well-defined. The create factory() function returns a PassOwnPtr: it's easy to trace the code and see who owns it, when it's created, and when it's destroyed.

With Oilpan, we lose this. Compare:

class FrameLoader : ... {

    // From the type, it is obvious that the lifetime of

    // ProgressTracker is tied to the lifetime of FrameLoader.

    OwnPtr<ProgressTracker> m_progressTracker;

};

vs

class FrameLoader : ... {

    // No obvious lifetime connection. The only way a reader will know that

    // the lifetime is connected is searching for a comment (which doesn't

    // exist today) or searching the codebase for any other strong references

    // to PageTracker.

    Member<ProgressTracker> m_progressTracker;

};

A strawman proposal:

- Objects with unique ownership semantics should inherit from UniqueGarbageCollected<T> (or UniqueGarbageCollectedFinalized<T>, etc)

- Objects with unique ownership can only be stored in UniqueMember<T>. UniqueMember<T> is move-only.

- GarbageCollected<T> objects cannot be stored in a UniqueMember<T> and UniqueGarbageCollected<T> objects cannot be stored in a Member<T>.

Thoughts? The Oilpan API is not the simplest, but for clarity, I think it's important to still be able to express this concept in Oilpan.

Daniel

[Kentaro Hara]

Yeah, I understand your concern. This has raised as one of the disadvantages of oilpan a couple of times. I want some solution on this.

On Fri, Nov 20, 2015 at 7:58 AM, Daniel Cheng <dch...@chromium.org> wrote:

In Oilpan, one thing I find myself missing is the concept of unique ownership.

There are currently many classes in Blink where the ownership (pre-Oilpan) is well-defined. The create factory() function returns a PassOwnPtr: it's easy to trace the code and see who owns it, when it's created, and when it's destroyed.

With Oilpan, we lose this. Compare:

class FrameLoader : ... {

    // From the type, it is obvious that the lifetime of

    // ProgressTracker is tied to the lifetime of FrameLoader.

    OwnPtr<ProgressTracker> m_progressTracker;

};

vs

class FrameLoader : ... {

    // No obvious lifetime connection. The only way a reader will know that

    // the lifetime is connected is searching for a comment (which doesn't

    // exist today) or searching the codebase for any other strong references

    // to PageTracker.

    Member<ProgressTracker> m_progressTracker;

};

A strawman proposal:

- Objects with unique ownership semantics should inherit from UniqueGarbageCollected<T> (or UniqueGarbageCollectedFinalized<T>, etc)

- Objects with unique ownership can only be stored in UniqueMember<T>. UniqueMember<T> is move-only.

- GarbageCollected<T> objects cannot be stored in a UniqueMember<T> and UniqueGarbageCollected<T> objects cannot be stored in a Member<T>.

One problem of the API is that we need to use raw pointers for the remaining pointers to the T (because we cannot use Members for the remaining pointers) and ask developers to make sure that the remaining pointers are cleared before the UniqueMember<T> gets destructed. This loses the advantage of Oilpan; i.e., all pointers are traced by Oilpan and thus developers don't need to worry about producing stale pointers...

Hmm, let me think about it a bit more.

 

Thoughts? The Oilpan API is not the simplest, but for clarity, I think it's important to still be able to express this concept in Oilpan.

Daniel

--

Kentaro Hara, Tokyo, Japan

[Daniel Cheng]

(Sorry to resurrect this old thread)

Couldn't we just say that all other heap references must be via a WeakMember<T>? This would give us unique ownership and Oilpan's tracing, right?

Daniel

[Kentaro Hara]

Couldn't we just say that all other heap references must be via a WeakMember<T>? This would give us unique ownership and Oilpan's tracing, right?

Yeah, we could do that, but I'm not quite sure how nice the programming model is.

If you use the UniqueMember & WeakMember model, you need to guarantee either of the following facts:

- Add an if(!m_xxx) check for all places where you use WeakMembers, because the WeakMember can be auto-cleared anytime.

- Refactor code to make sure that all WeakMembers are cleared explicitly (Pre-oilpan, we were doing this). Then add ASSERT(!m_xxx) to WeakMember::get.

[Daniel Cheng]

On Mon, Jan 11, 2016 at 5:02 PM Kentaro Hara <har...@chromium.org> wrote:

Couldn't we just say that all other heap references must be via a WeakMember<T>? This would give us unique ownership and Oilpan's tracing, right?

Yeah, we could do that, but I'm not quite sure how nice the programming model is.

If you use the UniqueMember & WeakMember model, you need to guarantee either of the following facts:

- Add an if(!m_xxx) check for all places where you use WeakMembers, because the WeakMember can be auto-cleared anytime.

- Refactor code to make sure that all WeakMembers are cleared explicitly (Pre-oilpan, we were doing this). Then add ASSERT(!m_xxx) to WeakMember::get.

I'm not sure that this situation will come up that often. For example, I just did a quick search for NavigationScheduler and ProgressTracker, and there seems to be only one heap reference to those classes. The same is true for several other classes stored in OwnPtrWillBeMembers. I'm sure there will be some classes where this won't be true, and for those things, it might not make sense to change to the proposed UniqueMember<T> at all.

How about I put together a draft CL and see how it looks?

Daniel

[Kentaro Hara]

On Tue, Jan 12, 2016 at 12:01 PM, Daniel Cheng <dch...@chromium.org> wrote:

On Mon, Jan 11, 2016 at 5:02 PM Kentaro Hara <har...@chromium.org> wrote:

Couldn't we just say that all other heap references must be via a WeakMember<T>? This would give us unique ownership and Oilpan's tracing, right?

Yeah, we could do that, but I'm not quite sure how nice the programming model is.

If you use the UniqueMember & WeakMember model, you need to guarantee either of the following facts:

- Add an if(!m_xxx) check for all places where you use WeakMembers, because the WeakMember can be auto-cleared anytime.

- Refactor code to make sure that all WeakMembers are cleared explicitly (Pre-oilpan, we were doing this). Then add ASSERT(!m_xxx) to WeakMember::get.

I'm not sure that this situation will come up that often. For example, I just did a quick search for NavigationScheduler and ProgressTracker, and there seems to be only one heap reference to those classes. The same is true for several other classes stored in OwnPtrWillBeMembers. I'm sure there will be some classes where this won't be true, and for those things, it might not make sense to change to the proposed UniqueMember<T> at all.

How about I put together a draft CL and see how it looks?

That would depend on how much you want to replace. If you want to replace all Members that had been using OwnPtrs, that would be many (Note that we've already replaced a bunch of OwnPtrWillBeMembers with Members). Otherwise, I'm not quite sure of the criteria where we should use UniqueMember.

Hmm, I agree with you that there should be a way to express ownership in the oilpan model but I'm not yet fully convinced about the UniqueMember.

[Daniel Cheng]

On Mon, Jan 11, 2016 at 8:47 PM Kentaro Hara <har...@chromium.org> wrote:

On Tue, Jan 12, 2016 at 12:01 PM, Daniel Cheng <dch...@chromium.org> wrote:

On Mon, Jan 11, 2016 at 5:02 PM Kentaro Hara <har...@chromium.org> wrote:

Couldn't we just say that all other heap references must be via a WeakMember<T>? This would give us unique ownership and Oilpan's tracing, right?

Yeah, we could do that, but I'm not quite sure how nice the programming model is.

If you use the UniqueMember & WeakMember model, you need to guarantee either of the following facts:

- Add an if(!m_xxx) check for all places where you use WeakMembers, because the WeakMember can be auto-cleared anytime.

- Refactor code to make sure that all WeakMembers are cleared explicitly (Pre-oilpan, we were doing this). Then add ASSERT(!m_xxx) to WeakMember::get.

I'm not sure that this situation will come up that often. For example, I just did a quick search for NavigationScheduler and ProgressTracker, and there seems to be only one heap reference to those classes. The same is true for several other classes stored in OwnPtrWillBeMembers. I'm sure there will be some classes where this won't be true, and for those things, it might not make sense to change to the proposed UniqueMember<T> at all.

How about I put together a draft CL and see how it looks?

That would depend on how much you want to replace. If you want to replace all Members that had been using OwnPtrs, that would be many (Note that we've already replaced a bunch of OwnPtrWillBeMembers with Members). Otherwise, I'm not quite sure of the criteria where we should use UniqueMember.

I don't think it's necessary to convert everything that was an OwnPtr to UniqueMember. For a draft CL, I'd likely just start with a small subset of the ones I find in Page.cpp and LocalFrame.cpp. The initial criteria for using UniqueMember can be very simple: anything that only has one heap reference to it should prefer to use a UniqueMember when possible.

 

Hmm, I agree with you that there should be a way to express ownership in the oilpan model but I'm not yet fully convinced about the UniqueMember.

Another instance where this would be useful is in the newly added IntersectionObserver code. Since Oilpan supports collecting reference cycles, IntersectionObservation::m_target could actually be a Member<Element>: the only strong reference to an IntersectionObservation should be from its target element.

However, nothing prevents another heap object from incorrectly storing an IntersectionObservation as a Member<T>, so to prevent an IntersectionObservation and its target node from unexpectedly leaking, IntersectionObservation::m_target must be a WeakMember<T> instead.

[Elliott Sprehn]

I'd like us to avoid adding this complexity right now. Oilpan already has way too many types. Let's ship it 100% first and remove all the WillBe types before we add even more types.

[Kentaro Hara]

On Tue, Jan 12, 2016 at 2:46 PM, Elliott Sprehn <esp...@chromium.org> wrote:

I'd like us to avoid adding this complexity right now. Oilpan already has way too many types. Let's ship it 100% first and remove all the WillBe types before we add even more types.

Yeah, I'm leaning toward to Elliott's point. I understand Daniel's point but don't want to add more complexity before shipping.

[Daniel Cheng]

On Mon, Jan 11, 2016 at 9:50 PM Kentaro Hara <har...@chromium.org> wrote:

On Tue, Jan 12, 2016 at 2:46 PM, Elliott Sprehn <esp...@chromium.org> wrote:

I'd like us to avoid adding this complexity right now. Oilpan already has way too many types. Let's ship it 100% first and remove all the WillBe types before we add even more types.

Yeah, I'm leaning toward to Elliott's point. I understand Daniel's point but don't want to add more complexity before shipping.

OK, fair enough, another set of WillBe types would definitely not be an improvement =)

Daniel