From 225ea22046a1193bd934a8ea308fa4a7788c9796 Mon Sep 17 00:00:00 2001 From: Ramsay Jones Date: Sun, 13 Jul 2014 02:41:41 -0400 Subject: alloc.c: remove the alloc_raw_commit_node() function In order to encapsulate the setting of the unique commit index, commit 969eba63 ("commit: push commit_index update into alloc_commit_node", 10-06-2014) introduced a (logically private) intermediary allocator function. However, this function (alloc_raw_commit_node()) was declared as a public function, which undermines its entire purpose. Introduce an inline function, alloc_node(), which implements the main logic of the allocator used by DEFINE_ALLOCATOR, and redefine the macro in terms of the new function. In addition, use the new function in the implementation of the alloc_commit_node() allocator, rather than the intermediary allocator, which can now be removed. Noticed by sparse ("symbol 'alloc_raw_commit_node' was not declared. Should it be static?"). Signed-off-by: Ramsay Jones Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- alloc.c | 47 +++++++++++++++++++++++++++++------------------ 1 file changed, 29 insertions(+), 18 deletions(-) (limited to 'alloc.c') diff --git a/alloc.c b/alloc.c index eb22a45c9d..d7c3605fd6 100644 --- a/alloc.c +++ b/alloc.c @@ -19,22 +19,10 @@ #define BLOCKING 1024 #define DEFINE_ALLOCATOR(name, type) \ -static unsigned int name##_allocs; \ +static struct alloc_state name##_state; \ void *alloc_##name##_node(void) \ { \ - static int nr; \ - static type *block; \ - void *ret; \ - \ - if (!nr) { \ - nr = BLOCKING; \ - block = xmalloc(BLOCKING * sizeof(type)); \ - } \ - nr--; \ - name##_allocs++; \ - ret = block++; \ - memset(ret, 0, sizeof(type)); \ - return ret; \ + return alloc_node(&name##_state, sizeof(type)); \ } union any_object { @@ -45,16 +33,39 @@ union any_object { struct tag tag; }; +struct alloc_state { + int count; /* total number of nodes allocated */ + int nr; /* number of nodes left in current allocation */ + void *p; /* first free node in current allocation */ +}; + +static inline void *alloc_node(struct alloc_state *s, size_t node_size) +{ + void *ret; + + if (!s->nr) { + s->nr = BLOCKING; + s->p = xmalloc(BLOCKING * node_size); + } + s->nr--; + s->count++; + ret = s->p; + s->p = (char *)s->p + node_size; + memset(ret, 0, node_size); + return ret; +} + DEFINE_ALLOCATOR(blob, struct blob) DEFINE_ALLOCATOR(tree, struct tree) -DEFINE_ALLOCATOR(raw_commit, struct commit) DEFINE_ALLOCATOR(tag, struct tag) DEFINE_ALLOCATOR(object, union any_object) +static struct alloc_state commit_state; + void *alloc_commit_node(void) { static int commit_count; - struct commit *c = alloc_raw_commit_node(); + struct commit *c = alloc_node(&commit_state, sizeof(struct commit)); c->index = commit_count++; return c; } @@ -66,13 +77,13 @@ static void report(const char *name, unsigned int count, size_t size) } #define REPORT(name, type) \ - report(#name, name##_allocs, name##_allocs * sizeof(type) >> 10) + report(#name, name##_state.count, name##_state.count * sizeof(type) >> 10) void alloc_report(void) { REPORT(blob, struct blob); REPORT(tree, struct tree); - REPORT(raw_commit, struct commit); + REPORT(commit, struct commit); REPORT(tag, struct tag); REPORT(object, union any_object); } -- cgit v1.2.1 From 600e2a69df6dfde134aba0ca88cfe1bd2c88ecbb Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sun, 13 Jul 2014 02:41:51 -0400 Subject: alloc: write out allocator definitions Because the allocator functions for tree, blobs, etc are all very similar, we originally used a macro to avoid repeating ourselves. Since the prior commit, though, the heavy lifting is done by an inline helper function. The macro does still save us a few lines, but at some readability cost. It obfuscates the function definitions (and makes them hard to find via grep). Much worse, though, is the fact that it isn't used consistently for all allocators. Somebody coming later may be tempted to modify DEFINE_ALLOCATOR, but they would miss alloc_commit_node, which is treated specially. Let's just drop the macro and write everything out explicitly. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- alloc.c | 38 +++++++++++++++++++++++++++----------- 1 file changed, 27 insertions(+), 11 deletions(-) (limited to 'alloc.c') diff --git a/alloc.c b/alloc.c index d7c3605fd6..03e458bad0 100644 --- a/alloc.c +++ b/alloc.c @@ -18,13 +18,6 @@ #define BLOCKING 1024 -#define DEFINE_ALLOCATOR(name, type) \ -static struct alloc_state name##_state; \ -void *alloc_##name##_node(void) \ -{ \ - return alloc_node(&name##_state, sizeof(type)); \ -} - union any_object { struct object object; struct blob blob; @@ -55,10 +48,33 @@ static inline void *alloc_node(struct alloc_state *s, size_t node_size) return ret; } -DEFINE_ALLOCATOR(blob, struct blob) -DEFINE_ALLOCATOR(tree, struct tree) -DEFINE_ALLOCATOR(tag, struct tag) -DEFINE_ALLOCATOR(object, union any_object) +static struct alloc_state blob_state; +void *alloc_blob_node(void) +{ + struct blob *b = alloc_node(&blob_state, sizeof(struct blob)); + return b; +} + +static struct alloc_state tree_state; +void *alloc_tree_node(void) +{ + struct tree *t = alloc_node(&tree_state, sizeof(struct tree)); + return t; +} + +static struct alloc_state tag_state; +void *alloc_tag_node(void) +{ + struct tag *t = alloc_node(&tag_state, sizeof(struct tag)); + return t; +} + +static struct alloc_state object_state; +void *alloc_object_node(void) +{ + struct object *obj = alloc_node(&object_state, sizeof(union any_object)); + return obj; +} static struct alloc_state commit_state; -- cgit v1.2.1 From d36f51c13b54a872cdaf08a1765a23afab26ae51 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sun, 13 Jul 2014 02:41:55 -0400 Subject: move setting of object->type to alloc_* functions The "struct object" type implements basic object polymorphism. Individual instances are allocated as concrete types (or as a union type that can store any object), and a "struct object *" can be cast into its real type after examining its "type" enum. This means it is dangerous to have a type field that does not match the allocation (e.g., setting the type field of a "struct blob" to "OBJ_COMMIT" would mean that a reader might read past the allocated memory). In most of the current code this is not a problem; the first thing we do after allocating an object is usually to set its type field by passing it to create_object. However, the virtual commits we create in merge-recursive.c do not ever get their type set. This does not seem to have caused problems in practice, though (presumably because we always pass around a "struct commit" pointer and never even look at the type). We can fix this oversight and also make it harder for future code to get it wrong by setting the type directly in the object allocation functions. This will also make it easier to fix problems with commit index allocation, as we know that any object allocated by alloc_commit_node will meet the invariant that an object with an OBJ_COMMIT type field will have a unique index number. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- alloc.c | 5 +++++ 1 file changed, 5 insertions(+) (limited to 'alloc.c') diff --git a/alloc.c b/alloc.c index 03e458bad0..fd2e32df8c 100644 --- a/alloc.c +++ b/alloc.c @@ -52,6 +52,7 @@ static struct alloc_state blob_state; void *alloc_blob_node(void) { struct blob *b = alloc_node(&blob_state, sizeof(struct blob)); + b->object.type = OBJ_BLOB; return b; } @@ -59,6 +60,7 @@ static struct alloc_state tree_state; void *alloc_tree_node(void) { struct tree *t = alloc_node(&tree_state, sizeof(struct tree)); + t->object.type = OBJ_TREE; return t; } @@ -66,6 +68,7 @@ static struct alloc_state tag_state; void *alloc_tag_node(void) { struct tag *t = alloc_node(&tag_state, sizeof(struct tag)); + t->object.type = OBJ_TAG; return t; } @@ -73,6 +76,7 @@ static struct alloc_state object_state; void *alloc_object_node(void) { struct object *obj = alloc_node(&object_state, sizeof(union any_object)); + obj->type = OBJ_NONE; return obj; } @@ -82,6 +86,7 @@ void *alloc_commit_node(void) { static int commit_count; struct commit *c = alloc_node(&commit_state, sizeof(struct commit)); + c->object.type = OBJ_COMMIT; c->index = commit_count++; return c; } -- cgit v1.2.1 From 94d5a22cf651174f91d40a140593628a04fdacf3 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sun, 13 Jul 2014 02:42:08 -0400 Subject: alloc: factor out commit index We keep a static counter to set the commit index on newly allocated objects. However, since we also need to set the index on any_objects which are converted to commits, let's make the counter available as a public function. While we're moving it, let's make sure the counter is allocated as an unsigned integer to match the index field in "struct commit". Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- alloc.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) (limited to 'alloc.c') diff --git a/alloc.c b/alloc.c index fd2e32df8c..12afadfacd 100644 --- a/alloc.c +++ b/alloc.c @@ -82,12 +82,17 @@ void *alloc_object_node(void) static struct alloc_state commit_state; +unsigned int alloc_commit_index(void) +{ + static unsigned int count; + return count++; +} + void *alloc_commit_node(void) { - static int commit_count; struct commit *c = alloc_node(&commit_state, sizeof(struct commit)); c->object.type = OBJ_COMMIT; - c->index = commit_count++; + c->index = alloc_commit_index(); return c; } -- cgit v1.2.1