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

[Jonathan Nieder <jrnieder@gmail.com>]

Ramkumar Ramachandra wrote:

> Add a memory pool library implemented using cpp macros. The library
> provides macros that can be used to create a type-specific memory pool
> API.

More details:

  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.

> +#define obj_pool_gen(pre, obj_t, initial_capacity) \
> +static struct { \
> +	uint32_t size; \
> +	uint32_t capacity; \
> +	obj_t *base; \
> +        FILE *file; \

Whitespace damaged.

> +} pre##_pool = { 0, 0, NULL, NULL}; \
> +static void pre##_init(void) \
> +{ \
> +	struct stat st; \
> +	size_t ps = sysconf (_SC_PAGESIZE); \

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

> +	/* Touch binary file before opening read/write */ \
> +	pre##_pool.file = fopen(#pre ".bin", "a"); \
> +	fclose(pre##_pool.file); \
> +	/* Open, check size, compute capacity */ \
> +	pre##_pool.file = fopen(#pre ".bin", "r+"); \

Maybe

	..._pool.file = fopen(..., "a+");
	rewind(..._pool.file);

would be simpler.  But given that there is no buffered I/O going on
for this file, why not just

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

?

> +	/* Truncate to calculated capacity and map to VM */ \
> +	ftruncate(fileno(pre##_pool.file), pre##_pool.capacity * sizeof(obj_t)); \

Reasonable.

> +	pre##_pool.base = mmap(0, pre##_pool.capacity * sizeof(obj_t), \
> +				PROT_READ | PROT_WRITE, MAP_SHARED, \
> +				fileno(pre##_pool.file), 0); \

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

> +} \
> +static uint32_t pre##_alloc(uint32_t count) \
> +{ \
> +	uint32_t offset; \
> +	if (pre##_pool.size + count > pre##_pool.capacity) { \
> +		if (NULL == pre##_pool.base) \
> +			pre##_init(); \
> +		fsync(fileno(pre##_pool.file)); \

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().)

[...]
> +static void pre##_reset(void) \
> +{ \
> +	if (pre##_pool.base) { \
> +		fsync(fileno(pre##_pool.file)); \

Same question.

> +		munmap(pre##_pool.base, \
> +			pre##_pool.capacity * sizeof(obj_t)); \
> +		ftruncate(fileno(pre##_pool.file), \
> +				pre##_pool.size * sizeof(obj_t)); \
> +		fclose(pre##_pool.file); \

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

Hope that helps,
Jonathan