545869 - Remove small buffer #defines and use preferences.

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Doug Turner (:dougt)]

Attachment: patch v.1

We have a #define called NECKO_SMALL_BUFFERS.  It was added a long time ago.  Data suggested that it was a good thing to do.  No one currently uses it, and now we have data that suggests that it puts alot more stress on the CPU.

I suggest that we remove this #define, and just add preferences so that we can more easily measure the best setting for segment count and size.

Comment 1

[Boris Zbarsky [:bzbarsky]]

>+        nsCOMPtr<nsIMemory> eyeMemory = do_QueryInterface(recyclingAllocator);
>+        gBufferCache = eyeMemory.get();
>+        NS_IF_ADDREF(gBufferCache);

  CallQueryInterface(recyclingAllocator, &gBufferCache);

?

Jason, do you think you can do this review?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Oh, and at least the change to nsSocketTransport::OpenInputStream is wrong, since it changes the behavior.  I haven't looked at the other changes that involve net_ResolveSegmentParams in detail, but at first glance they're all likely to be wrong.

Oh, and the HTTP changes mean that HTTP might now use two different buffer sizes internally, depending on the pref value.  Did you mean to change all the HTTP_SEGMENT_SIZE consumers?

Comment 3

[Doug Turner (:dougt)]

>+        nsCOMPtr<nsIMemory> eyeMemory = do_QueryInterface(recyclingAllocator);
>+        gBufferCache = eyeMemory.get();
>+        NS_IF_ADDREF(gBufferCache);

Just moving stuff around.

> nsSocketTransport::OpenInputStream 

I will just keep the utility header, and make it use the nsIOService for getting at the defaults.

new patch coming up....

Comment 4

[Boris Zbarsky [:bzbarsky]]

> Just moving stuff around.

Ah, indeed.  Well, worth fixing it in the proces anyway.  ;)

Comment 5

[Doug Turner (:dougt)]

Attachment: patch v.2

Comment 6

[Doug Turner (:dougt)]

Attachment: a few more buffers

Comment 7

[Doug Turner (:dougt)]

jason, this is a bit of a wip.  but, please review in any case.

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

Some initial timing data. I applied this patch to a current m-192 tree and built Fennec. I added some simple timing code to the webprogress listener onStateChange to measure the time from network-start to network-stop.

I used two text files I have on people: one is a 566KB file (med file) the other is a 2.0MB file (large file). I measured the network time at 3 buffer sizes. Here are the averages over 7 consecutive runs:

           4096KB   16384KB   32768KB
-------------------------------------
med:       6204ms    5458ms    4872ms
large:    16613ms   14794ms   15229ms

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

Note: 4096KB buffer size is the necko default. I was using an N900 over Wifi.

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

Crap, those buffer sizes are 4KB, 16KB and 32KB (sorry for bad labels)

There is also information suggesting the cpu load is lower with 16KB buffers
https://bugzilla.mozilla.org/show_bug.cgi?id=545334#c0

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 426756 [details] [diff] [review]
a few more buffers

Asking for full review since the patch applies well and tweaking the buffer size seems to yield faster pageloads.

Comment 12

[Honza Bambas (:mayhemer)]

Comment on attachment 426756 [details] [diff] [review]
a few more buffers

