Efficiency issues with nsObserverTopic, etc

RESOLVED FIXED in Future

Status

()

Core
HTML: Parser
P4
normal
RESOLVED FIXED
16 years ago
3 years ago

People

(Reporter: jesup, Unassigned)

Tracking

({perf})

Trunk
Future
x86
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

16 years ago
Mozilla 20010717xx

nsObserverTopic has a few efficiency issues:

1. GetTopic() is a linear search of topics.  Perhaps it should be a hashtable of
some sort.

2. Notify() takes all the keys and values (which are strings), and then adds
them to nsVoidArrays along with a few extra items.  This causes quite a few
allocations (both of strings and nsVoidArray structures), and the Observers
shouldn't be modifying the keys/values anyways.  Perhaps these should be passed
by reference.  (The strings may not duplicate all the storage due to how our
string classes work; here's the code:
  nsString* string = new nsString(aString);
  if (nsVoidArray::InsertElementAt(string, aIndex))
    ...
(Reporter)

Updated

16 years ago
Keywords: perf
(Reporter)

Updated

16 years ago
Blocks: 71668
(Reporter)

Comment 1

16 years ago
I dealt with the allocations of VoidArray in bug 90545.  The issues of
allocations of strings still applies.

Updated

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

Updated

16 years ago
Status: NEW → ASSIGNED
Priority: -- → P2

Updated

16 years ago
QA Contact: bsharma → moied

Comment 2

16 years ago
--> 0.9.6
Target Milestone: mozilla0.9.5 → mozilla0.9.6

Comment 3

16 years ago
With fix for bug 96364 GetTopic() ( rather GetEntry() )should not be an issue
since the number of calls to GetEntry() has reduced a LOT.

Comment 4

16 years ago
Lowering the priority based on my previous comment.
Priority: P2 → P4

Comment 5

16 years ago
Out of time :-( Moving to 0.9.7
Target Milestone: mozilla0.9.6 → mozilla0.9.7

Comment 6

16 years ago
Randell: Is this still an issue?
Target Milestone: mozilla0.9.7 → mozilla0.9.8
(Reporter)

Comment 7

16 years ago
Yes, I believe so.  Check dbaron's pageload jprof to get an idea if it's important.

Comment 8

16 years ago
Out of time. Mass move to 0.9.9
Target Milestone: mozilla0.9.8 → mozilla0.9.9

Comment 9

16 years ago
Mass moving to 1.1.
Target Milestone: mozilla0.9.9 → mozilla1.1
[was 1.1alpha]
Target Milestone: mozilla1.1alpha → Future
Assignee: harishd → nobody
Status: ASSIGNED → NEW
QA Contact: moied → parser
(Reporter)

Comment 11

6 years ago
Case 1 is now a hashtable as suggested, and case 2 no longer does the code snippet shown; it inserts fairly directly into an nsCOMArray in nsObserverList, so this is fixed.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.