Closed Bug 1409394 Opened 2 years ago Closed 2 years ago

Avoid extra buffer copy in FileReader

Categories

(Core :: DOM: Core & HTML, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: baku, Assigned: baku)

References

Details

Attachments

(1 file)

Depends on: 1408397
Attached patch fix4.patchSplinter Review
Attachment #8919302 - Flags: review?(bugs)
Comment on attachment 8919302 [details] [diff] [review]
fix4.patch

Review of attachment 8919302 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/file/FileReader.cpp
@@ +324,5 @@
>  
> +    if (NS_InputStreamIsBuffered(mAsyncStream)) {
> +      nsresult rv = mAsyncStream->ReadSegments(ReadFuncBinaryString, dest,
> +                                               aCount, &bytesRead);
> +      if (NS_WARN_IF(NS_FAILED(rv))) {

nit: If you are going to use NS_ENSURE_SUCCESS() below you might as well use it here too.  (FWIW, I'm pro-NS_ENSURE_* these days.)
I've always been pro-NS_ENSURE_* ;)
Comment on attachment 8919302 [details] [diff] [review]
fix4.patch


>+void
>+PopolateBufferForBinaryString(char16_t* aDest, const char* aSource,
>+                              uint32_t aCount)
Populate


>+ReadFuncBinaryString(nsIInputStream* aInputStream,
>+                     void* aClosure,
>+                     const char* aFromRawSegment,
>+                     uint32_t aToOffset,
>+                     uint32_t aCount,
>+                     uint32_t *aWriteCount)
* goes with the type.


>     dest += oldLen;
> 
>-    while (aCount > 0) {
>-      char tmpBuffer[4096];
>-      uint32_t minCount = XPCOM_MIN(aCount, sizeof(tmpBuffer));
>-      uint32_t read;
>+    if (NS_InputStreamIsBuffered(mAsyncStream)) {
>+      nsresult rv = mAsyncStream->ReadSegments(ReadFuncBinaryString, dest,
>+                                               aCount, &bytesRead);
>+      if (NS_WARN_IF(NS_FAILED(rv))) {
>+        return rv;
>+      }
Feel free to use NS_ENSURE_SUCCESS(rv, rv);
Attachment #8919302 - Flags: review?(bugs) → review+
Priority: -- → P3
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef64f4c98cc6
Avoid extra buffer copy in FileReader, r=smaug
Flags: needinfo?(amarchesini)
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b7edd51196c
Avoid extra buffer copy in FileReader, r=smaug
https://hg.mozilla.org/mozilla-central/rev/1b7edd51196c
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.