191416 - clean up nsIUnicharInputStream

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P2)

Description

[Alec Flett]

nsIUnicharInputStream needs some small cleanup so that I can implement
ReadSegments() to reduce the number of copies done during a read. This had some
reprocussions throughout the tree, so the patch tends to be a bit big..  

In the process I also cleaned up nsPersistentProperties to remove some dead
code, and fixed nsIUnicharInputStream::Read() so its parameters matched
nsIInputStream.

patch forthcoming.

Comment 1

[Alec Flett]

ok, I don't know what is going on here, but I can't seem to attach patches to
this bug. I can attach them to other bugs no problem. Maybe its something screwy
on my end. Anyway, the patch will eventually arrive here.

I should add that this will reduce the number of copies that
nsPersistentProperties and the CSS parser use when parsing buffers.

Comment 2

[Alec Flett]

Attachment: cleanup

here's what I fixed:
- removed the extra offset from Read() which was never used
- removed Fill() which was never used
- added ReadSegments() to avoid extra copies (I still have to fix consumers of
course..)

Comment 3

[Alec Flett]

Comment on attachment 115389 [details] [diff] [review]
cleanup

darin/dougt - another review for you. Please see my previous comment for the 3
things I cleaned up in this patch. Thanks!

Comment 4

[Darin Fisher]

Comment on attachment 115389 [details] [diff] [review]
cleanup

ok, some comments based on nsIInputStream::ReadSegments.  i'm assuming we
want this ReadSegments to behave like the nsIInputStream::ReadSegments.
even if not completely necessary to implement the same semantics, it is 
probably a good idea because it is more consistent and future-proof.


>+NS_IMETHODIMP
>+StringUnicharInputStream::ReadSegments(nsWriteUnicharSegmentFun aWriter,
>+                                       void* aClosure,
>+                                       PRUint32 aCount)

ReadSegments should return the number of bytes read from the stream.  this
is important if |aWriter| chooses to accept only part of the data in the
stream, or if the stream has fewer than |aCount| bytes available.


>+{
>+  PRUint32 bytesWritten;
>+  PRUint32 totalBytesWritten = 0;
>+
>+  nsresult rv;
>+  do {
>+    rv = aWriter(this, aClosure, mString->get() + mPos,
>+                 totalBytesWritten, aCount, &bytesWritten);
>+    aCount -= bytesWritten;
>+    totalBytesWritten += bytesWritten;
>+    mPos += bytesWritten;
>+    
>+  } while (NS_SUCCEEDED(rv) && aCount > 0);
>+
>+  return rv;
>+}

Suppose |mString->Length() - mPos < aCount|??  use PR_MIN before calling
writer :)

ReadSegments should not propogate errors returned from |aWriter|.  use
a failure return value to mean "stop iterating".  since you're calling
the writer in a loop, it could choose to only read part of the data.
we should allow for partial reads by allowing the writer to return a
failure code to stop the iteration.

same comments apply to the other ReadSegments impls.

Comment 5

[Alec Flett]

Comment on attachment 115389 [details] [diff] [review]
cleanup

cool, thanks for the info. I whipped up those readSegments implementations but
wasn't exactly surehow to go...clearing dougt's request, new patch coming up

Comment 6

[Alec Flett]

Attachment: cleanup v1.1

ok, I was just being lazy, not returning the read count... 

Here's the updated cleanup patch with working versions of ReadSegments(), with
proper [lack of] error propagation and so forth.

Comment 7

[Alec Flett]

Comment on attachment 116333 [details] [diff] [review]
cleanup v1.1

Darin/Doug - once more can I get reviews on this cleanup? thanks!

Comment 8

[Darin Fisher]

Comment on attachment 116333 [details] [diff] [review]
cleanup v1.1

>Index: xpcom/io/nsUnicharInputStream.cpp

>+StringUnicharInputStream::ReadSegments(nsWriteUnicharSegmentFun aWriter,
...
>+  nsresult rv = NS_OK;
...
>+    rv = aWriter(this, aClosure, mString->get() + mPos,
>+                 totalBytesWritten, aCount, &bytesWritten);
>+    if (NS_FAILED(rv)) {
>+      // don't propagate errors to the caller
>+      rv = NS_OK;
>+      break;
...
>+  return rv;
>+}

looks like you can just always return NS_OK here, then you don't
need to overwrite rv when aWriter returns a failure code and you
would not need to initialize rv.


>+UTF8InputStream::ReadSegments(nsWriteUnicharSegmentFun aWriter,

same thing with this method.


>Index: intl/uconv/src/nsConverterInputStream.cpp

>+nsConverterInputStream::ReadSegments(nsWriteUnicharSegmentFun aWriter,

here too.


otherwise, this looks great.  sr=darin with those tweaks.

Comment 9

[Doug Turner (:dougt)]

Comment on attachment 116333 [details] [diff] [review]
cleanup v1.1

in ReadSegments - 

if bytesToWrite == 0 and Fill() finds that there is no more data, we will
return NS_OK without setting the aReadCount.  Is that bad?

+  if (0 == bytesToWrite) {
+    // Fill the unichar buffer
+    bytesToWrite = Fill(&rv);
+    if (bytesToWrite <= 0)
+      return rv;
+  }

Look in Fill() here:

  if (nb <= 0 && mLeftOverBytes == 0) {
    // No more data 
    *aErrorCode = NS_OK;
    return 0;
  }

OOC, is there a reason that that these streams don't derive from
nsIInputStream.  Do they have to derive from nsISupports?

Comment 10

[Alec Flett]

Attachment: cleanup v1.2

ok, good catch by dougt on the not setting of *aReadCount... this one fixes
that (in both the UTF8InputStream and the nsConverterInputStream) - other than
that the patch is identical to the previous one.

Comment 11

[Alec Flett]

Comment on attachment 116438 [details] [diff] [review]
cleanup v1.2

dougt says r=dougt, carrying over sr=darin

Comment 12

[Alec Flett]

thanks everyone, fix is in.

Comment 13

[Alec Flett]

this caused bug 196731 because the mozdev.org spellchecker needs to be respun
against the current trunk.