585661 - Consider borrowing wtf's CrossThreadRefCounted for strings

Status: NEW

(Core :: XPCOM, defect)

Description

[Boris Zbarsky [:bzbarsky]]

wtf has an interesting class called CrossThreadRefCounted which has the following properties:

1) It points to some data (string data in the case I care about).
2) To quote the header:

   // Is this class threadsafe?
   // While each instance of the class is not threadsafe, the copied instance
   // is threadsafe with respect to the original and any other copies.  The
   // underlying m_data is jointly owned by the original instance and all
   // copies.

They seem to implement this by having two separate refcounts: a thread-unsafe per-instance one, and a shard threadsafe one that counts how many different CrossThreadRefCounteds are around for the given data.  Their deref method decrements the local refcount, and it that hits zero decrements the threadsafe one and deletes this.  If the threadsafe refcount goes to 0, the threadsafe refcounter and the data are also deleted.

The big caveat is that there's a crossThreadCopy method involved.  So they seem to have a bunch of crossThreadString() calls (which does the crossThreadCopy) around...  not sure how to best address that.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Olli, you may be interested in this.

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

So does this mean having a setup like:

nsString -> CrossThreadRefCounted -> nsStringBuffer

The refcount in CrossThreadRefCounted would not be threadsafe, but the refcount in nsStringBuffer would be. Is this the setup they use?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Their setup is:

* Their UString class (which other stuff is built on), which lives in
  JavaScriptCore/runtime/UString.h, has a RefPtr<Rep> member.
* The Rep struct has this member (in addition to some others):

    union {
      BaseString* m_baseString;
      SharedUChar* m_sharedBuffer;
    };

  where BaseString seems to be a subclass of Rep that has an editable buffer
  and whatnot (a shared buffer is of course not editable).
* SharedUChar is a typedef for CrossThreadRefCounted<OwnFastMallocPtr<UChar> >
* CrossThreadRefCounted.h lives in JavaScriptCore/wtf and
  CrossThreadRefCounted<T> has three members: a T*, a RefCountedBase, and a
  ThreadSafeSharedBase.  In this case, T would be OwnFastMallocPtr<UChar>,
  whatever that is.

Inside the CrossThreadRefCounted, the RefCountedBase is used for the non-threadsafe refcount, and the ThreadSafeSharedBase, if non-null, indicates that the T* is in use on multiple threads and keeps track of the number of live 
CrossThreadRefCounted instances pointing to the shared buffer.  Note that Rep itself is also refcounted and has its own refcount...

The whole thing is really complicated and not much documented at all, and I haven't dug into it quite so much.  I don't _think_ we need all that complexity... but the concept of not making all strings pay the cost of the few places we pass strings cross-thread is something that we should seriously look into.

Comment 4

[Justin Lebar (not reading bugmail)]

I'm going to play around with a simpler approach:

 * Keep a main-thread refcnt and a global refcnt.
 * In addref / release, check whether we're the main thread.  If so, modify only the main-thread refcnt using a regular, thread-unsafe operation.  When the main-thread refcnt goes from 1 to 0, atomically decrement the global refcnt and, if the new global refcnt is 0, delete the object.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> In addref / release, check whether we're the main thread.

Is this really fast?

Comment 6

[Justin Lebar (not reading bugmail)]

It uses thread-local storage afaict.  I'm not sure how fast that is, but it's plausibly faster than an atomic op.

Comment 7

[Justin Lebar (not reading bugmail)]

Didn't mean to hijack this bug; I filed bug 593231.

Comment 8

[Justin Lebar (not reading bugmail)]

To try and get a bound on the speedup we might expect from this, I changed nsStringBuffer to use thread-unsafe refcount operations (simple ++ and --).  I ran Dromaeo on my fast Linux x64 box, and it was remarkably stable!

The benchmark ran faster, but nowhere was it a more than 1% improvement.

There might be more to be done here; I'll poke around some more.

Comment 9

[Nathan Froyd [:froydnj]]

I think the work dbaron did in bug 1353181 to use weaker memory synchronization in string buffer refcounting gives us some of the benefits the proposed work here would with a much smaller amount of work.