bugmonotone - Bugs: bug #13300, Performance of network pulls/pushs...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

bug #13300: Performance of network pulls/pushs is atrociously poor with large binary files

Submitter:  Eric Anderson <ea9bkueh7s7g>
Submitted:  Sun 05 Jun 2005 04:35:00 AM UTC
   
 
Category:  None Severity:  3 - Normal
Item Group:  code cleanup Status:  Fixed
Privacy:  Public Assigned to:  None
Open/Closed:  Closed
mtn version --full: 

monotone 0.19 (base revision: 168adf9537ff136c9b7fe7faad5991f92859390d)
Running on: Linux 2.6.11.9 #1 Thu May 12 12:47:38 PDT 2005 i686
Changes since base revision:

new_manifest [52a617d908ac6c4bb5b837ce5306f75155dc59ef]

old_revision [168adf9537ff136c9b7fe7faad5991f92859390d]
old_manifest [a9ee1d741b855fdcc0d038d64d913cef70da72f5]

patch "po/monotone.pot"
 from [10e6d7cbad87eaa0dbe35c803cafa371567f024b]
   to [0465f6ac8d09fac5938ac067747b179aca677b67]


  Generated from data cached in the distribution;
  further changes may have been made.

  Generated from data cached in the distribution;
  further changes may have been made.

  Generated from data cached in the distribution;
  further changes may have been made.

Mon 06 Jun 2005 08:59:48 PM UTC, comment #4: 

With a bunch of 2-5 MB files, I get a 70x CPU reduction and a 23x wall time speedup.  This strikes me as enough to be worth merging immediately, and examining the memory usage problem later; it's close to a show-stopper for us.

There's definitely something to look at with the memory usage.  I just did a commit of a 100MB random file and it used ~800 MB to do the commit.  When I did the push of that database, one of the two sides used ~1GB.  However it looked like most of the usage was after the transfer had completed.  During the transfer, the machines hovered around 200MB, which is tolerable.  Once the transfer reached 100% I got the bloat up to 1GB.

It would be easy to extend my patch to have a "condense buffer" operation; I don't think it will integrate well with the swap trick however.  In order to avoid copies, I can have the start of a string not at the beginning of the malloced region -- this won't play well with how C++ strings represent things.

I may have a change to take a look at the memory usage problem in the next few days; a 8-10x increase over the file size could become a problem.  I just hadn't noticed since my laptop has 1.5GB of memory.

Eric Anderson <ea9bkueh7s7g>
Mon 06 Jun 2005 03:17:21 PM UTC, comment #3: 

So, following the principle of "never look at code if you can get someone to do it for you" ;-), I asked Tim Brownawell (who wrote that other patch) if he agreed with your estimate:
http://www.loglibrary.com/show_page/view/106?Multiplier=3600&Interval=6&StartTime=1117959873

To summarize: his patch was more about reducing memory usage, which is a good thing to do independent of buffer copies, I think.  (We really do have a memory usage problem in these cases.)  Do you think there are any chances of a solution combining these features?  Is your patch enough faster that we should just apply it and worry about also reducing memory usage in a second iteration?

Thanks for the work, by the way...

Nathaniel Smith <njs>
Group Member
Sun 05 Jun 2005 06:20:32 AM UTC, comment #2: 

So the adjustments that I made are pretty much a ring buffer (a copy occurs when we run out of space in the buffer at the end, and when we need to resize the buffer). A possibly better approach is to use a collection of buffers, which could entirely eliminate the copying within the buffer except when there are requests for contigous bits of the buffer.  After I applied the patch, the profiling numbers indicated that the bottlenecks were no longer there, so while it was possible to do further optimization, additional complexity for negligable performance improvement seemed like a bad tradeoff.

A std::deque could be a bad choice with some compilers.  Many years ago I discovered the performance was significantly worse than the freshman algorithms deque (array + pair of pointers rotating through).  The stl implementation (near as I can tell) also doesn't leave things contigous, which makes things that are done with the buffer more inconvinient.

I just finished looking at the fix in 1dcf*; I think the string buffer approach in my patch with the explicit copy is better -- the very first inbuf.erase(0,pos) at line 135 in that version right after the require_bytes has already done all the damage by forcing a copy from pos+1..end of string to 0; in fact this version may be worse because it has two calls to inbuf.erase(0,something), which means that everything after is potentially copied twice.

Eric Anderson <ea9bkueh7s7g>
Sun 05 Jun 2005 05:14:46 AM UTC, comment #1: 

Thanks for the patch.  This exact problem actually got some work recently on mainline, but so far only on the receiving end (using "swap" to avoid buffer copies -- not sure how your changes to the input buffer compare).  The changes occurred in 1dcf5b3cb746ea6c1848280d7ad6ca9f42a29eb3 , if you'd like to see.

Would it be better to use a ring buffer, instead of this moving pointer + occasional copies?  then you only have to copy when the buffer overflows/underflows?  Or possibly a std::deque, I suppose?

Nathaniel Smith <njs>
Group Member
Sun 05 Jun 2005 04:35:00 AM UTC, original submission:  

I wanted to put a collection of large binary files into a monotone repository, and I discovered the the performance of the sender with these large files is atrociously bad.  In particular, there are two related problems:
  1) in netsync.cc it will continue to arm connections (and hence parse more requests, and generate more pending output) even if the output buffer is entirely backed up.
  2) in netcmd.cc, a string is used to store the bits that are pending, and when data is sent (at most 0xfff bytes), it is removed from the front of the string, requiring a copy of the data.  If the output buffer contains ~1MB, this will generate a stunning amount of memory moves as 4095 bytes are stripped off the front of the buffer repeatedly, and the remainder is copied.

The solution is to maintain a string buffer that has pointers to the active data partway through the buffer.  This means that in most cases, removing from the front just costs a pointer update, and only occasionally does the data in the buffer need to be copied to the front.  (If the data didn't need to be contiguous, this too could be fixed, but it would be more complicated to apply this fix)

The net effect is a 73.5x cpu reduction on one of my tests, and a 2-4x cpu reduction on the others.  Since I was testing on a local machine, the pulls were almost completely CPU bound, and hence this worked out to a 1.56x-23x wall clock speedup.

Eric Anderson <ea9bkueh7s7g>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attached Files
file #3039:  perf-improve.patch added by ea9bkueh7s7g (12KiB - text/x-diff - Patch to provide for the performance improvement)

 

Depends on the following items: None found

Items that depend on this one: None found

 

CC list is empty

 

There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

 

Follow 3 latest changes.

Date Changed by Updated Field Previous Value => Replaced by
2006-04-26 tbrownaw StatusNone Fixed
    Open/ClosedOpen Closed
2005-06-05 ea9bkueh7s7g Attached File- Added perf-improve.patch, #2573

Back to the top

Powered by Savane 3.15-64aa.
Corresponding source code