Re: [PATCH 3/8] Add memory pool library
- From
Jonathan Nieder <jrnieder@gmail.com>
- Date
- Jul 15, 2010, 18:57 UTC
- Message-ID
- <20100715185706.GA2774@burratino>
- In-Reply-To
- <1279210984-31604-4-git-send-email-artagnon@gmail.com>
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:
Show 14 quoted lines
> +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).
Show 6 quoted lines
> +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.
---
diff --git a/Makefile b/Makefile index fc31ee0..386a586 100644 --- a/Makefile +++ b/Makefile @@ -409,6 +409,7 @@ TEST_PROGRAMS_NEED_X += test-delta TEST_PROGRAMS_NEED_X += test-dump-cache-tree TEST_PROGRAMS_NEED_X += test-genrandom TEST_PROGRAMS_NEED_X += test-match-trees +TEST_PROGRAMS_NEED_X += test-obj-pool TEST_PROGRAMS_NEED_X += test-parse-options TEST_PROGRAMS_NEED_X += test-path-utils TEST_PROGRAMS_NEED_X += test-run-command diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh index 680d7d6..262f304 100755 --- a/t/t0070-fundamental.sh +++ b/t/t0070-fundamental.sh @@ -12,4 +12,10 @@ test_expect_success 'character classes (isspace, isalpha etc.)' ' test-ctype ' +test_expect_success 'allocator for svn importer' ' + printf "%s\n" 0 0 0 0 0 2 2 2 2 2 -1 >expected && + test-obj-pool >actual && + test_cmp expected actual +' + test_done diff --git a/test-obj-pool.c b/test-obj-pool.c new file mode 100644 index 0000000..1300049 --- /dev/null +++ b/test-obj-pool.c @@ -0,0 +1,55 @@ +/* + * test-obj-pool.c: code to exercise the svn importer's object pool + */ + +#include "vcs-svn/obj_pool.h" + +static const char usage_str[] = + "test-obj-pool"; + +obj_pool_gen(int, int, 2) +obj_pool_gen(other, int, 4096) + +int main(int argc, char *argv[]) +{ + int *p; + int i; + + if (argc != 1) { + fprintf(stderr, "Usage: %s\n", usage_str); + return 1; + } + + int_init(); + + p = int_pointer(int_alloc(10)); + for (i = 0; i < 10; i++) + p[i] = 0; + + *other_pointer(other_alloc(1)) = -1; + other_commit(); + + p = other_pointer(other_alloc(10)); + for (i = 0; i < 10; i++) + p[i] = 1; + + int_free(5); + + p = int_pointer(int_alloc(10)); + for (i = 0; i < 10; i++) + p[i] = 2; + + int_free(5); + int_commit(); + + for (i = 0; i < int_pool.committed; i++) + printf("%d\n", *int_pointer(i)); + + for (i = 0; i < other_pool.committed; i++) + printf("%d\n", *other_pointer(i)); + + int_reset(); + int_reset(); + other_reset(); + return 0; +} diff --git a/vcs-svn/obj_pool.h b/vcs-svn/obj_pool.h index f60c872..90eda15 100644 --- a/vcs-svn/obj_pool.h +++ b/vcs-svn/obj_pool.h @@ -16,21 +16,9 @@ static struct { \ uint32_t size; \ uint32_t capacity; \ obj_t *base; \ - FILE *file; \ -} pre##_pool = { 0, 0, 0, NULL, NULL}; \ +} pre##_pool = {0, 0, 0, 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,19 +50,15 @@ 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) \ { \ free(pre##_pool.base); \ - if (pre##_pool.file) \ - fclose(pre##_pool.file); \ pre##_pool.base = NULL; \ pre##_pool.size = 0; \ pre##_pool.capacity = 0; \ - pre##_pool.file = NULL; \ + pre##_pool.committed = 0; \ } #endif