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

Hi Jonathan,

Jonathan Nieder writes:

Ramkumar Ramachandra wrote:

quoted

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:

quoted

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

quoted

+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) \
 { \