Re: [PATCH 1/6] Add memory pool library

From: Ramkumar Ramachandra <artagnon@gmail.com>
Date: Jun 7, 2010, 13:28 UTC
Message-ID: <AANLkTimzuJXjX4OJUkmQtLwjGDgHBaZUHSGA0xUUi6kY@mail.gmail.com>
In-Reply-To: <20100604182928.GB20909@progeny.tock>

Hi Jonathan,

Jonathan Nieder wrote:

Show 15 quoted lines

>  The memory pool library is distinguished from the existing specialized
>  allocators in alloc.c by using a contiguous block for all allocations.
>  This means that on one hand, long-lived pointers have to be written as
>  offsets, since the base address changes as the pool grows, but on the
>  other hand, the entire pool can be easily written to the file system.
>  This allows the memory pool to persist between runs of an application.
>
>  For svn-fe, such a facility is useful because each svn revision can
>  copy trees and files from any previous revision.  Therefore the
>  relevant information for all revisions has to persist somehow to
>  support incremental runs.
>
>  The current implementation is backed by the file system using mmap().
>
> Thanks for the explanations, David.

Thanks. Added to commit message: should be fine the next time I post the series.

> Whitespace damaged.

Fixed in b180ad7 (pushed few seconds ago).

> Probably an ignorant question, but why?  I do not think the win32 mmap
> emulation in git currently supports sysconf().

I'm not sure why the pool capacity should be dependent on the page size.

>        ..._pool.file = open(..., O_RDWR);

Fixed in b180ad7 (pushed few seconds ago).

> This is the first use of MAP_SHARED in git.  I wouldn’t be surprised if
> the win32 mmap emulation does not support it.

You're right; I just checked. compat/mmap.c:7 clearly specifies that
only MAP_PRIVATE is supported.
	if (start != NULL || !(flags & MAP_PRIVATE))
		die("Invalid usage of mmap when built with NO_MMAP");

What can we do about this? Should we attempt to implement MAP_SHARED?

> Necessary?  (Maybe yes: we are about to truncate the file, so the
> fsync() may be intended to force the data to be committed before
> the metadata.)  Portable?  (I suspect the fsync() should go after
> the munmap().)

You want to flush changes *after* unmapping the pages of memory from the file? I can't find any notes on portability relating to this- can you clarify?

> Because of these portability concerns, I’d rather use the old version
> until the incremental parser is ready.

Rolling back that many changes can be more painful than actually fixing these few concerns (especially large number of change after `dirents` was merged in). It means more delay in getting the exporter merged.

-- Ram