Re: [PATCH 11/17] Fully activate the sliding window pack access.

From: Junio C Hamano <junkio@cox.net>
Date: Dec 23, 2006, 19:45 UTC
Message-ID: <7vodpuqtuf.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <Pine.LNX.4.64.0612231038410.3671@woody.osdl.org>

Linus Torvalds <torvalds@osdl.org> writes:

Show 21 quoted lines

> On Sat, 23 Dec 2006, Shawn O. Pearce wrote:
>>
>> This finally turns on the sliding window behavior for packfile data
>> access by mapping limited size windows and chaining them under the
>> packed_git->windows list.
>
> Hmm. You seem to default the window size to 32MB.
>
> Maybe I'm reading that code wrong, but I think that's a bit sad.
>
> In particular, in any setup that doesn't like mmap() at all (eg Cygwin), 
> and uses the "malloc+read" emulation, 32MB is actually likely much too 
> big. It's probably better to use something like a 1MB slice instead, since 
> otherwise you'll often be reading much too much.
>
> So I'd argue that if you fall back to read() (or pread) instead of mmap, 
> the 32MB thing is way too big.
>
> So maybe you should make the default depend on NO_MMAP (although it would 
> seem that the default Makefile makes Cygwin actually default to using mmap 
> these days, so maybe it's not a big deal).

I have been thinking that we should perhaps change Cygwin default to NO_MMAP. Safer but slower would be better than not working correctly for people on FAT32.

I agree that 32MB is too big for emulated mmap(). We might want to further enhance the new use_pack() API so that the caller can say how much it expects to consume, to help pread() based emulation avoid reading unnecessary data.

Also the patch makes the maximum mapped from (1<<26) bytes to 256MB which is four-fold; I think tweaking such settings should be done as a separate step.