Experimental: How to implement willFinalize()?

Experimental: How to implement willFinalize()?

BUG=none

[zerny-chromium, 2014-03-13 06:41:46]

This pattern should work and I'm ok with adding it to the heap tests. I'm not
sure we should move the Observer clas into Handle.h. At least we should first
solve some of the concrete issues and if there is a clear pattern we can
abstract that out.

lgtm

https://codereview.chromium.org/198563002/diff/1/Source/heap/HeapTest.cpp
File Source/heap/HeapTest.cpp (right):

https://codereview.chromium.org/198563002/diff/1/Source/heap/HeapTest.cpp#new...
Source/heap/HeapTest.cpp:897: T* m_data;
Nit: WeakMember<T> m_data;

https://codereview.chromium.org/198563002/diff/1/Source/heap/HeapTest.cpp#new...
Source/heap/HeapTest.cpp:931: // This is a trick. Use Persistent for Observer<T>
to avoid it is collected
it is collected -> it from being collected

[kouhei (in TOK), 2014-03-13 06:56:06]

If this is ok, can we reconsider GarbageCollectedDelayedFinalized? It is doing
the exactly same thing, but will not require Observer<>.

[haraken, 2014-03-13 07:00:12]

I think that it would be much better if we could support touch-on-heap-object in
destructors (I think we should experiment once the
object-allocation-during-destructor issue is resolved), but if it's hard for
performance reasons I'm OK with this pattern.

[haraken, 2014-03-13 07:01:57]

On 2014/03/13 06:56:06, kouhei wrote:
> If this is ok, can we reconsider GarbageCollectedDelayedFinalized? It is doing
> the exactly same thing, but will not require Observer<>.

The benefit of this approach over DelayedFinalized is that we don't need to add
a new, complicated infrastructure to oilpan. This approach uses
registerWeakMembers(), which is an existing infrastructure.

[zerny-chromium, 2014-03-13 07:09:55]

>
> The benefit of this approach over DelayedFinalized is that we don't need
> to add
>  a new, complicated infrastructure to oilpan. This approach uses
> registerWeakMembers(), which is an existing infrastructure.
>

Exactly. And also, the Observer class is not really needed. I'd expect that
usually we will already have a class that is weakly observing some other
class. In any case, we should look at solving some concrete problems with
the existing infrastruction for weak callbacks. After that we can look at
abstracting and adding infrastructure.

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

[Mads Ager (chromium), 2014-03-13 07:33:27]

This makes sense, it is implementing Kouhei's pre death callback using the
existing infrastructure. We still have to be aware that the pre death callback
is limited. No allocation is allowed because we are getting this callback during
garbage collection. 

We should implement this without having a persistent per object that we want to
observer.

https://codereview.chromium.org/198563002/diff/1/Source/heap/HeapTest.cpp
File Source/heap/HeapTest.cpp (right):

https://codereview.chromium.org/198563002/diff/1/Source/heap/HeapTest.cpp#new...
Source/heap/HeapTest.cpp:931: // This is a trick. Use Persistent for Observer<T>
to avoid it is collected
Instead of embedding the Persistent in the object itself I would prefer to have
one static PersistentHashSet<Member<Observer<WillFinalized> > > observers. When
the observer dies it can pull itself out of the set (or pass itself to
willFinalize and it can be pulled out there).

[Mads Ager (chromium), 2014-03-13 07:42:28]

There are multiple things that you have to be aware of when writing the code in
this callback. You cannot revive objects for instance. Marking is over and we
have decided which objects are going to die. Therefore, you cannot take a
pointer to another heap object and pass it somewhere in this callback unless it
is alive. Objects that are not alive *will* be destructed in this GC round and
there is nothing you can do about it (and we don't want to support that.)

So, the bottom line is that this pattern can be used if you have some fairly
simple cleanup that you have to do with access to the fields of an object.
However, you have to be very careful and we should use this very rarely.

[tkent, 2014-03-13 07:51:30]

ok, I'll:

1. Make a CL for HeapTest.cpp to verify lifetime of on-heap objects.  I won't
add WillFinalized class in this CL to HeapTest.cpp, and won't add anything to
Handle.h.
2. Try to resolve the SQLTransactionSync issue with this pattern.

[Mads Ager (chromium), 2014-03-13 07:53:38]

Sounds good! :)


On Thu, Mar 13, 2014 at 8:51 AM, <tkent@chromium.org> wrote:

