Closed Bug 29474 Opened 20 years ago Closed 14 years ago

Investigate remove nested eventQs from xpcom/proxy to improve preformance.

Categories

(Core :: XPCOM, defect, P3, major)

x86
Other
defect

Tracking

()

RESOLVED WONTFIX

People

(Reporter: dougt, Unassigned)

References

Details

(Keywords: perf, Whiteboard: [PDT+] w/b minus on 03/03)

Attachments

(1 file)

No description provided.
nested eventQs create/destroy an invisible window for *every* call.  We may be
able to remove this sucessfully.  My concern is that we would be processing
events on the Q which might not be ours.  Need to think a bit about this.
Severity: normal → major
Status: NEW → ASSIGNED
Keywords: beta1
Whiteboard: pref
(spawn of bug 25979, right?) The reason we make a new window for each queue is 
historical: NSPR was designed with one thread, one queue in mind. We added 
stacked queues and brought along some extra weight. I think Rick is dead on in 
25979: all the queues for a thread should be able to share the same window.

However, I'm not sure it's all that important. Do we need to spend a lot of time 
doing this? I remember once noticing a situation where a proxy repeatedly created 
and destroyed queues in a loop. (I seem to remember that it was the main queue 
for the thread. If that's true, you're stuck with the window creation, of 
course.) Regardless, can you teach the proxy to recycle?
What I'm seeing is that each time a method is called on a proxy object 
PushThreadEventQueue/PopThreadEventQueue are called.  This means that in the 
current world, we create/destroy a window *each* time we call a proxy method.

Of course it all depends on how much we use proxy objects.  Right now I believe 
that FTP and maybe IMAP are the major consumers of proxy objects.  In order to 
analyze the performance impact of not fixing this, you should look at the number 
of proxy objects calls made when FTPing a file, or doing IMAP stuff...

For normal browsing, *no* proxy objects are used (yet)

-- rick
I am not seeing a big preformance hit at all.  On a run of two or two thosand, 
the delta between nested q's and not is in the few milliseconds.  I am going to 
mark this as wont fix.
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → WONTFIX
doug, I think this could have some platform specific ramifications. Can you 
ellaborate a little more on you testing/verifying? This bug might explain why nt 
hauls ass for FTP directory navigation, but win95 doesn't...
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
getting some data on window 98.
hey doug,

I guess the fact that we are calling Push/PopThreadEventQueue() for each method 
call is not in itself a problem :-)

I think that the *real* problem is that PushThreadEventQueue() is creating the 
wrong kind of event queue for worker threads.  And that for UI threads, a new 
window is being created rather than sharing a single (per thread) window.

If these issues are resolved in bug #25979, then doing a 
Push/PopThreadEventQueue(...) may not matter too much.

-- rick
Putting on PDT- radar for beta1.  If you get concrete data and fix to enhance 
perf by a large percentage, please remove PDT- and resubmit.
Keywords: perf
Whiteboard: pref → [PDT-]
This is a big win.  We save 3.14 ms *per* proxy function call on my p3/500.
Whiteboard: [PDT-]
cha-ching! what platform? NT?
yeah, nt.  98/95 has the same problem and should have a speed improvement, but I 
do not have data on that.
when the diff is applied, simply remove the NESTED_Q define, and nested eventQs 
will be disabled.  I have tested IMAP and FTP, and both seam to work fine.
I sent out diff.  I also sent a build to leger to get some performance data on 
it.  
Putting on PDT+ radar for beta1.  But will make PDT- and release note if not 
fixed by 03/03.

This needs LOTS of testing once fix is in!!!  What do you want QA to do to test?
Whiteboard: [PDT+] w/b minus on 03/03
QA should focus on IMAP, FTP, and cookies.

Fix is checked in.
Status: REOPENED → RESOLVED
Closed: 20 years ago20 years ago
Resolution: --- → FIXED
Assigning to tever.  he works in this area, and if cookies and ftp have a good 
day with 03/02 and 03/03 builds, I would call this verified.
QA Contact: leger → tever
I cannot see any problems with FTP or cookies.  Giving to Lisa to check out 
IMAP.  mscott would be a possible contact if you need help testing.
QA Contact: tever → lchiang
We've been using IMAP since 3/1 and seems fine.  mscott - ok to mark verified?
yup. We are good to go on this one.
Status: RESOLVED → VERIFIED
I need to resurrect nested queues for the zap code [1], maybe as an option by
adding a PROXY_NESTED_QUEUES flag to nsIProxyObjectManager.idl

I have 2 threads talking to the UI thread via proxies: a media pipeline thread
and the socket thread. Without nested queues, events from these two threads are
not sequenced correctly onto the UI thread. Occasionally while I'm synchronously
calling a function on the media thread and waiting in
nsProxyObject::PostAndWait(), the UI thread gets reentrantly called from the
socket thread.

Potentially I might need an even more elaborate scheme where eventQs are tagged
with the thread that they are accepting events from. When a call through a proxy
is made, the proxy would check if there is a younger queue accepting events from
the caller's thread.

[1] http://www.croczilla.com/zap
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Blocks: 317491
Bug 16773 now contains a patch that (optionally) reenables nested event queues for proxies.
Depends on: 16773
Assignee: dougt → nobody
Status: REOPENED → NEW
QA Contact: lchiang → xpcom
Obsoleted by Darin's thread code rewrite earlier this year.
Status: NEW → RESOLVED
Closed: 20 years ago14 years ago
Resolution: --- → FIXED
Resolution: FIXED → WONTFIX
You need to log in before you can comment on or make changes to this bug.