13506 - fix news connection cache

Status: VERIFIED FIXED

(MailNews Core :: Networking, defect, P3)

Description

[Scott MacGregor]

In order to get good performance for news we are going to eventually need a
connection cache. I've been working a lot on optimizing message display time and
a connection cache for news would really help.

Filing this bug to keep track of that. I may do it today or tomorrow. =)

Comment 1

[leger]

Putting on [Perf] radar.

Comment 2

[Phil Peterson]

Triage to M15

Comment 3

[leger]

Bulk add of "perf" to new keyword field.  This will replace the [PERF] we were
using in the Status Summary field.

Comment 4

[Scott MacGregor]

*** Bug 31537 has been marked as a duplicate of this bug. ***

Comment 5

[selmer (gone)]

If we lose something from mscott's list for beta2, this is it.  Marking M16.

Comment 6

[David :Bienvenu]

Scott has graciously agreed to let me bang my head against this one for a few 
days.

Comment 7

[David :Bienvenu]

accepting

Comment 8

[(not reading, please use [e-mail] instead)]

I'd like to get this fixed before we rtm.

from http://www.newsreaders.com/misc/twpierce/news/newsreader-manifesto.html

"Don't disconnect between every command.  I hate to embarrass anyone but the
authors of Netscape made the mistake in a beta version (the current one is
fixed) where they closed the connection after every single article. You could
just hear your system performance die as your kernel locks out everything trying
to fork() fast enough to keep up with the Netscape users."

the author is talking about an old version of netscape.

while this will help performance, it more of the "don't pound on nntp servers"
feature aspect that is nsbeta2.

bienvenu has started the work, but I know he is busy with search.
david, is this something I can finish?  how much is left?

Comment 9

[David :Bienvenu]

The code is all written - it basically needs to be debugged - in 
nsNNTPProtocol.cpp, there's a #define, USE_CONN_CACHE, which is NOT turned on. 
If you turn it on, you can see the problem, which is basically that when we 
use a cached connection, the url's keep getting interrupted so the connection 
gets dropped, rendering the cache not so useful. If you want to have a crack at 
debugging this, that would be great.

Comment 10

[leger]

Putting on [nsbeta2-] radar. Not critical to beta2. Performance is for beta3 
work.  Adding nsbeta3 keyword.

Comment 11

[(not reading, please use [e-mail] instead)]

I can wait until nsbeta2, since we've had this problem all along.

but we really got to fix this, since it causes us to put a lot of stress on nntp
servers.

taking back from david b.  (thanks for doing the hard part)

Comment 12

[Mike]

M16 has been out for a while now, these bugs target milestones need to be 
updated.

Comment 13

[(not reading, please use [e-mail] instead)]

adding david b back to the cc list, as he may take this bug back.

Comment 14

[David :Bienvenu]

Yes, I'll take it back, but I think I'll do it for beta3, since it's really a
big performance issue.

Comment 15

[David :Bienvenu]

accepting for nsbeta3

Comment 16

[David :Bienvenu]

changing summary - the feature is in, it just has a bug or two.

Comment 17

[David :Bienvenu]

fix checked in.

Comment 18

[[e-mail] (gone - use [e-mail] instead)]

Is this anything QA can verify with a log?

Comment 19

[David :Bienvenu]

yes, you can generate a protocol log and make sure we're not opening up a
connection to the server for every news message read.

Comment 20

[[e-mail] (gone - use [e-mail] instead)]

is it mode reader that I should check to make sure we're not sending on reading
a message in the same server?

Comment 21

[[e-mail] (gone - use [e-mail] instead)]

Attachment: Log of our commands on reading 3 messages from the same server

Comment 22

[David :Bienvenu]

yes, mode reader, or the connection response, is what you're looking for. This
looks OK, except that we shouldn't need to issue the group command for the group
multiple times - I wonder if 4.x did that? It should be fast enough on the
server if we've already grouped into the group, but we shouldn't send the
protocol at all.

Comment 23

[David :Bienvenu]

can I get reviews on the fix I'm about to attach? This fix will make it so that
we don't issue a group on every message download, and should speed up news a
bit. I moved the initialization of m_currentGroup to the constructor, instead of
Initialize, which gets called on every news url.

Comment 24

[David :Bienvenu]

Attachment: proposed speedup fix

Comment 25

[(not reading, please use [e-mail] instead)]

r=sspitzer

thanks for finding and fixing this.

Comment 26

[[e-mail] (gone - use [e-mail] instead)]

Attachment: Looks like we're not sending GROUP, looks fine.

Comment 27

[[e-mail] (gone - use [e-mail] instead)]

David, you agree?  If so, I'll leave verified, and thanks!

Comment 28

[David :Bienvenu]

yes, that looks much better, just mark it verified, thanks!

Comment 29

[[e-mail] (gone - use [e-mail] instead)]

David B - you rock!