Re: [PATCH 3/8] Add memory pool library

[Ramkumar Ramachandra <artagnon@gmail.com>]

Hi Jonathan,

Jonathan Nieder writes:
> Ramkumar Ramachandra wrote:
> 
> > void pre_commit(void);
> > 
> > 	Write the pool to file.
> 
> Except as a proof of concept, this is the wrong API to have.  The problem
> is that the caller cannot choose the filename, so it ends up being a .bin
> file in the current directory, wherever that is.
> 
> The log message leaves out a subtlety: this also increases the
> ‘committed’ value, and bookkeeping for that might be useful to some
> callers.
> 
> In other words:
> 
> > +static MAYBE_UNUSED void pre##_init(void) \
> > +{ \
> > +	struct stat st; \
> > +	pre##_pool.file = fopen(#pre ".bin", "a+"); \
> > +	rewind(pre##_pool.file); \
> > +	fstat(fileno(pre##_pool.file), &st); \
> > +	pre##_pool.size = st.st_size / sizeof(obj_t); \
> > +	pre##_pool.committed = pre##_pool.size; \
> > +	pre##_pool.capacity = pre##_pool.size * 2; \
> > +	if (pre##_pool.capacity < initial_capacity) \
> > +		pre##_pool.capacity = initial_capacity; \
> > +	pre##_pool.base = malloc(pre##_pool.capacity * sizeof(obj_t)); \
> > +	fread(pre##_pool.base, sizeof(obj_t), pre##_pool.size, pre##_pool.file); \
> > +} \
> 
> If you just want something working, I’d suggest stubbing this out:
> 
>  static MAYBE_UNUSED void pre##_init(void) \
>  { \
>  } \
> 
> It even almost makes sense as API: the _init function does all
> initialization tasks required, which is to say, none.  (The {0, ...}
> initializer already has taken care of setting all fields to 0).
> 
> > +static MAYBE_UNUSED void pre##_commit(void) \
> > +{ \
> > +	pre##_pool.committed += fwrite(pre##_pool.base + pre##_pool.committed, \
> > +		sizeof(obj_t), pre##_pool.size - pre##_pool.committed, \
> > +		pre##_pool.file); \
> > +} \
> 
> This can be simplified
> 
>  static MAYBE_UNUSED void pre##_commit(void) \
>  { \
> 	pre##_pool.committed = pre##_pool.size; \
>  } \
> 
> In other words, maybe something like this on top?  This includes the
> vestigal _init() function which really should not be there (it is
> confusing that some callers use it and others don’t).  I did not
> spend much time on it because in the end I suspect we might throw
> obj_pool away anyway.

Oh, right. I remember that you asked to turn off persistence for this
merge. We can include persistence it in a later series.

Junio: Could you squash this diff into the commit?

diff --git a/vcs-svn/obj_pool.h b/vcs-svn/obj_pool.h
index f60c872..7a256d4 100644
--- a/vcs-svn/obj_pool.h
+++ b/vcs-svn/obj_pool.h
@@ -20,17 +20,6 @@ static struct { \
 } pre##_pool = { 0, 0, 0, NULL, NULL}; \
 static MAYBE_UNUSED void pre##_init(void) \
 { \
-	struct stat st; \
-	pre##_pool.file = fopen(#pre ".bin", "a+"); \
-	rewind(pre##_pool.file); \
-	fstat(fileno(pre##_pool.file), &st); \
-	pre##_pool.size = st.st_size / sizeof(obj_t); \
-	pre##_pool.committed = pre##_pool.size; \
-	pre##_pool.capacity = pre##_pool.size * 2; \
-	if (pre##_pool.capacity < initial_capacity) \
-		pre##_pool.capacity = initial_capacity; \
-	pre##_pool.base = malloc(pre##_pool.capacity * sizeof(obj_t)); \
-	fread(pre##_pool.base, sizeof(obj_t), pre##_pool.size, pre##_pool.file); \
 } \
 static MAYBE_UNUSED uint32_t pre##_alloc(uint32_t count) \
 { \
@@ -62,9 +51,7 @@ static MAYBE_UNUSED obj_t *pre##_pointer(uint32_t offset) \
 } \
 static MAYBE_UNUSED void pre##_commit(void) \
 { \
-	pre##_pool.committed += fwrite(pre##_pool.base + pre##_pool.committed, \
-		sizeof(obj_t), pre##_pool.size - pre##_pool.committed, \
-		pre##_pool.file); \
+	pre##_pool.committed = pre##_pool.size; \
 } \
 static MAYBE_UNUSED void pre##_reset(void) \
 { \