> Reviewers: zerny-chromium, haraken, Mads Ager (chromium),
>
> Message:
> ok, I'll:
>
> 1. Make a CL for HeapTest.cpp to verify lifetime of on-heap objects.  I
> won't
> add WillFinalized class in this CL to HeapTest.cpp, and won't add anything
> to
> Handle.h.
> 2. Try to resolve the SQLTransactionSync issue with this pattern.
>
>
>
> Description:
> Experimental: How to implement willFinalize()?
>
> BUG=none
>
> Please review this at https://codereview.chromium.org/198563002/
>
> SVN Base: svn://svn.chromium.org/blink/trunk
>
> Affected files (+89, -0 lines):
>   M Source/heap/HeapTest.cpp
>
>
> Index: Source/heap/HeapTest.cpp
> diff --git a/Source/heap/HeapTest.cpp b/Source/heap/HeapTest.cpp
> index 9cd048780a643d7955f177fcf0db06f2ac18d55e..
> bbbb821ea9ff267b008f0ea18c8427f4f3dca0d7 100644
> --- a/Source/heap/HeapTest.cpp
> +++ b/Source/heap/HeapTest.cpp
> @@ -531,6 +531,7 @@ public:
>          m_magic = 0;
>          s_live--;
>      }
> +    bool isLive() const { return m_magic; }
>
>      virtual void trace(Visitor* visitor) { }
>      static unsigned s_live;
> @@ -562,6 +563,13 @@ public:
>
>      void clear() { m_bar.release(); }
>
> +    // willFinalize is called before finalize()/destructor.
> +    void willFinalize()
> +    {
> +        EXPECT_TRUE(m_bar->isLive());
> +        EXPECT_EQ(1u, Bar::s_live);
> +    }
> +
>  private:
>      explicit Baz(Bar* bar)
>          : m_bar(bar)
> @@ -865,6 +873,65 @@ private:
>      WeakMember<Bar> m_weakBar;
>  };
>
> +// FIXME: This should be moved to heap/Handle.h.
> +template <typename T> class Observer : public
> GarbageCollected<Observer<T>
>
>> {
>>
> +public:
> +    static Observer* create(T* data) { return new Observer(data); }
> +
> +    void trace(Visitor* visitor)
> +    {
> +        visitor->registerWeakMembers(this, zapWeakMembers);
> +    }
> +
> +    static void zapWeakMembers(Visitor* visitor, void* self)
> +    {
> +        Observer* o = reinterpret_cast<Observer*>(self);
> +        if (o->m_data && !visitor->isAlive(o->m_data)) {
> +            o->m_data->willFinalize();
> +            o->m_data = 0;
> +        }
> +    }
> +
> +private:
> +    Observer(T* data) : m_data(data) { }
> +    T* m_data;
> +};
> +
> +// This demonstrates how to use Observer<T> as a part object of the
> target.
> +class WillFinalized : public GarbageCollectedFinalized<WillFinalized> {
> +public:
> +    WillFinalized(Bar* bar)
> +        : m_data(2014)
> +        , m_bar(bar)
> +        , m_observer(Observer<WillFinalized>::create(this))
> +    {
> +    }
> +
> +    // willFinalize is called before finalize()/destructor.
> +    void willFinalize()
> +    {
> +        EXPECT_NE(0, m_data);
> +        EXPECT_TRUE(m_bar->isLive());
> +        m_data = 0;
> +    }
> +
> +    ~WillFinalized()
> +    {
> +        EXPECT_EQ(0, m_data);
> +    }
> +
> +    void trace(Visitor* visitor)
> +    {
> +        visitor->trace(m_bar);
> +    }
> +
> +private:
> +    int m_data;
> +    Member<Bar> m_bar;
>
> +    // This is a trick. Use Persistent for Observer<T> to avoid it is
> collected
> +    // with the target.
> +    Persistent<Observer<WillFinalized> > m_observer;
> +};
>
>  class SuperClass;
>
> @@ -2481,6 +2548,28 @@ TEST(HeapTest, WeakMembers)
>      EXPECT_EQ(0u, Bar::s_live); // All gone.
>  }
>
> +TEST(HeapTest, WillFinalize)
> +{
> +    // |o| observes |baz|.
> +    Persistent<Observer<Baz> > o;
> +    {
> +        Baz* baz = Baz::create(Bar::create());
> +        o = Observer<Baz>::create(baz);
> +    }
> +    // Observer doesn't have a strong reference to baz. So baz and its
> member
> +    // will be collected.
> +    Heap::collectGarbage(ThreadState::NoHeapPointersOnStack);
> +    EXPECT_EQ(0u, Bar::s_live);
> +
> +    // |wf| observes itself, and calls willFinalize().
> +    {
> +        WillFinalized* wf = new WillFinalized(Bar::create());
> +        wf = 0;
> +    }
> +    Heap::collectGarbage(ThreadState::NoHeapPointersOnStack);
> +    EXPECT_EQ(0u, Bar::s_live);
> +}
> +
>  TEST(HeapTest, Comparisons)
>  {
>      Persistent<Bar> barPersistent = Bar::create();
>
>
>

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