Re: [PATCH] Vastly improve GnomeVFS xfer performance

[Christian Neumair <chris gnome-de org>]

Am Montag, den 13.03.2006, 10:22 +0100 schrieb Alexander Larsson:
> On Sun, 2006-03-12 at 16:12 +0100, Christian Neumair wrote:
> > The attached patch vastly improves the Xfer perfomance of GnomeVFS, in
> > particular the sftp performance.
> > 
> > Changes
> > 
> > * When copying file data, consider both the source and the target I/O
> > block size, reading with source block-sized buffer, writing with target
> > block-sized buffer. It could be further refined with odd I/O block sizes
> > (cf. the FIXME comment).
> 
> The block size is not in any way a limit, its more like a hint to the
> minimal size and good block size multiple to the app doing i/o, so I
> believe just using MAX(source-size, target-size) as one buffer is as
> good as using both sizes in more complicated ways. 
> 
> Also, the way you loop reading from the source in order to get at least
> isn't ideal. If you're reading from a slow source you'd block reading
> for quite some time before even starting to write to the target. What
> you want to do is to request enough data

How do you determine what "enough" means? For now, it's MAX
(source_block_size, target_block_size), so it's still sort of a lower
boundary. Of course, because we map the whole buffer in-memory, passing
a huge size may not be wise either, but was told that write() and read()
should receive the max. possible size. I'm not sure how relevant all
these considerations are for network I/O, maybe somebody knows good
literature on this, which provides statistical evidence of buffer size
and other design decisions under concrete scenarios, instead of mere
thumb rules and guesses.

> so that you could send a large
> block to the target if you got it all, but if you don't get it you
> should still send what you got instead of waiting, leaving the target
> idle.

Thanks for clarifying this, I wasn't sure on that. I'm attaching a new
patch which hopefully addresses your performance concerns.

I'd also like to replace some of the really stupid g_return_if_fail
macros in the modules by either g_asserts, or remove them completely.,
or enclose them in some DEBUG statement.

For instance, we're initializing, clearing, reading, writing, sending
and freeing buffers all the time in the sftp module, and all of them
have g_return_if_fail macros. Besides, the error conditions are really
braindead and just hint at you having misused a buffer, or at a failed
malloc. We should rather check for NULL mallocs in the realloc/init
function. I also noted that the _init function uses g_new0, but during
realloc we'll have invalid bytes at the end anyway, and we always write
to the buffer before reading.

I also think the buffer should be initialized/resized to a multiple of
the payload, i.e. the leading word for storing the payload length should
be added to the g_new, and during the resize it should be
subtracted/readded.

-- 
Christian Neumair <chris gnome-de org>

