separate threadsafe nsBaseHashtable into superclass

RESOLVED FIXED in mozilla1.5alpha

Status

()

RESOLVED FIXED
16 years ago
5 years ago

People

(Reporter: benjamin, Assigned: benjamin)

Tracking

Trunk
mozilla1.5alpha
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

16 years ago
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.
(Assignee)

Comment 1

16 years ago
Created attachment 125011 [details] [diff] [review]
nsBaseHashtableL super-class

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

Updated

16 years ago
Attachment #125011 - Flags: review?(alecf)
(Assignee)

Updated

16 years ago
Blocks: 193031

Comment 2

16 years ago
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

16 years ago
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
Attachment #125011 - Flags: superreview+
Attachment #125011 - Flags: review?(dougt)
Attachment #125011 - Flags: review?(alecf)

Comment 4

16 years ago
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()");
Attachment #125011 - Flags: review?(dougt)
(Assignee)

Comment 5

16 years ago
> 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

16 years ago
You need to do experiments to find out whether PRLock
or PRRWLock is better for your hash tables.
(Assignee)

Comment 7

16 years ago
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

16 years ago
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?
(Assignee)

Comment 9

16 years ago
I am going to instrument these nsHashtables using PRP_TryLock to see what I can
see...
(Assignee)

Comment 10

16 years ago
Created attachment 125518 [details] [diff] [review]
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.
Attachment #125011 - Attachment is obsolete: true

Comment 11

16 years ago
"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.
Thread is not an adjective -- either MT or ThreadSafe would be better.

Shorter is better, even in this InterCaps Mozilla world.

/be
(Assignee)

Comment 13

16 years ago
Created attachment 125835 [details] [diff] [review]
nsBaseHashtableMT

Fixed the name (again) to use the MT suffix.
Attachment #125518 - Attachment is obsolete: true
(Assignee)

Comment 14

16 years ago
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 ;)
Attachment #125835 - Flags: superreview+
Attachment #125835 - Flags: review?(dougt)

Comment 15

16 years ago
Comment on attachment 125835 [details] [diff] [review]
nsBaseHashtableMT

r=dougt
Attachment #125835 - Flags: review?(dougt) → review+
(Assignee)

Comment 16

16 years ago
Fixed on trunk.
Target Milestone: --- → mozilla1.5alpha
(Assignee)

Comment 17

16 years ago
heh
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.