1255070 - implement nsIBufferedInputStream and nsIBufferedOutputStream on more stream classes that support ReadSegments and WriteSegments

Status: NEW

(Core :: XPCOM, defect, P3)

Description

[Ben Kelly [:bkelly, not reviewing]]

Currently methods like NS_InputStreamIsBuffered() and NS_OutputStreamIsBuffered() do something like this:

1) Check if the stream implements the appropriate nsIBuffered* interface type.
2) If not, call ReadSegments() or WriteSegments() with a dummy callback.

Currently the only stream types that implement the nsIBuffered* interfaces are nsBufferedInputStream and nsBufferedOutputStream.  This means we hit case (2) a lot.

The fallback in (2) is a problem for a couple reasons:

a) It can cause I/O
b) It is unreliable.  For example, an async stream might return WOULD_BLOCK which results in the stream being marked as non-buffered.

It would be much better all around if we simply implemented nsIBufferedInputStream and nsIBufferedOutputStream on my concrete stream classes.

I have a patch to do this for nsPipeInputStream in bug 1093357, but I think we should do it to others as well.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P0 Explicitly advertise that nsPipeInputStream supports ReadSegments(). r=froydnj

This patch has r+, but needs some review comments addressed from bug 1093357 comment 64.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Make nsPipeInputStream implement nsIBufferedInputStream. r=froydnj

Addressed review feedback.  Updated commit message to reflect new bug.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Make nsPipeOutputStream implement nsIBufferedOuptutStream. r=froydnj

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Make nsStringInputStream implement nsIBufferedInputStream. r=froydnj

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Make nsPipeOutputStream implement nsIBufferedOuptutStream. r=froydnj

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Lets just see how these few changes do on try:

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=bd3866e72dd7

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Make SnappyUncompressInputStream implement nsIBufferedInputStream. r=froydnj

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Make nsStringInputStream implement nsIBufferedInputStream. r=froydnj

Fix classinfo on string streams.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ee7b703ef2e

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Make SnappyCompressOutputStream implement nsIBufferedOutputStream. r=froydnj

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P6 Make dom/cache ReadStream implement nsIBufferedInputStream. r=froydnj

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P7 Make nsStorageInputStream implement nsIBufferedInputStream. r=froydnj

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P8 Make nsStorageStream implement nsIBufferedOutputStream. r=froydnj

This is probably as far as I will go for now.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fe642fcccfd8

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8728590 [details] [diff] [review]
P1 Make nsPipeInputStream implement nsIBufferedInputStream. r=froydnj

I landed this in bug 1093357 after all.

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

I'm surprised this is such a priority for e10s.  Do you have data that shows this is a bad problem in practice.  My work here has been a bit speculative.

Incidentally I think we should take this bug in a slightly different direction.  Instead of directly implementing nsIBufferedInputStream I think we should create a new interface type.  This new type would provide a better that could be used to test buffered status at runtime in order to support aggregate stream types.  nsIBufferedInputStream and these other stream types would implement this new interface.

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

Andrea, this might be of interest to you.