Created attachment 338944 [details] [review]
gio: Bump copy buffer size to 256k by default

This is small enough that it shouldn't cause problems on most machines
we support, but big enough to increase throughput on a lot of devices
and network protocols.

See https://bugzilla.gnome.org/show_bug.cgi?id=773632

256kb is kinda large on the stack though. I think the default thread stack size is 2 meg, and this is 12% of that. Maybe we should use a dynamic allocation for this? I mean, the allocation will be fast compared to the i/o anyway.

Alex, see also:
https://bugzilla.gnome.org/show_bug.cgi?id=773826

Created attachment 341554 [details] [review]
gio: Use heap-allocated buffer

As if we were to increase the buffer size, it would be a bit too big to
fit on the stack.

Created attachment 341555 [details] [review]
gio: Bump copy buffer size to 256k by default

This is small enough that it shouldn't cause problems on most machines
we support, but big enough to increase throughput on a lot of devices
and network protocols.

See https://bugzilla.gnome.org/show_bug.cgi?id=773632

Review of attachment 341554 [details] [review]:

I am not GLib maint, but I suppose you need somethink like the following, because sizeof (buffer) is probably 8, or so:

@@ -2820 +2820 @@ copy_stream_with_progress (GInputStream           *in,
-      n_read = g_input_stream_read (in, buffer, sizeof (buffer), cancellable, error);
+      n_read = g_input_stream_read (in, buffer, 1024*64, cancellable, error);

So, it would be best to define some constant with the buffer size...

Created attachment 343152 [details] [review]
gio: Use heap-allocated buffer

As if we were to increase the buffer size, it would be a bit too big to
fit on the stack.

Created attachment 343153 [details] [review]
gio: Bump copy buffer size to 256k by default

This is small enough that it shouldn't cause problems on most machines
we support, but big enough to increase throughput on a lot of devices
and network protocols.

See https://bugzilla.gnome.org/show_bug.cgi?id=773632

(In reply to Ondrej Holy from comment #6)
> Review of attachment 341554 [details] [review] [review]:
> 
> I am not GLib maint, but I suppose you need somethink like the following,
> because sizeof (buffer) is probably 8, or so:

Duh, yes, fixed in the latest patches.

Review of attachment 343152 [details] [review]:

looks good

Review of attachment 343153 [details] [review]:

Can you make this (1024*256 - 2 *sizeof(gpointer)) instead? That generally means we fit in a 256k block with the standard malloc overhead.
Otherwise ok to commit.

Created attachment 343320 [details] [review]
gio: Bump copy buffer size to 256k by default

This is small enough that it shouldn't cause problems on most machines
we support, but big enough to increase throughput on a lot of devices
and network protocols.

Note that the actual value is 256k minus malloc overhead, so that it
fits nicely in a 256k block (as suggested by Alexander Larsson).

See https://bugzilla.gnome.org/show_bug.cgi?id=773632

Attachment 343152 [details] pushed as 0106a6c - gio: Use heap-allocated buffer
Attachment 343320 [details] pushed as 3b5b569 - gio: Bump copy buffer size to 256k by default

It seems that this causes deadlocks when copying, please see Bug 773823.

(In reply to Ondrej Holy from comment #14)
> It seems that this causes deadlocks when copying, please see Bug 773823.

I have a very hard time believing that, and you just pointed to the same bug you commented on.

Sorry, Bug 785391.

(In reply to Ondrej Holy from comment #16)
> Sorry, Bug 785391.

Looking at comment #32 in that bug, it seems that this problem is fixed now, without needing to revert the GLib patch from this bug? If so, it looks like it’s entirely a GVFS bug, and hence can be tracked in bug #785391.

Not fixed, but yes, it is probably gvfs-only bug.