164278 - reading cookie file seems very inefficient

Status: RESOLVED FIXED

(Core :: Networking: Cookies, defect, P2)

Description

[Kathleen :Brade]

While starting up Chimera, the cookie code made 183 read calls to the system
(out of 828 total read calls).  The cookies file appears to be read in 128 byte
blocks rather than a more optimal size like 4k or 8k.  Increasing the buffer
size would reduce the number of system calls made and increase performance.

Also, I'm somewhat surprised that the buffer is not passed around but instead is
a static.  Wouldn't it be clearer to pass around the buffer?

Comment 1

[Stephen P. Morse]

Is there a reason that the component is set to Chimera rather than browser? 
Also should the OS be All instead of MacOSX?

Comment 2

[Kathleen :Brade]

changing product/component and platform/OS

(I was busy filing lots of performance and memory leak issues and just kept
reusing the same page without resetting everything.)

Comment 3

[Jo Hermans]

All that is required is to change the definition of bufsize at
http://lxr.mozilla.org/seamonkey/source/extensions/cookie/nsUtils.cpp#50 ?

Comment 4

[Kathleen :Brade]

Actually, I would think that there is a better class out there to use to read
the file into a buffer.  At a minimum, increasing the buffer size would be ok. 
However, I really don't see any reason for the cookie code to keep around a
static buffer (no matter what size it is).

Comment 5

[Kathleen :Brade]

Attachment: proof of concept / first attempt

here is a proof of concept or a first attempt (whichever way you want to think
about it).  This is more inline with what I was thinking.  I was actually
hoping to use the bufferedInputStream class but that is in necko and I'm not
sure that can be used in this module.

Comment 6

[Stephen P. Morse]

Kathy,

Thanks.  I was about to start on this bug today, so your "proof of concept" is
very timely.  I look it over right now and either use it as is or modify it if
necessary.

Comment 7

[Stephen P. Morse]

Kathy's proof of concept is much more than that -- it is a final patch.  I'm for
checking it in as is.

r = morse

Comment 8

[Kathleen :Brade]

Attachment: this is the patch to apply to the trunk (essentially same except string changes made to api on trunk)

previous patch is for Chimera branch; this patch is for trunk

Comment 9

[Kathleen :Brade]

cc heikki for sr= on both patches
-->brade

Comment 10

[Stephen P. Morse]

Comment on attachment 97071 [details] [diff] [review]
this is the patch to apply to the trunk (essentially same except string changes made to api on trunk)

r=morse for trunk patch

Comment 11

[Darin Fisher]

Comment on attachment 96898 [details] [diff] [review]
proof of concept / first attempt

>Index: nsUtils.cpp

>+//  static PRInt32 next = bufsize, count = bufsize;

nit: remove this comment.  sr=darin

Comment 12

[Darin Fisher]

Comment on attachment 97071 [details] [diff] [review]
this is the patch to apply to the trunk (essentially same except string changes made to api on trunk)

sr=darin

Comment 13

[Stephen P. Morse]

Patch checked in on trunk.  Kathy, could you please do the checkin for the
Chimera branch.  Don't forget to remove the comment that darin noted.  Thanks.