Index: libgnomevfs/gnome-vfs-xfer.c
===================================================================
RCS file: /cvs/gnome/gnome-vfs/libgnomevfs/gnome-vfs-xfer.c,v
retrieving revision 1.135
diff -u -p -r1.135 gnome-vfs-xfer.c
--- libgnomevfs/gnome-vfs-xfer.c	3 Mar 2006 16:09:31 -0000	1.135
+++ libgnomevfs/gnome-vfs-xfer.c	13 Mar 2006 11:18:37 -0000
@@ -1204,11 +1204,13 @@ copy_file_data (GnomeVFSHandle *target_h
 		GnomeVFSProgressCallbackState *progress,
 		GnomeVFSXferOptions xfer_options,
 		GnomeVFSXferErrorMode *error_mode,
-		guint block_size,
+		guint source_block_size,
+		guint target_block_size,
 		gboolean *skip)
 {
 	GnomeVFSResult result;
 	gpointer buffer;
+	char *read_buffer;
 	const char *write_buffer;
 	GnomeVFSFileSize total_bytes_read;
 	gboolean forget_cache;
@@ -1219,16 +1221,18 @@ copy_file_data (GnomeVFSHandle *target_h
 		return GNOME_VFS_ERROR_INTERRUPTED;
 	}
 
-	buffer = g_malloc (block_size);
+	buffer = g_malloc (MAX (source_block_size, target_block_size));
 	total_bytes_read = 0;
 
 	/* Enable streaming if the total size is large */
 	forget_cache = progress->progress_info->bytes_total >= DROP_CACHE_SIZE_LIMIT;
 	
 	do {
+		GnomeVFSFileSize bytes_to_read;
 		GnomeVFSFileSize bytes_read;
 		GnomeVFSFileSize bytes_to_write;
 		GnomeVFSFileSize bytes_written;
+		GnomeVFSFileSize bytes_written_one;
 		gboolean retry;
 
 		progress->progress_info->status = GNOME_VFS_XFER_PROGRESS_STATUS_OK;
@@ -1236,11 +1240,16 @@ copy_file_data (GnomeVFSHandle *target_h
 
 		progress->progress_info->phase = GNOME_VFS_XFER_PHASE_READSOURCE;
 
+		read_buffer = buffer;
+
+		bytes_to_read = MAX (source_block_size, target_block_size);
+		bytes_read = 0;
+
 		do {
 			retry = FALSE;
 
-			result = gnome_vfs_read (source_handle, buffer,
-						 block_size, &bytes_read);
+			result = gnome_vfs_read (source_handle, read_buffer,
+						 bytes_to_read, &bytes_read);
 			if (forget_cache) {
 				gnome_vfs_forget_cache (source_handle,
 							total_bytes_read, bytes_read);
@@ -1248,6 +1257,9 @@ copy_file_data (GnomeVFSHandle *target_h
 			if (result != GNOME_VFS_OK && result != GNOME_VFS_ERROR_EOF)
 				retry = handle_error (&result, progress,
 						      error_mode, skip);
+
+			bytes_to_read -= bytes_read;
+			read_buffer += bytes_read;
 		} while (retry);
 
 		if (result != GNOME_VFS_OK || bytes_read == 0 || *skip) {
@@ -1255,24 +1267,26 @@ copy_file_data (GnomeVFSHandle *target_h
 		}
 
 		total_bytes_read += bytes_read;
-		bytes_to_write = bytes_read;
 
 		progress->progress_info->phase = GNOME_VFS_XFER_PHASE_WRITETARGET;
 
+		bytes_to_write = bytes_read;
+		bytes_written = 0;
+
 		write_buffer = buffer;
 		do {
 			retry = FALSE;
 
 			result = gnome_vfs_write (target_handle, write_buffer,
-						  bytes_to_write,
-						  &bytes_written);
+						  bytes_to_write, &bytes_written_one);
 
 			if (result != GNOME_VFS_OK) {
 				retry = handle_error (&result, progress, error_mode, skip);
 			}
 
-			bytes_to_write -= bytes_written;
-			write_buffer += bytes_written;
+			bytes_to_write -= bytes_written_one;
+			write_buffer += bytes_written_one;
+			bytes_written += bytes_written_one;
 		} while ((result == GNOME_VFS_OK && bytes_to_write > 0) || retry);
 		
 		if (forget_cache && bytes_to_write == 0) {
@@ -1280,6 +1294,11 @@ copy_file_data (GnomeVFSHandle *target_h
 						total_bytes_read - bytes_read, bytes_read);
 		}
 
+		if (result == GNOME_VFS_OK && bytes_read != bytes_written) {
+			g_warning ("bytes read/bytes written don't match!");
+			result = GNOME_VFS_ERROR_INTERNAL;
+		}
+
 		progress->progress_info->phase = GNOME_VFS_XFER_PHASE_COPYING;
 
 		progress->progress_info->bytes_copied += bytes_read;
@@ -1443,7 +1462,8 @@ copy_symlink (GnomeVFSURI *source_uri,
 }
 
 static GnomeVFSResult
-copy_file (GnomeVFSFileInfo *info,  
+copy_file (GnomeVFSFileInfo *info,
+	   GnomeVFSFileInfo *target_dir_info,
 	   GnomeVFSURI *source_uri,
 	   GnomeVFSURI *target_uri,
 	   GnomeVFSXferOptions xfer_options,
@@ -1498,6 +1518,8 @@ copy_file (GnomeVFSFileInfo *info,  
 					  */
 					 (info->valid_fields & GNOME_VFS_FILE_INFO_FIELDS_IO_BLOCK_SIZE && info->io_block_size > 0)
 					 ? info->io_block_size : 8192,
+					 (target_dir_info->valid_fields & GNOME_VFS_FILE_INFO_FIELDS_IO_BLOCK_SIZE && target_dir_info->io_block_size > 0)
+					 ? target_dir_info->io_block_size : 8192,
 					 skip);
 	}
 
@@ -1644,7 +1666,8 @@ copy_directory (GnomeVFSFileInfo *source
 				progress_set_source_target_uris (progress, source_uri, dest_uri);
 
 				if (info->type == GNOME_VFS_FILE_TYPE_REGULAR) {
-					result = copy_file (info, source_uri, dest_uri, 
+					result = copy_file (info, target_dir_info,
+							    source_uri, dest_uri, 
 							    xfer_options, error_mode, overwrite_mode, 
 							    progress, &skip_child);
 				} else if (info->type == GNOME_VFS_FILE_TYPE_DIRECTORY) {
@@ -1657,7 +1680,8 @@ copy_directory (GnomeVFSFileInfo *source
 						result = gnome_vfs_get_file_info_uri (source_uri, symlink_target_info,
 										      GNOME_VFS_FILE_INFO_FOLLOW_LINKS);
 						if (result == GNOME_VFS_OK) 
-							result = copy_file (symlink_target_info, source_uri, dest_uri, 
+							result = copy_file (symlink_target_info, target_dir_info,
+									    source_uri, dest_uri, 
 									    xfer_options, error_mode, overwrite_mode, 
 									    progress, &skip_child);
 						gnome_vfs_file_info_unref (symlink_target_info);
@@ -1786,7 +1810,6 @@ copy_items (const GList *source_uri_list
 			if (result == GNOME_VFS_OK && GNOME_VFS_FILE_INFO_SGID(target_dir_info)) {
 				info->gid = target_dir_info->gid;
 			}
-			gnome_vfs_file_info_unref (target_dir_info);
 			result = GNOME_VFS_OK;
 
 			/* optionally keep trying until we hit a unique target name */
@@ -1811,7 +1834,8 @@ copy_items (const GList *source_uri_list
 				
 				
 				if (info->type == GNOME_VFS_FILE_TYPE_REGULAR) {
-					result = copy_file (info, source_uri, target_uri, 
+					result = copy_file (info, target_dir_info,
+							    source_uri, target_uri, 
 							    xfer_options, error_mode, 
 							    &overwrite_mode_abort, 
 							    progress, &skip);
@@ -1871,6 +1895,8 @@ copy_items (const GList *source_uri_list
 				
 				/* try again with new uri */
 			}
+
+			gnome_vfs_file_info_unref (target_dir_info);
 		}
 
 		gnome_vfs_file_info_unref (info);
Index: modules/sftp-method.c
===================================================================
RCS file: /cvs/gnome/gnome-vfs/modules/sftp-method.c,v
retrieving revision 1.40
diff -u -p -r1.40 sftp-method.c
--- modules/sftp-method.c	23 Feb 2006 19:08:12 -0000	1.40
+++ modules/sftp-method.c	13 Mar 2006 11:18:53 -0000
@@ -523,7 +525,7 @@ buffer_read_file_info (Buffer *buf, Gnom
 	}
 
 	info->valid_fields |= GNOME_VFS_FILE_INFO_FIELDS_IO_BLOCK_SIZE;
-	info->io_block_size = default_req_len;
+	info->io_block_size = max_req * default_req_len;
 
 	DEBUG4 (g_log (G_LOG_DOMAIN, G_LOG_LEVEL_DEBUG, "Read file info from %p", buf));
 }
@@ -1963,11 +1969,10 @@ do_read (GnomeVFSMethod       *method, 
 	Buffer msg;
 	char type;
 	int recv_id, status;
-	guint num_req, req_ptr, req_svc_ptr, req_svc;
+	guint num_req, req_ptr, req_svc;
 	guint len;
 	guchar *buffer;
 	guchar *curr_ptr;
-	gboolean out_of_order;
 	GnomeVFSResult result;
 	gboolean got_eof;
 	guint outstanding;
@@ -1989,7 +1994,6 @@ do_read (GnomeVFSMethod       *method, 
 	*bytes_read = 0;
 	num_req = 0;
 	req_ptr = 0;
-	req_svc_ptr = 0;
 	curr_ptr = buffer_in;
 	buffer = buffer_in;
 	outstanding = 0;
@@ -2041,16 +2045,12 @@ do_read (GnomeVFSMethod       *method, 
 		recv_id = buffer_read_gint32 (&msg);
 
 		/* Look for the id received among sent ids */
-		out_of_order = FALSE;
-		req_svc = req_svc_ptr;
-		while (read_req[req_svc].id != recv_id && req_svc != req_ptr) {
-			if (read_req[req_svc].id != 0)
-				out_of_order = TRUE;
-			if (++req_svc >= max_req)
-				req_svc = 0;
+		for (req_svc = 0; req_svc < max_req; req_svc++) {
+			if (read_req[req_svc].id == recv_id)
+				break;
 		}
 
-		if (req_svc == req_ptr) { /* Didn't find the id -- unexpected reply */
+		if (req_svc >= max_req) { /* Didn't find the id -- unexpected reply */
 			DEBUG (g_log (G_LOG_DOMAIN, G_LOG_LEVEL_DEBUG, "%s: Unexpected id %d",
 				      __FUNCTION__, recv_id));
 			buffer_free (&msg);
@@ -2119,9 +2119,6 @@ do_read (GnomeVFSMethod       *method, 
 			sftp_connection_unlock (handle->connection);
 			return GNOME_VFS_ERROR_PROTOCOL_ERROR;
 		}
-
-		if (!out_of_order)
-			req_svc_ptr = req_svc;
 	}
 
 	handle->offset += *bytes_read;