The default bug view has changed. See this FAQ.

service manager has too many locks

RESOLVED WORKSFORME

Status

()

Core
XPCOM
P2
major
RESOLVED WORKSFORME
18 years ago
2 months ago

People

(Reporter: Chris Waterson, Assigned: neeti)

Tracking

({perf})

Trunk
mozilla0.9.9
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [PDT-])

(Reporter)

Description

18 years ago
The service manager has APIs that are protected by a monitor, as well as using
a threadsafe hashtable for internal use. Travis said "service manager is
dying", so I'm handing him this bundle o' joy to remind him that the new
incarnation shouldn't do that.

Updated

18 years ago
Assignee: travis → dp

Comment 1

18 years ago
This probably belongs to you dp as you are the one working on that side of the
service manager.

Updated

18 years ago
Severity: normal → major
Status: NEW → ASSIGNED
Priority: P3 → P2
Target Milestone: M11

Comment 2

18 years ago
It might take a while before the servicemanager goes away. In the mean time, any
suggestions for improvement here.

Updated

18 years ago
QA Contact: beppe → gerardok

Updated

18 years ago
Summary: [perf] service manager has too many locks → [DOGFOOD] [perf] service manager has too many locks

Comment 3

18 years ago
This is a performance bottleneck. We should see if we can eliminate the
excessive locking.

Comment 4

18 years ago
If the service manager entry points are protected by a lock, the hashtable
doesn't need to use one. I see line 180 of nsServiceManager.cpp says "Get a
threadSafe hashtable" which may not be necessary any more. I'd investigate
setting that parameter to false.

Comment 5

18 years ago
Just curious, why are you eliminating the other case: remove locks from apis and
protect the data (hashtable entries).

Updated

18 years ago
Whiteboard: [PDT-]

Comment 6

18 years ago
performance team should nominate this to make it happen.

Comment 7

18 years ago
Correction, performance team should nominate this IF they want to make it
happen.

Comment 8

18 years ago
Performance guys, can you evaluate this and let us know if we should fix it.

Updated

18 years ago
Blocks: 17432

Updated

18 years ago
Target Milestone: M11 → M13

Updated

17 years ago
Target Milestone: M13 → M16

Comment 9

17 years ago
whatever. We are going to leave it like this for a while.

Updated

17 years ago
Keywords: perf
Summary: [DOGFOOD] [perf] service manager has too many locks → [DOGFOOD]service manager has too many locks

Comment 10

17 years ago
Moving "perf" to keyword field.  This is the are we use now :-)

Comment 11

17 years ago
moving from leftover dogfood to beta1 radar per beta criteria priority #2
Keywords: beta1
Summary: [DOGFOOD]service manager has too many locks → service manager has too many locks
Whiteboard: [PDT-]

Comment 12

17 years ago
Need to quanify what impact this has on the spec and where you think it will 
help and how much please.
Where is the spec?

Comment 14

17 years ago
This bug came out of the timing analysis done by the performance effort. I am 
sure this was a blaring number and hence the bug. We need to quantify to really 
get the number by which this is a problem and further by how much we can reduce 
it since there is no way we can eliminate all the locks.

I know this doesn't help much. Just thinking aloud at this point. My general 
sense was that this wont improve performance a great deal. So let us go after 
bigger fish...

Comment 15

17 years ago
Putting on PDT- radar for beta1.  If you have date to make a better call, let us 
know - pdt
Whiteboard: [PDT-]

Comment 16

17 years ago
A recent Quantify run shows that GetService accounts for 15.18% of total 
runtime. The enter/exit monitors there account for about 2% of that.

The service manager also uses a threadsafe hashtable, which accounts for 
another 1%. That hashtable is threadsafe (unnecessarily?), and 44% of all time 
spent in nsHashtable::Get is entering and exiting the lock. 

Attempting to calculate this all out, I come up with (.02 + .44 * .01) * .15 
==> .366% of execution time manipulating these locks. However, the simple fix 
is probably to change the flag passed to the hashtable constructor to make it 
not be threadsafe since the higher level locks are already protecting it.
Whiteboard: [PDT-]

Comment 17

17 years ago
Based on Warren's analysis, there is a tiny .366% gain (even if we got it down 
to zero locks) and notable risk.  PDT gave this a PDT- for beta1.
Whiteboard: [PDT-]

Updated

17 years ago
Target Milestone: M16 → Future

Comment 18

17 years ago
Moving all current open XPCOM and XPCOM Registry bugs to rayw since dp is on 
sabbatical.  rayw is now default assignee for these components.
Assignee: dp → rayw
Status: ASSIGNED → NEW

Updated

17 years ago
Status: NEW → ASSIGNED

Updated

17 years ago
No longer blocks: 17432

Comment 19

17 years ago
Assigning rest of xpcom stuff to myself (from Ray).
Assignee: rayw → warren
Status: ASSIGNED → NEW
Target Milestone: Future → mozilla1.0

Comment 20

16 years ago
-> kandrot 

now is a good time for smaller and larger perf wins...
Assignee: warren → kandrot

Updated

16 years ago
Status: NEW → ASSIGNED

Comment 21

16 years ago
reassign all kandrot xpcom bug.
Assignee: kandrot → dougt
Status: ASSIGNED → NEW
Target Milestone: mozilla1.0 → ---

Updated

16 years ago
Blocks: 98275

Comment 22

16 years ago
neeti is working on this as part of her cleanup/performance work in xpcom.
Assignee: dougt → neeti

Updated

16 years ago
Blocks: 104166

Updated

16 years ago
Target Milestone: --- → mozilla0.9.7

Comment 23

16 years ago
Add myself to the CC list. I'll try to see if I can do some work on it.
(Assignee)

Updated

16 years ago
Target Milestone: mozilla0.9.7 → mozilla0.9.8
(Assignee)

Updated

15 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

15 years ago
Target Milestone: mozilla0.9.8 → mozilla0.9.9
(Assignee)

Comment 24

15 years ago
This was fixed by bug 96461. nsComponentManager now uses PLDHashtable, instead 
of the threadsafe version of nsHashtable.
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.