208437 - separate threadsafe nsBaseHashtable into superclass

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Benjamin Smedberg]

Codesize numbers for nsBaseHashtable are looking pretty rotten, mainly because
of the thread-safety code. Moving this code into a super-class allows inlining
and significant codesize shrinkage for normal cases.

Comment 1

[Benjamin Smedberg]

Attachment: nsBaseHashtableL super-class

Basically, this patch adds nsBaseHashtableL (and Data/Interface/Class) as a
superclass of nsBaseHashtable.

Comment 2

[Alec Flett]

lets not unnecessarily abbreviate here. There must be something better than "L"?
lockable? threadsafe? synchronized? (ok, that last one is a java joke)

Comment 3

[Alec Flett]

Comment on attachment 125011 [details] [diff] [review]
nsBaseHashtableL super-class

this seems fine, aside from the name. make the "L" a word, and [s]r=alecf. I'd
still like to get dougt's eyes on this, so I'm marking this r=alecf, but
requesting r=dougt too

Comment 4

[Doug Turner (:dougt)]

Comment on attachment 125011 [details] [diff] [review]
nsBaseHashtableL super-class

what alec said about the 'L' -- "Locked" works for me.	Or "ThreadSafe" even.

remind me again why these class destructors's aren't virtual?

Like in xpcom/components, i am not convinced that we need reader/write locks
here.  We are paying the cost of two normal locks when we don't even know if
there is a performance delta.

I think you might want to cast this:

+  PRBool IsInitialized() const { return mLock; }

is NS_WARN_ enough?  
+  mLock = PR_NewRWLock(PR_RWLOCK_RANK_NONE, "nsBaseHashtableL");
+  NS_WARN_IF_FALSE(mLock, "Error creating lock during
nsBaseHashtableL::Init()");

Comment 5

[Benjamin Smedberg]

> remind me again why these class destructors's aren't virtual?

for performance reasons, mainly... virtual destructors cannot be inlined, and we
know that these destructors are not going to be called virtually, because these
classes are not accessed through base-class pointers.

> Like in xpcom/components, i am not convinced that we need reader/write locks

If the hashtables aren't going to be accessed concurrently most of the time,
PRLock is fine, but if there is any significant chance of concurrency, PRRWLock
is much better, without a whole lot of runtime overhead. cc'ing wtc -- there are
probably stats on how they compare.

is NS_WARN_ enough?  
+  mLock = PR_NewRWLock(PR_RWLOCK_RANK_NONE, "nsBaseHashtableL");
+  NS_WARN_IF_FALSE(mLock, "Error creating lock during
nsBaseHashtableL::Init()");

The Init() function returns false if the lock is not created.

Comment 6

[Wan-Teh Chang]

You need to do experiments to find out whether PRLock
or PRRWLock is better for your hash tables.

Comment 7

[Benjamin Smedberg]

dougt/alecf: here are the current consumers of thread-safe nsHashtables. Do you
know offhand whether any of these are subject to significant lock contention?

nsDirectoryService::nsDirectoryService
  (nsSupportsHashtable)
nsNativeComponentLoader::nsNativeComponentLoader
  (nsHashtable,nsObjectHashtable)
nsObserverService::GetObserverList
  (nsObjectHashtable)
nsExceptionService::nsExceptionService
  (nsSupportsHashtable)
nsStringBundleService::nsStringBundleService
  (nsHashtable)
nsProxyObjectManager::nsProxyObjectManager
  (nsHashtable,twice)

Comment 8

[Doug Turner (:dougt)]

I could guess, but I think that it would be quicker just to get the data.  wtc,
is the PR_Lock currently instrumented for this already?

Comment 9

[Benjamin Smedberg]

I am going to instrument these nsHashtables using PRP_TryLock to see what I can
see...

Comment 10

[Benjamin Smedberg]

Attachment: nsBaseHashtableThread

* Instrumentation showed no significant lock contention, so I'm using PRLock;
* Renamed classes to use a "Thread" suffix instead of "L";
* Removed unneeded empty constructor/destructors... in some cases, default
  constructors are more efficient
* fixed casting nit

dougt/alecf: do you want to review again? I would like to land this and bug
193031 together RSN, if that's acceptable.

Comment 11

[Wan-Teh Chang]

"MT" is a common abbreviation for "multithreaded".
If you want to have a shorter class name, you can
use "MT" instead of "Thread".

It is a good idea to use PRLock unless there is proof
that PRRWLock performs better.

Comment 12

[Brendan Eich [:brendan]]

Thread is not an adjective -- either MT or ThreadSafe would be better.

Shorter is better, even in this InterCaps Mozilla world.

/be

Comment 13

[Benjamin Smedberg]

Attachment: nsBaseHashtableMT

Fixed the name (again) to use the MT suffix.

Comment 14

[Benjamin Smedberg]

Comment on attachment 125835 [details] [diff] [review]
nsBaseHashtableMT

Got permission from alecf to carry over sr. dougt, I would love to get this in
soon ;)

Comment 15

[Doug Turner (:dougt)]

Comment on attachment 125835 [details] [diff] [review]
nsBaseHashtableMT

r=dougt

Comment 16

[Benjamin Smedberg]

Fixed on trunk.

Comment 17

[Benjamin Smedberg]

heh