1409394 - Avoid extra buffer copy in FileReader

Status: RESOLVED FIXED

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

Description

[Andrea Marchesini [:baku]]

Follow up of https://bugzilla.mozilla.org/show_bug.cgi?id=1408397#c19

Comment 1

[Andrea Marchesini [:baku]]

Attachment: fix4.patch

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

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.)

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

I've always been pro-NS_ENSURE_* ;)

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

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);

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef64f4c98cc6
Avoid extra buffer copy in FileReader, r=smaug

Comment 6

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out bug 1409394, bug 1409329 and bug 1409327 for failing browser-chrome's browser_save_link-perwindowpb.js and browser_libraryDrop.js on OS X and leaks detected by chrome test on Linux x64 asan:

Bug 1409394
https://hg.mozilla.org/integration/mozilla-inbound/rev/a461a81095f8b734427f244e1bb7e09d53713e65

Bug 1409329
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec2f3cbc85809e0d9937383f1a0db0750dd9ed1f

Bug 1409327
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb8c65940190581eb878f9999ae71a50d9fec873

Push which ran many failing tests: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=8b0c016826e027fad395b6e6e11d8fe178080489&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception
Likely fallout from bug 1409329.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b7edd51196c
Avoid extra buffer copy in FileReader, r=smaug

Comment 8

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/1b7edd51196c