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

Hi Jonathan,

Jonathan Nieder wrote:

 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