421127 - Decouple thebes refcounting from XPCOM refcounting

Status: RESOLVED INCOMPLETE

(Core :: Graphics, defect)

Description

[Benjamin Smedberg]

In XPCOMGC I'm leaving thebes alone, in all its refcounted glory. In order that thebes doesn't accidentally get caught up in the XPCOMGC work, I have a patch which replaces nsRefPtr with gfxRefPtr for thebes classes.

The patch here works on Linux... still needs testing on Mac/Win.

Comment 1

[Benjamin Smedberg]

Attachment: gfxRefPtr and friends, rev. 1

Comment 2

[Stuart Parmenter]

Comment on attachment 308785 [details] [diff] [review]
gfxRefPtr and friends, rev. 1

ugh.  imho, this stuff really belongs in some core set of code (xpcom or some new dumping ground for general purpose stuff)

I would keep nsAutoPtr, NS_ADDREF, NS_RELEASE, etc.

I certainly wouldn't change the AddRef and Release methods to gfxAddRef and gfxRelease.  There is nothing gfx specific about these.

We, as a platform, should have this functionality somewhere outside of gfx.

Comment 3

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

I agree with stuart -- even in an xpcom-gc world, we will have things that we want to be refcounted; we should have a generic way of doing that.

Comment 4

[Benjamin Smedberg]

I disagree pretty strongly, for both practical and theoretical reasons:

* Because nsRefPtr is currently used for reference counting of things that will become GC objects, it is becoming an alias for the new write barrier class. So we're going to have to rename the current references from nsRefPtr<gfx*> to someotherRefPtr<gfx*> anyway

* In order to implement XPCOMGC effectively NS_ADDREF is becoming a no-op and NS_RELEASE is becoming (arg = nsnull)... and why are the macros better than an explicit call to addref/release?

* The simplest way to make sure that using nsRefPtr on a gfx class is a compile-time error now is to rename AddRef/Release on those classes.

* I'm not switching thebes objects from refcounted to GCed now, but I'm not sure that we might not want to make thebes into GCed objects in the future; keeping separate refcounting types for thebes will make it much easier to do this kind of switch in the future

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

moz::RefPtr or moz::ref_ptr and I'm good with it!

Comment 6

[Ben Turner (not reading bugmail, use the needinfo flag!)]

moz::RefPtr gets my vote!

Comment 7

[Benjamin Smedberg]

Attachment: Add more template love to hashtables, rev. 1

This patch templates the hashtable getter function so that you can pass in any type* which can be assigned from the value.

Comment 8

[Benjamin Smedberg]

Attachment: Core RefPtr support, build system, rev. 1

Comment 9

[Benjamin Smedberg]

Attachment: Use the new RefPtr in thebes, rev. 1

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 311858 [details] [diff] [review]
Add more template love to hashtables, rev. 1

>+   * The following signature exists only to disambigate "nsnull" passed
>+   * as pData.
>+   */
>+  PRBool Get(KeyType aKey, UserDataType* pData) const
>+  {
>+    return Get(aKey, pData);
>+  }

Could you explain why this doesn't call itself?

Also, disambiguate has a "u" in it.


Other than that this looks fine; sorry for the long delay.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 311860 [details] [diff] [review]
Core RefPtr support, build system, rev. 1

>diff --git a/core/Makefile.in b/core/Makefile.in
>new file mode 100644
>--- /dev/null
>+++ b/core/Makefile.in


Brendan needs to approve the new toplevel dir.  I'm not crazy about this one.

>diff --git a/core/RefPtr.h b/core/RefPtr.h

An explanation of the purpose of this file (what it's for, why somebody would use it) would be useful, probably in comments at the top.  Both to me (reviewer), and to everyone else (users).

>+#ifndef mozilla_RefPtr_h__
>+#define mozilla_RefPtr_h__

names with "__" are implementation-reserved; include guards should not use it.

>+namespace mozilla
>+{
>+    /**
>+     * A lightweight wrapper around uint32_t which auto-initializes to 0
>+     */
>+    class RefCnt
>+    {
>+    private:
>+        uint32_t mValue;

Should we consider using uintptr_t instead?

>+    template<class T>
>+    class RefPtr
>+    {
>+    private:
>+        T *mPtr;
>+
>+    public:
>+        RefPtr() : mPtr(NULL) { }
>+        RefPtr(const RefPtr<T> &copy)

maybe |other| rather than |copy|?

>+        T* operator=(RefPtr<T> &ref)
>+        {
>+            set(ref.mPtr);
>+            return mPtr;
>+        }
>+        T* operator=(T *ptr)
>+        {
>+            set(ptr);
>+            return ptr;
>+        }
>+        T* operator=(AddRefed<T> ref)
>+        {
>+            if (mPtr)
>+                mPtr->ReleaseReference();
>+
>+            mPtr = ref.get();
>+            return mPtr;
>+        }

It seems a little odd to return T* rather than RefPtr<T>&

>+        AddRefed<T> forget()
>+        {
>+            T *ptr = mPtr;
>+            mPtr = NULL;
>+            return AddRefed<T>(ptr);
>+        }
>+
>+        void forget(T **outparam)
>+        {
>+            *outparam = mPtr;
>+            mPtr = NULL;
>+        }

Do you want NULL or nsnull?  You refer to "nsnull" in comments above.

>+        T** assign_assuming_addref()
>+        {
>+            set(NULL);
>+            return &mPtr;
>+        }

Could you call this something else?  It doesn't actually do an assignment.  nsRefPtr/nsCOMPtr use begin_assignment().

>+    /**
>+     * This is a return type which can become a null AddRefed of
>+     * any type.
>+     */
>+    class
>+        NS_STACK_CLASS
>+    NullRefType
>+    {
>+        template<class T>
>+        operator AddRefed<T>()
>+        {
>+            return AddRefed<T>(NULL);
>+        }
>+    };
>+
>+    inline NullRefType
>+    NullRef()
>+    {
>+        return NullRefType();
>+    }

Why NullRef rather than NullAddRefed?


>+#define DECL_INLINE_REFCOUNTING(_class)                                 \
>+    protected:                                                          \
>+        mozilla::RefCnt mRefCnt;                                        \
>+    public:                                                             \
>+        void AddReference()                                             \
>+        {                                                               \
>+            ++mRefCnt;                                                  \
>+        }                                                               \
>+        void ReleaseReference()                                         \
>+        {                                                               \
>+            NS_PRECONDITION(mRefCnt, "Duplicate release?");             \
>+            --mRefCnt;                                                  \
>+            if (mRefCnt == 0)                                           \
>+                delete this;                                            \
>+        }

How about NS_LOG_ADDREF and NS_LOG_RELEASE hooks?

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 311858 [details] [diff] [review]
Add more template love to hashtables, rev. 1

Should have marked these at the time I commented.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

In bug 461087 Karl added nsAutoRef/nsCountedRef:
http://mxr.mozilla.org/mozilla-central/source/xpcom/base/nsAutoRef.h

Seems to me any fix for this bug should be merged into that. If this is the place to introduce namespaces, then we can still quite easily move nsAutoRef to namespaces, since it's not used much yet.

Comment 14

[Benjamin Smedberg]

The old XPCOMGC project is dead, going to INCOMPLETE this.