>diff --git a/netwerk/base/src/nsAsyncStreamCopier.cpp b/netwerk/base/src/nsAsyncStreamCopier.cpp
>@@ -229,17 +230,17 @@ nsAsyncStreamCopier::Init(nsIInputStream

Indention (4 spaces).

>+++ b/netwerk/base/src/nsIOService.cpp
>+#define NECKO_BUFFER_CACHE_COUNT_PREF "network.buffer.cache.count"
>+#define NECKO_BUFFER_CACHE_SIZE_PREF  "network.buffer.cache.size"

Have defaults in prefs.js for these?

>@@ -246,16 +234,33 @@ nsIOService::Init()
>+    // Get the allocator ready
>+    if (!gBufferCache)
>+    {
>+        nsresult rv = NS_OK;
>+        nsCOMPtr<nsIRecyclingAllocator> recyclingAllocator =
>+            do_CreateInstance(NS_RECYCLINGALLOCATOR_CONTRACTID, &rv);
>+        if (NS_FAILED(rv))
>+            return rv;
>+        rv = recyclingAllocator->Init(gDefaultSegmentCount,
>+                                      (15 * 60), // 15 minutes
>+                                      "necko");
>+        if (NS_FAILED(rv))
>+            return rv;
>+
>+        CallQueryInterface(recyclingAllocator, &gBufferCache);
>+    }
>+

We shouldn't fail Init() when recyclingAllocator init fails.  Just don't have gBufferCache then.

>@@ -789,16 +794,30 @@ nsIOService::PrefsChanged(nsIPrefBranch 
>+
>+    if (!pref || strcmp(pref, NECKO_BUFFER_CACHE_COUNT_PREF) == 0) {
>+        PRInt32 count;
>+        if (NS_SUCCEEDED(prefs->GetIntPref(NECKO_BUFFER_CACHE_COUNT_PREF,
>+                                           &count)))
>+            gDefaultSegmentCount = count;
>+    }
>+
>+    if (!pref || strcmp(pref, NECKO_BUFFER_CACHE_SIZE_PREF) == 0) {
>+        PRInt32 size;
>+        if (NS_SUCCEEDED(prefs->GetIntPref(NECKO_BUFFER_CACHE_SIZE_PREF,
>+                                           &size)))
>+            gDefaultSegmentSize = size;
>+    }

Sanitize negative or zero values of the prefs and also check for 32 bits overflows (if not even 31 bits or even yet smaller) after multiply.  It should fail over to default values then.  Perfect would also be to have the size be checked or automatically aligned for being power of 2.  It is required on many places to be that.

>diff --git a/netwerk/base/src/nsNetSegmentUtils.h b/netwerk/base/src/nsNetSegmentUtils.h

Indention in functions.

>diff --git a/netwerk/protocol/file/src/nsFileChannel.cpp b/netwerk/protocol/file/src/nsFileChannel.cpp
> nsFileCopyEvent::DoCopy()
>-  const PRInt32 chunk = NET_DEFAULT_SEGMENT_SIZE * NET_DEFAULT_SEGMENT_COUNT;
>+  const PRInt32 chunk = nsIOService::gDefaultSegmentSize * nsIOService::gDefaultSegmentCount;

nsIOService::gDefaultSegment* are PRUint32.  This might overflow to negaitive buffer sizes (if we get that far with such buffer settings ;) ).

>diff --git a/netwerk/protocol/http/src/nsHttpPipeline.cpp b/netwerk/protocol/http/src/nsHttpPipeline.cpp
>@@ -553,18 +553,18 @@ nsHttpPipeline::FillSendBuf()
>-                        NS_HTTP_SEGMENT_SIZE,
>-                        NS_HTTP_SEGMENT_SIZE,
>+                        nsIOService::gDefaultSegmentSize,
>+                        nsIOService::gDefaultSegmentCount,

It both has to be SegmentSize. See http://mxr.mozilla.org/mozilla-central/source/xpcom/io/nsPipe3.cpp#1262

It is ok in nsHttpTransaction::Init where NS_NewPipe2 is used instead.

diff --git a/netwerk/streamconv/converters/nsBinHexDecoder.cpp b/netwerk/streamconv/converters/nsBinHexDecoder.cpp
@@ -271,17 +267,17 @@ nsresult nsBinHexDecoder::ProcessNextSta
+      else if (mPosOutputBuff >= nsIOService::gDefaultSegmentSize)

You should cast nsIOService::gDefaultSegmentSize to PRInt32 here.  This is another reason to enforce 31 bit boundry on the segmentsize * segmentcount result.
 
>diff --git a/uriloader/exthandler/nsExternalHelperAppService.cpp b/uriloader/exthandler/nsExternalHelperAppService.cpp
> nsresult nsExternalHelperAppService::Init()
>+  nsCOMPtr<nsIPrefBranch> prefs(do_GetService(NS_PREFSERVICE_CONTRACTID));
>+  if (!prefs)
>+    return NS_ERROR_UNEXPECTED;
>+  

Probably left over? You don't use it here but you do in the nsExternalAppHandler's constructor.


Question is if we should also change the defaults in nsPipe, but it is on a lower level then necko and might be dangerous to change.

Comment 13

[Doug Turner (:dougt)]

we can look at nsPipe values in a separate bug, is suppose..

Comment 14

[Doug Turner (:dougt)]

Attachment: issues address. applies to m-c

Comment 15

[Honza Bambas (:mayhemer)]

Comment on attachment 440134 [details] [diff] [review]
issues address.  applies to m-c

Few nits:
+++ b/netwerk/protocol/http/src/nsHttpPipeline.cpp
You added a new line to the top of the file

nsHttpPipeline::FillSendBuf
You forgot to fix the arguments

Comment 16

[Doug Turner (:dougt)]

honza, good catch.  will fix these locally and push.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Do we really want to use the large segments for the FTP control channel?  I suppose it doesn't matter that much...

The change to nsBinHexDecoder::OnStartRequest doesn't make sense to me.  Why are you decreasing both the buffer size and the number of buffers?

Similar for the external helper app handler.  Why use smaller buffers now (but default to the old bigger size if the pref is not set)?  And the OOM checking here is broken...  Either don't check at all (and document why not) or do it right?

Comment 18

[Doug Turner (:dougt)]

as for the buffer size and segment count changes, I thought about having preferences for each of the areas that allocate using different dimension.  However, I couldn't see any signification performance difference using the default in nsIOService.

The change's benefit is that there is one consistent buffer size and segment size used throughout necko.  And the challenge now is figure out if there are areas that could be optimized with a different buffer size / segment count.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Unless I missed something, nsExternalHelperAppHandler is not using those consistent buffer sizes.

Not is SSL, fwiw; NSS uses 16KB buffers internally.

Comment 20

[Doug Turner (:dougt)]

sure.  necko == mozilla/netwerk.

Comment 21

[Alfred Kayser]

Why is this hardcoded, and not at least #defined?
(15 * 60), // 15 minutes

Comment 22

[Doug Turner (:dougt)]

alfred, it was:

-#define NS_NECKO_15_MINS (15 * 60)

I moved the code, and dropped the #define.  The define was only used once and it really didn't offer anything more than the comment that I added.

Comment 23

[Boris Zbarsky [:bzbarsky]]

> sure.  necko == mozilla/netwerk.

Sure, but I'm still confused about why exthandler uses the same prefs but a different default.  Is there a reason?

Comment 24

[Doug Turner (:dougt)]

no reason.

 mBufferSize = 8192;

should become:


 mBufferSize = 4096;


Since we added a pref to all.js, it really doesn't matter unless someone removes the pref, but I will push this small change when the tree go green.  bz, okay?

Comment 25

[Boris Zbarsky [:bzbarsky]]

Sounds good.  And for the record, dougt pointed me to where the correct if (!mDataBuffer) check is done in the exthandler, so that code is ok.

Comment 26

[Doug Turner (:dougt)]

http://hg.mozilla.org/mozilla-central/rev/10d5c11a5b9a
http://hg.mozilla.org/mozilla-central/rev/ab2dbfc1d099

Comment 28

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 440134 [details] [diff] [review]
issues address.  applies to m-c

We would love to get this on 192 for Fennec 1.1

Comment 29

[Alfred Kayser]

Bug 545334 asks for bigger buffers (16K instead of 4K). 
This bug only added prefs to set buffer size and count.
I think that 545334 is not a dup of this one, but still a separate and valid request.

Comment 31

[Boris Zbarsky [:bzbarsky]]

So we have a followup bug to change buffer sizes here, right?  Is that just bug 545334?

Comment 32

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #31)
> So we have a followup bug to change buffer sizes here, right?  Is that just bug
> 545334?

That seems correct. Mobile updated buffer sizes in bug 560591.

Comment 33

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch for m-192

same as the m-c patch, but unbitrotted for m-192

Comment 34

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 440134 [details] [diff] [review]
issues address.  applies to m-c

separate patch for m-192

Comment 35

[Jason Duell]

Comment on attachment 442488 [details] [diff] [review]
patch for m-192

>     if (!mSendBufIn) {
>         // allocate a single-segment pipe
>         rv = NS_NewPipe(getter_AddRefs(mSendBufIn),
>                         getter_AddRefs(mSendBufOut),
>-                        NS_HTTP_SEGMENT_SIZE,
>-                        NS_HTTP_SEGMENT_SIZE,
>+                        nsIOService::gDefaultSegmentSize,
>+                        nsIOService::gDefaultSegmentCount,

In the m-c patch we have gDefaultSegmentSize being passed twice, while here you're passing gDefaultSegmentCount the 2nd time.  It's very unlikely that one or the other of these isn't wrong.  From a quick perusal of the code, it looks like the m-c version is right (ie pass SegSize twice).

Unless I'm mistaken, this is the only change in the 1.9.2. patch other than context changes.


>diff --git a/uriloader/exthandler/nsExternalHelperAppService.cpp b/uriloader/exthandler/nsExternalHelperAppService.cpp
>--- a/uriloader/exthandler/nsExternalHelperAppService.cpp
>+++ b/uriloader/exthandler/nsExternalHelperAppService.cpp
>@@ -1104,16 +1104,17 @@ nsExternalAppHandler::nsExternalAppHandl
> , mShouldCloseWindow(PR_FALSE)
> , mReceivedDispositionInfo(PR_FALSE)
> , mStopRequestIssued(PR_FALSE)
> , mProgressListenerInitialized(PR_FALSE)
> , mReason(aReason)
> , mContentLength(-1)
> , mProgress(0)
> , mRequest(nsnull)
>+, mDataBuffer(nsnull)
> {
> 
>   // make sure the extention includes the '.'
>   if (!aTempFileExtension.IsEmpty() && aTempFileExtension.First() != '.')
>     mTempFileExtension = PRUnichar('.');
>   AppendUTF8toUTF16(aTempFileExtension, mTempFileExtension);
> 
>   // replace platform specific path separator and illegal characters to avoid any confusion
>@@ -1132,22 +1133,40 @@ nsExternalAppHandler::nsExternalAppHandl
>     mSuggestedFileName.ReplaceChar(unsafeBidiCharacters[i], '_');
>     mTempFileExtension.ReplaceChar(unsafeBidiCharacters[i], '_');
>   }
>   
>   // Make sure extension is correct.
>   EnsureSuggestedFileName();
> 
>   gExtProtSvc->AddRef();
>+
>+  nsCOMPtr<nsIPrefBranch> prefs(do_GetService(NS_PREFSERVICE_CONTRACTID));
>+  if (!prefs)
>+    return;
>+
>+  mBufferSize = 8192;

Any reason why you're not changing mBufferSize to 4096, as the 2nd checkin in comment 26 did for m-c?  

+r with those two issues cleared up.

Comment 36

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch for 192 v2

Updated patch for mozilla-1925, with review comments addressed. Carrying jduell's r+ forward. Asking for approval for 1925.

Comment 37

[Mark Finkle (:mfinkle) (use needinfo?)]

pushed to m-1.9.2.5:
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/9abae22747f5