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

Status: RESOLVED WONTFIX

(Core :: XPCOM, defect, P3)

Comment 1

[Doug Turner (:dougt)]

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.

Comment 2

[Dan M]

(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?

Comment 3

[rpotts (gone)]

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

Comment 4

[Doug Turner (:dougt)]

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.

Comment 5

[Judson Valeski]

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...

Comment 6

[Doug Turner (:dougt)]

getting some data on window 98.

Comment 7

[rpotts (gone)]

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

Comment 8

[leger]

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.

Comment 9

[Doug Turner (:dougt)]

This is a big win.  We save 3.14 ms *per* proxy function call on my p3/500.

Comment 10

[Judson Valeski]

cha-ching! what platform? NT?

Comment 11

[Doug Turner (:dougt)]

yeah, nt.  98/95 has the same problem and should have a speed improvement, but I 
do not have data on that.

Comment 12

[Doug Turner (:dougt)]

Attachment: #ifdefing nested eventQ

Comment 13

[Doug Turner (:dougt)]

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.

Comment 14

[Doug Turner (:dougt)]

I sent out diff.  I also sent a build to leger to get some performance data on 
it.  

Comment 15

[leger]

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?

Comment 16

[Doug Turner (:dougt)]

QA should focus on IMAP, FTP, and cookies.

Fix is checked in.

Comment 17

[leger]

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.

Comment 18

[Tom Everingham]

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.

Comment 19

[lchiang]

We've been using IMAP since 3/1 and seems fine.  mscott - ok to mark verified?

Comment 20

[Scott MacGregor]

yup. We are good to go on this one.

Comment 21

[Alex Fritze]

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

Comment 22

[Alex Fritze]

Bug 16773 now contains a patch that (optionally) reenables nested event queues for proxies.

Comment 23

[Alex Fritze]

Obsoleted by Darin's thread code rewrite earlier this year.