12346 - nsOutputFileStream should buffer the output

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect, P3)

Description

[Dan Mosedale (:dmosedale, :dmose)]

Attaching with strace(1) reveals that writes are not being buffered in any
useful way.  Example output from strace:

write(20, "=", 1)                       = 1
write(20, "\"", 1)                      = 1
write(20, "http://bugzilla.mozilla.org/show"..., 48) = 48
write(20, "\"", 1)                      = 1
write(20, ">", 1)                       = 1
write(20, "3256", 4)                    = 4
write(20, "<", 1)                       = 1
write(20, "/", 1)                       = 1
write(20, "a", 1)                       = 1
write(20, ">", 1)                       = 1
write(20, ": Add ScrollIntoView() method to"..., 52) = 52
write(20, "<", 1)                       = 1
write(20, "/", 1)                       = 1
write(20, "td", 2)                      = 2
write(20, ">", 1)                       = 1
write(20, "<", 1)                       = 1
write(20, "/", 1)                       = 1
write(20, "tr", 2)                      = 2
write(20, ">", 1)                       = 1

Comment 1

[Simon Fraser [no longer active]]

The save code passes an nsIOutputStream to the content sink, and the
nsHTMLContentSinkStream calls Write() with lots of small chunks on this output
stream. The stream does not buffering, so perhaps it should?

Alternatively, the nsHTMLContentSinkStream could buffer each line of outout.

Comment 2

[Akkana Peck]

I'll make the output sink buffer its output in the stream case (it already
buffers the whole file in the nsString case).

Comment 3

[Akkana Peck]

Line buffering turns out not to be straightforward, since
nsHTMLContentSinkStream::Write can take an nsString, a char* or a char.  So I'll
probably just write when it reaches some number of characters.

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

That seems better anyway, since line buffering would still often end up with one
write() per < 80 chars, which still seems like too many.

Comment 5

[Simon Fraser [no longer active]]

The alternative (which is the Right Thing, IMO) is to make nsOutputFileStream
do buffering. Then everyone gets buffering. I don't know who owns this now.

Comment 6

[Akkana Peck]

Turns out that would really be much better.  Buffering in the HTML content sink
stream turns out to be tricky because of the way it does different unicode
conversions in the three different Write() methods.

Comment 7

[Simon Fraser [no longer active]]

Adjust summary, reassign.

Comment 8

[Doug Turner (:dougt)]

not happening for m10, moving to m11.

Comment 9

[Doug Turner (:dougt)]

looking at this now.

Comment 10

[Doug Turner (:dougt)]

just checked in.  Buffer is 4k.

Comment 11

[sujay]

I need help verifying this bug and marking it VERIFIED-FIXED...any takers?

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

Should be pretty easy; on linux:

1) go to a large web page
2) edit the page
3) attach to the apprunner process with strace(1), using -o to save the output
   to a file
4) save the page
5) look at the strace output and make sure that the calls to write() are using
   large (4K) buffers rather than 1 and 2 byte buffers as shown above

Comment 13

[sujay]

verified in 9/9 build.