Closed Bug 545869 Opened 10 years ago Closed 10 years ago

Remove small buffer #defines and use preferences.

Categories

(Core :: Networking, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.3a5
Tracking Status
status1.9.2 --- .5-fixed

People

(Reporter: dougt, Assigned: dougt)

References

Details

Attachments

(2 files, 4 obsolete files)

Attached patch patch v.1 (obsolete) — Splinter Review
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.
Attachment #426684 - Flags: review?(bzbarsky)
Attachment #426684 - Attachment is patch: true
Attachment #426684 - Attachment mime type: application/octet-stream → text/plain
Assignee: nobody → mozbugz
>+        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?
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?
Attachment #426684 - Flags: review?(bzbarsky) → review-
>+        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....
> Just moving stuff around.

Ah, indeed.  Well, worth fixing it in the proces anyway.  ;)
Attached patch patch v.2 (obsolete) — Splinter Review
Attachment #426684 - Attachment is obsolete: true
Attachment #426704 - Flags: review?(jduell.mcbugs)
Attached patch a few more buffers (obsolete) — Splinter Review
Attachment #426704 - Attachment is obsolete: true
Attachment #426756 - Flags: review?
Attachment #426704 - Flags: review?(jduell.mcbugs)
jason, this is a bit of a wip.  but, please review in any case.
Attachment #426756 - Flags: review?
Attachment #426756 - Flags: feedback?(bzbarsky)
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
Note: 4096KB buffer size is the necko default. I was using an N900 over Wifi.
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 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.
Attachment #426756 - Flags: review?(bzbarsky)
Attachment #426756 - Flags: feedback?(bzbarsky)
Attachment #426756 - Flags: feedback+
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.
Attachment #426756 - Flags: review?(bzbarsky) → review+
we can look at nsPipe values in a separate bug, is suppose..
Attachment #426756 - Attachment is obsolete: true
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
honza, good catch.  will fix these locally and push.
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?
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.
Unless I missed something, nsExternalHelperAppHandler is not using those consistent buffer sizes.

Not is SSL, fwiw; NSS uses 16KB buffers internally.
sure.  necko == mozilla/netwerk.
Why is this hardcoded, and not at least #defined?
(15 * 60), // 15 minutes
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.
> 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?
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?
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.
http://hg.mozilla.org/mozilla-central/rev/10d5c11a5b9a
http://hg.mozilla.org/mozilla-central/rev/ab2dbfc1d099
Status: NEW → RESOLVED
tracking-fennec: --- → ?
Closed: 10 years ago
Resolution: --- → FIXED
Duplicate of this bug: 545334
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
Attachment #440134 - Flags: approval1.9.2.5?
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.
Duplicate of this bug: 545827
Target Milestone: --- → mozilla1.9.3a5
Version: unspecified → Trunk
So we have a followup bug to change buffer sizes here, right?  Is that just bug 545334?
(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.
Attached patch patch for m-192 (obsolete) — Splinter Review
same as the m-c patch, but unbitrotted for m-192
Attachment #442488 - Flags: review?(jduell.mcbugs)
Attachment #442488 - Flags: approval1.9.2.5?
Comment on attachment 440134 [details] [diff] [review]
issues address.  applies to m-c

separate patch for m-192
Attachment #440134 - Flags: approval1.9.2.5?
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.
Attachment #442488 - Flags: review?(jduell.mcbugs) → review+
Attached patch patch for 192 v2Splinter Review
Updated patch for mozilla-1925, with review comments addressed. Carrying jduell's r+ forward. Asking for approval for 1925.
Attachment #442488 - Attachment is obsolete: true
Attachment #443155 - Flags: review+
Attachment #443155 - Flags: approval1.9.2.5?
Attachment #442488 - Flags: approval1.9.2.5?
Attachment #443155 - Flags: approval1.9.2.5? → approval1.9.2.5+
tracking-fennec: ? → ---
You need to log in before you can comment on or make changes to this bug.