{"thread":{"id":"46319","subject":"[RFC/WIP PATCH] object store classification","startedAt":"2017-07-06T20:27:49Z","lastAt":"2017-07-11T20:28:06Z","messageCount":9,"participants":["Stefan Beller","Junio C Hamano","Ben Peart","Brandon Williams"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"323914","messageId":"20170706202739.6056-1-sbeller@google.com","threadId":"46319","inReplyTo":null,"subject":"[RFC/WIP PATCH] object store classification","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-07-06T20:27:39Z","receivedAt":"2017-07-06T20:27:49Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This continues the efforts of bw/repo-object, making our code base\nmore object oriented by adding the object store to the the repository struct.\n\nLong term goal of the series that would evolve from this discussion:\n* Easier to implement submodule features as it can be in the same process.\n\nShort term goal:\n* get rid of 'add_submodule_odb' in submodule.c\n  When fetching or pushing we need to determine if a submodule needs processing\n  by finding out if certain commits are present.  To do this we add the submodule\n  objects as an alternate object database and do the processing in the same\n  process. In case of multiple submodules, this would pollute the object store\n  hence being slower and increasing memory usage, too.\n\nThis patch only changes the object store code to be object oriented based\non the repository struct.\n\nTo go for the short term goal we'd need to convert a few more places, mostly\nall the construction (blob.c, tree.c, commit.c)\n\nThis is marked RFC as I'd want to gather feedback if the approach, presented\nin the header files is sound.\n\nThanks!\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n\n  Peff, this area of the code seems performance sensitive for a variety of\n  use cases, specifically yours. So I cc'd you here. :)\n\n object.c     | 77 +++++++++++++++++++++++++++++++++++-------------------------\n object.h     | 51 ++++++++++++++++++++++++++++++++--------\n repository.h |  6 +++++\n 3 files changed, 92 insertions(+), 42 deletions(-)\n\ndiff --git a/object.c b/object.c\nindex 06ba3a11d8..b5ec0bb2f9 100644\n--- a/object.c\n+++ b/object.c\n@@ -5,17 +5,15 @@\n #include \"commit.h\"\n #include \"tag.h\"\n \n-static struct object **obj_hash;\n-static int nr_objs, obj_hash_size;\n-\n-unsigned int get_max_object_index(void)\n+unsigned int object_store_get_max_index(struct repository *r)\n {\n-\treturn obj_hash_size;\n+\treturn r->objects.obj_hash_size;\n }\n \n-struct object *get_indexed_object(unsigned int idx)\n+struct object *object_store_get_indexed(struct repository *r,\n+\t\t\t\t\tunsigned int idx)\n {\n-\treturn obj_hash[idx];\n+\treturn r->objects.obj_hash[idx];\n }\n \n static const char *object_type_strings[] = {\n@@ -82,11 +80,15 @@ static void insert_obj_hash(struct object *obj, struct object **hash, unsigned i\n  * Look up the record for the given sha1 in the hash map stored in\n  * obj_hash.  Return NULL if it was not found.\n  */\n-struct object *lookup_object(const unsigned char *sha1)\n+struct object *object_store_lookup(struct repository *r,\n+\t\t\t\t   const unsigned char *sha1)\n {\n \tunsigned int i, first;\n \tstruct object *obj;\n \n+\tstruct object **obj_hash = r->objects.obj_hash;\n+\tint obj_hash_size = r->objects.obj_hash_size;\n+\n \tif (!obj_hash)\n \t\treturn NULL;\n \n@@ -114,29 +116,31 @@ struct object *lookup_object(const unsigned char *sha1)\n  * power of 2 (but at least 32).  Copy the existing values to the new\n  * hash map.\n  */\n-static void grow_object_hash(void)\n+static void grow_object_hash(struct repository *r)\n {\n \tint i;\n \t/*\n \t * Note that this size must always be power-of-2 to match hash_obj\n \t * above.\n \t */\n-\tint new_hash_size = obj_hash_size < 32 ? 32 : 2 * obj_hash_size;\n+\tint new_hash_size = r->objects.obj_hash_size < 32 ?\n+\t\t\t32 : 2 * r->objects.obj_hash_size;\n \tstruct object **new_hash;\n \n \tnew_hash = xcalloc(new_hash_size, sizeof(struct object *));\n-\tfor (i = 0; i < obj_hash_size; i++) {\n-\t\tstruct object *obj = obj_hash[i];\n+\tfor (i = 0; i < r->objects.obj_hash_size; i++) {\n+\t\tstruct object *obj = r->objects.obj_hash[i];\n \t\tif (!obj)\n \t\t\tcontinue;\n \t\tinsert_obj_hash(obj, new_hash, new_hash_size);\n \t}\n-\tfree(obj_hash);\n-\tobj_hash = new_hash;\n-\tobj_hash_size = new_hash_size;\n+\tfree(r->objects.obj_hash);\n+\tr->objects.obj_hash = new_hash;\n+\tr->objects.obj_hash_size = new_hash_size;\n }\n \n-void *create_object(const unsigned char *sha1, void *o)\n+void *object_store_create(struct repository *r,\n+\t\t\t  const unsigned char *sha1, void *o)\n {\n \tstruct object *obj = o;\n \n@@ -145,15 +149,17 @@ void *create_object(const unsigned char *sha1, void *o)\n \tobj->flags = 0;\n \thashcpy(obj->oid.hash, sha1);\n \n-\tif (obj_hash_size - 1 <= nr_objs * 2)\n-\t\tgrow_object_hash();\n+\tif (r->objects.obj_hash_size - 1 <= r->objects.nr_objs * 2)\n+\t\tgrow_object_hash(r);\n \n-\tinsert_obj_hash(obj, obj_hash, obj_hash_size);\n-\tnr_objs++;\n+\tinsert_obj_hash(obj, r->objects.obj_hash, r->objects.obj_hash_size);\n+\tr->objects.nr_objs++;\n \treturn obj;\n }\n \n-void *object_as_type(struct object *obj, enum object_type type, int quiet)\n+void *object_as_type(struct object *obj,\n+\t\t     enum object_type type,\n+\t\t     int quiet)\n {\n \tif (obj->type == type)\n \t\treturn obj;\n@@ -172,15 +178,20 @@ void *object_as_type(struct object *obj, enum object_type type, int quiet)\n \t}\n }\n \n-struct object *lookup_unknown_object(const unsigned char *sha1)\n+struct object *object_store_lookup_unknown_object(struct repository *r,\n+\t\t\t\t\t\t  const unsigned char *sha1)\n {\n-\tstruct object *obj = lookup_object(sha1);\n+\tstruct object *obj = object_store_lookup(r, sha1);\n \tif (!obj)\n-\t\tobj = create_object(sha1, alloc_object_node());\n+\t\tobj = object_store_create(r, sha1, alloc_object_node());\n \treturn obj;\n }\n \n-struct object *parse_object_buffer(const struct object_id *oid, enum object_type type, unsigned long size, void *buffer, int *eaten_p)\n+struct object *object_store_parse_object_buffer(struct repository *r,\n+\t\t\t\t\t\tconst struct object_id *oid,\n+\t\t\t\t\t\tenum object_type type,\n+\t\t\t\t\t\tunsigned long size,\n+\t\t\t\t\t\tvoid *buffer, int *eaten_p)\n {\n \tstruct object *obj;\n \t*eaten_p = 0;\n@@ -230,17 +241,19 @@ struct object *parse_object_buffer(const struct object_id *oid, enum object_type\n \treturn obj;\n }\n \n-struct object *parse_object_or_die(const struct object_id *oid,\n-\t\t\t\t   const char *name)\n+struct object *object_store_parse_object_or_die(struct repository *r,\n+\t\t\t\t\t\tconst struct object_id *oid,\n+\t\t\t\t\t\tconst char *name)\n {\n-\tstruct object *o = parse_object(oid);\n+\tstruct object *o = object_store_parse_object(r, oid);\n \tif (o)\n \t\treturn o;\n \n \tdie(_(\"unable to parse object: %s\"), name ? name : oid_to_hex(oid));\n }\n \n-struct object *parse_object(const struct object_id *oid)\n+struct object *object_store_parse_object(struct repository *r,\n+\t\t\t\t\t const struct object_id *oid)\n {\n \tunsigned long size;\n \tenum object_type type;\n@@ -413,12 +426,12 @@ void object_array_remove_duplicates(struct object_array *array)\n \t}\n }\n \n-void clear_object_flags(unsigned flags)\n+void object_store_clear_object_flags(struct repository *r, unsigned flags)\n {\n \tint i;\n \n-\tfor (i=0; i < obj_hash_size; i++) {\n-\t\tstruct object *obj = obj_hash[i];\n+\tfor (i = 0; i < r->objects.obj_hash_size; i++) {\n+\t\tstruct object *obj = r->objects.obj_hash[i];\n \t\tif (obj)\n \t\t\tobj->flags &= ~flags;\n \t}\ndiff --git a/object.h b/object.h\nindex 33e5cc9943..1b2ab8ee88 100644\n--- a/object.h\n+++ b/object.h\n@@ -1,6 +1,8 @@\n #ifndef OBJECT_H\n #define OBJECT_H\n \n+#include \"repository.h\"\n+\n struct object_list {\n \tstruct object *item;\n \tstruct object_list *next;\n@@ -59,12 +61,15 @@ extern int type_from_string_gently(const char *str, ssize_t, int gentle);\n /*\n  * Return the current number of buckets in the object hashmap.\n  */\n-extern unsigned int get_max_object_index(void);\n+extern unsigned int object_store_get_max_index(struct repository *r);\n+#define get_max_object_index() object_store_get_max_index(the_repository)\n \n /*\n  * Return the object from the specified bucket in the object hashmap.\n  */\n-extern struct object *get_indexed_object(unsigned int);\n+extern struct object *object_store_get_indexed(struct repository *r,\n+\t\t\t\t\t       unsigned int);\n+#define get_indexed_object(i) object_store_get_indexed(the_repository, i)\n \n /*\n  * This can be used to see if we have heard of the object before, but\n@@ -78,34 +83,59 @@ extern struct object *get_indexed_object(unsigned int);\n  * half-initialised objects, the caller is expected to initialize them\n  * by calling parse_object() on them.\n  */\n-struct object *lookup_object(const unsigned char *sha1);\n+extern struct object *object_store_lookup(struct repository *r,\n+\t\t\t\t\t  const unsigned char *sha1);\n+#define lookup_object(hash) object_store_lookup(the_repository, hash)\n \n-extern void *create_object(const unsigned char *sha1, void *obj);\n+extern void *object_store_create(struct repository *r,\n+\t\t\t\t const unsigned char *sha1,\n+\t\t\t\t void *obj);\n+#define create_object(sha1, obj) object_store_create(the_repository, sha1, obj)\n \n-void *object_as_type(struct object *obj, enum object_type type, int quiet);\n+extern void *object_as_type(struct object *obj,\n+\t\t\t    enum object_type type,\n+\t\t\t    int quiet);\n \n /*\n  * Returns the object, having parsed it to find out what it is.\n  *\n  * Returns NULL if the object is missing or corrupt.\n  */\n-struct object *parse_object(const struct object_id *oid);\n+extern struct object *object_store_parse_object(struct repository *r,\n+\t\t\t\t\t\tconst struct object_id *oid);\n+#define parse_object(sha1) \\\n+\tobject_store_parse_object(the_repository, sha1)\n \n /*\n  * Like parse_object, but will die() instead of returning NULL. If the\n  * \"name\" parameter is not NULL, it is included in the error message\n  * (otherwise, the hex object ID is given).\n  */\n-struct object *parse_object_or_die(const struct object_id *oid, const char *name);\n+extern struct object *object_store_parse_object_or_die(struct repository *r,\n+\t\t\t\t\t\t       const struct object_id *oid,\n+\t\t\t\t\t\t       const char *name);\n+#define parse_object_or_die(oid, name) \\\n+\tobject_store_parse_object_or_die(the_repository, oid, name)\n \n /* Given the result of read_sha1_file(), returns the object after\n  * parsing it.  eaten_p indicates if the object has a borrowed copy\n  * of buffer and the caller should not free() it.\n  */\n-struct object *parse_object_buffer(const struct object_id *oid, enum object_type type, unsigned long size, void *buffer, int *eaten_p);\n+struct object *object_store_parse_object_buffer(struct repository *r,\n+\t\t\t\t\t\tconst struct object_id *oid,\n+\t\t\t\t\t\tenum object_type type,\n+\t\t\t\t\t\tunsigned long size,\n+\t\t\t\t\t\tvoid *buffer,\n+\t\t\t\t\t\tint *eaten_p);\n+#define parse_object_buffer(oid, type, size, buffer, eaten_p) \\\n+\tobject_store_parse_object_buffer(the_repository, oid, type, \\\n+\t\t\t\t\t size, buffer, eaten_p)\n \n /** Returns the object, with potentially excess memory allocated. **/\n-struct object *lookup_unknown_object(const unsigned  char *sha1);\n+struct object *object_store_lookup_unknown_object(struct repository *r,\n+\t\t\t\t\t\t  const unsigned char *sha1);\n+#define lookup_unknown_object(sha1) \\\n+\tobject_store_lookup_unknown_object(the_repository, sha1)\n \n struct object_list *object_list_insert(struct object *item,\n \t\t\t\t       struct object_list **list_p);\n@@ -138,6 +168,7 @@ void object_array_remove_duplicates(struct object_array *array);\n  */\n void object_array_clear(struct object_array *array);\n \n-void clear_object_flags(unsigned flags);\n+extern void object_store_clear_object_flags(struct repository *r, unsigned flags);\n+#define clear_object_flags(flags) object_store_clear_object_flags(the_repository, flags);\n \n #endif /* OBJECT_H */\ndiff --git a/repository.h b/repository.h\nindex 417787f3ef..aeb50a19bc 100644\n--- a/repository.h\n+++ b/repository.h\n@@ -1,6 +1,8 @@\n #ifndef REPOSITORY_H\n #define REPOSITORY_H\n \n+#include \"object.h\"\n+\n struct config_set;\n struct index_state;\n struct submodule_cache;\n@@ -24,6 +26,10 @@ struct repository {\n \t * Cannot be NULL after initialization.\n \t */\n \tchar *objectdir;\n+\tstruct object_store {\n+\t\tstruct object **obj_hash;\n+\t\tint nr_objs, obj_hash_size;\n+\t} objects;\n \n \t/*\n \t * Path to the repository's graft file.\n-- \n2.13.2.559.g3a0a3146e4\n\n"},{"id":"323921","messageId":"xmqq7ezldlhe.fsf@gitster.mtv.corp.google.com","threadId":"46319","inReplyTo":"20170706202739.6056-1-sbeller@google.com","subject":"Re: [RFC/WIP PATCH] object store classification","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-07T02:33:33Z","receivedAt":"2017-07-07T02:33:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> Subject: Re: [RFC/WIP PATCH] object store classification\n\nI thought you are writing different object-store backends and\nclassifying them into many categories (e.g. local, networked,\ntelepathic, etc.)\n\nIt is a logical next step to put per-repository things into\nthe_repository.  I skimmed the patch and the changes smelled sane,\nbut I didn't read it really carefully with fine toothed comb.\n\nThanks.  The remainder kept for reference, but no additional\ncomments in there.\n\n> This continues the efforts of bw/repo-object, making our code base\n> more object oriented by adding the object store to the the repository struct.\n>\n> Long term goal of the series that would evolve from this discussion:\n> * Easier to implement submodule features as it can be in the same process.\n>\n> Short term goal:\n> * get rid of 'add_submodule_odb' in submodule.c\n>   When fetching or pushing we need to determine if a submodule needs processing\n>   by finding out if certain commits are present.  To do this we add the submodule\n>   objects as an alternate object database and do the processing in the same\n>   process. In case of multiple submodules, this would pollute the object store\n>   hence being slower and increasing memory usage, too.\n>\n> This patch only changes the object store code to be object oriented based\n> on the repository struct.\n>\n> To go for the short term goal we'd need to convert a few more places, mostly\n> all the construction (blob.c, tree.c, commit.c)\n>\n> This is marked RFC as I'd want to gather feedback if the approach, presented\n> in the header files is sound.\n>\n> Thanks!\n>\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n>\n>   Peff, this area of the code seems performance sensitive for a variety of\n>   use cases, specifically yours. So I cc'd you here. :)\n>\n>  object.c     | 77 +++++++++++++++++++++++++++++++++++-------------------------\n>  object.h     | 51 ++++++++++++++++++++++++++++++++--------\n>  repository.h |  6 +++++\n>  3 files changed, 92 insertions(+), 42 deletions(-)\n>\n> diff --git a/object.c b/object.c\n> index 06ba3a11d8..b5ec0bb2f9 100644\n> --- a/object.c\n> +++ b/object.c\n> @@ -5,17 +5,15 @@\n>  #include \"commit.h\"\n>  #include \"tag.h\"\n>  \n> -static struct object **obj_hash;\n> -static int nr_objs, obj_hash_size;\n> -\n> -unsigned int get_max_object_index(void)\n> +unsigned int object_store_get_max_index(struct repository *r)\n>  {\n> -\treturn obj_hash_size;\n> +\treturn r->objects.obj_hash_size;\n>  }\n>  \n> -struct object *get_indexed_object(unsigned int idx)\n> +struct object *object_store_get_indexed(struct repository *r,\n> +\t\t\t\t\tunsigned int idx)\n>  {\n> -\treturn obj_hash[idx];\n> +\treturn r->objects.obj_hash[idx];\n>  }\n>  \n>  static const char *object_type_strings[] = {\n> @@ -82,11 +80,15 @@ static void insert_obj_hash(struct object *obj, struct object **hash, unsigned i\n>   * Look up the record for the given sha1 in the hash map stored in\n>   * obj_hash.  Return NULL if it was not found.\n>   */\n> -struct object *lookup_object(const unsigned char *sha1)\n> +struct object *object_store_lookup(struct repository *r,\n> +\t\t\t\t   const unsigned char *sha1)\n>  {\n>  \tunsigned int i, first;\n>  \tstruct object *obj;\n>  \n> +\tstruct object **obj_hash = r->objects.obj_hash;\n> +\tint obj_hash_size = r->objects.obj_hash_size;\n> +\n>  \tif (!obj_hash)\n>  \t\treturn NULL;\n>  \n> @@ -114,29 +116,31 @@ struct object *lookup_object(const unsigned char *sha1)\n>   * power of 2 (but at least 32).  Copy the existing values to the new\n>   * hash map.\n>   */\n> -static void grow_object_hash(void)\n> +static void grow_object_hash(struct repository *r)\n>  {\n>  \tint i;\n>  \t/*\n>  \t * Note that this size must always be power-of-2 to match hash_obj\n>  \t * above.\n>  \t */\n> -\tint new_hash_size = obj_hash_size < 32 ? 32 : 2 * obj_hash_size;\n> +\tint new_hash_size = r->objects.obj_hash_size < 32 ?\n> +\t\t\t32 : 2 * r->objects.obj_hash_size;\n>  \tstruct object **new_hash;\n>  \n>  \tnew_hash = xcalloc(new_hash_size, sizeof(struct object *));\n> -\tfor (i = 0; i < obj_hash_size; i++) {\n> -\t\tstruct object *obj = obj_hash[i];\n> +\tfor (i = 0; i < r->objects.obj_hash_size; i++) {\n> +\t\tstruct object *obj = r->objects.obj_hash[i];\n>  \t\tif (!obj)\n>  \t\t\tcontinue;\n>  \t\tinsert_obj_hash(obj, new_hash, new_hash_size);\n>  \t}\n> -\tfree(obj_hash);\n> -\tobj_hash = new_hash;\n> -\tobj_hash_size = new_hash_size;\n> +\tfree(r->objects.obj_hash);\n> +\tr->objects.obj_hash = new_hash;\n> +\tr->objects.obj_hash_size = new_hash_size;\n>  }\n>  \n> -void *create_object(const unsigned char *sha1, void *o)\n> +void *object_store_create(struct repository *r,\n> +\t\t\t  const unsigned char *sha1, void *o)\n>  {\n>  \tstruct object *obj = o;\n>  \n> @@ -145,15 +149,17 @@ void *create_object(const unsigned char *sha1, void *o)\n>  \tobj->flags = 0;\n>  \thashcpy(obj->oid.hash, sha1);\n>  \n> -\tif (obj_hash_size - 1 <= nr_objs * 2)\n> -\t\tgrow_object_hash();\n> +\tif (r->objects.obj_hash_size - 1 <= r->objects.nr_objs * 2)\n> +\t\tgrow_object_hash(r);\n>  \n> -\tinsert_obj_hash(obj, obj_hash, obj_hash_size);\n> -\tnr_objs++;\n> +\tinsert_obj_hash(obj, r->objects.obj_hash, r->objects.obj_hash_size);\n> +\tr->objects.nr_objs++;\n>  \treturn obj;\n>  }\n>  \n> -void *object_as_type(struct object *obj, enum object_type type, int quiet)\n> +void *object_as_type(struct object *obj,\n> +\t\t     enum object_type type,\n> +\t\t     int quiet)\n>  {\n>  \tif (obj->type == type)\n>  \t\treturn obj;\n> @@ -172,15 +178,20 @@ void *object_as_type(struct object *obj, enum object_type type, int quiet)\n>  \t}\n>  }\n>  \n> -struct object *lookup_unknown_object(const unsigned char *sha1)\n> +struct object *object_store_lookup_unknown_object(struct repository *r,\n> +\t\t\t\t\t\t  const unsigned char *sha1)\n>  {\n> -\tstruct object *obj = lookup_object(sha1);\n> +\tstruct object *obj = object_store_lookup(r, sha1);\n>  \tif (!obj)\n> -\t\tobj = create_object(sha1, alloc_object_node());\n> +\t\tobj = object_store_create(r, sha1, alloc_object_node());\n>  \treturn obj;\n>  }\n>  \n> -struct object *parse_object_buffer(const struct object_id *oid, enum object_type type, unsigned long size, void *buffer, int *eaten_p)\n> +struct object *object_store_parse_object_buffer(struct repository *r,\n> +\t\t\t\t\t\tconst struct object_id *oid,\n> +\t\t\t\t\t\tenum object_type type,\n> +\t\t\t\t\t\tunsigned long size,\n> +\t\t\t\t\t\tvoid *buffer, int *eaten_p)\n>  {\n>  \tstruct object *obj;\n>  \t*eaten_p = 0;\n> @@ -230,17 +241,19 @@ struct object *parse_object_buffer(const struct object_id *oid, enum object_type\n>  \treturn obj;\n>  }\n>  \n> -struct object *parse_object_or_die(const struct object_id *oid,\n> -\t\t\t\t   const char *name)\n> +struct object *object_store_parse_object_or_die(struct repository *r,\n> +\t\t\t\t\t\tconst struct object_id *oid,\n> +\t\t\t\t\t\tconst char *name)\n>  {\n> -\tstruct object *o = parse_object(oid);\n> +\tstruct object *o = object_store_parse_object(r, oid);\n>  \tif (o)\n>  \t\treturn o;\n>  \n>  \tdie(_(\"unable to parse object: %s\"), name ? name : oid_to_hex(oid));\n>  }\n>  \n> -struct object *parse_object(const struct object_id *oid)\n> +struct object *object_store_parse_object(struct repository *r,\n> +\t\t\t\t\t const struct object_id *oid)\n>  {\n>  \tunsigned long size;\n>  \tenum object_type type;\n> @@ -413,12 +426,12 @@ void object_array_remove_duplicates(struct object_array *array)\n>  \t}\n>  }\n>  \n> -void clear_object_flags(unsigned flags)\n> +void object_store_clear_object_flags(struct repository *r, unsigned flags)\n>  {\n>  \tint i;\n>  \n> -\tfor (i=0; i < obj_hash_size; i++) {\n> -\t\tstruct object *obj = obj_hash[i];\n> +\tfor (i = 0; i < r->objects.obj_hash_size; i++) {\n> +\t\tstruct object *obj = r->objects.obj_hash[i];\n>  \t\tif (obj)\n>  \t\t\tobj->flags &= ~flags;\n>  \t}\n> diff --git a/object.h b/object.h\n> index 33e5cc9943..1b2ab8ee88 100644\n> --- a/object.h\n> +++ b/object.h\n> @@ -1,6 +1,8 @@\n>  #ifndef OBJECT_H\n>  #define OBJECT_H\n>  \n> +#include \"repository.h\"\n> +\n>  struct object_list {\n>  \tstruct object *item;\n>  \tstruct object_list *next;\n> @@ -59,12 +61,15 @@ extern int type_from_string_gently(const char *str, ssize_t, int gentle);\n>  /*\n>   * Return the current number of buckets in the object hashmap.\n>   */\n> -extern unsigned int get_max_object_index(void);\n> +extern unsigned int object_store_get_max_index(struct repository *r);\n> +#define get_max_object_index() object_store_get_max_index(the_repository)\n>  \n>  /*\n>   * Return the object from the specified bucket in the object hashmap.\n>   */\n> -extern struct object *get_indexed_object(unsigned int);\n> +extern struct object *object_store_get_indexed(struct repository *r,\n> +\t\t\t\t\t       unsigned int);\n> +#define get_indexed_object(i) object_store_get_indexed(the_repository, i)\n>  \n>  /*\n>   * This can be used to see if we have heard of the object before, but\n> @@ -78,34 +83,59 @@ extern struct object *get_indexed_object(unsigned int);\n>   * half-initialised objects, the caller is expected to initialize them\n>   * by calling parse_object() on them.\n>   */\n> -struct object *lookup_object(const unsigned char *sha1);\n> +extern struct object *object_store_lookup(struct repository *r,\n> +\t\t\t\t\t  const unsigned char *sha1);\n> +#define lookup_object(hash) object_store_lookup(the_repository, hash)\n>  \n> -extern void *create_object(const unsigned char *sha1, void *obj);\n> +extern void *object_store_create(struct repository *r,\n> +\t\t\t\t const unsigned char *sha1,\n> +\t\t\t\t void *obj);\n> +#define create_object(sha1, obj) object_store_create(the_repository, sha1, obj)\n>  \n> -void *object_as_type(struct object *obj, enum object_type type, int quiet);\n> +extern void *object_as_type(struct object *obj,\n> +\t\t\t    enum object_type type,\n> +\t\t\t    int quiet);\n>  \n>  /*\n>   * Returns the object, having parsed it to find out what it is.\n>   *\n>   * Returns NULL if the object is missing or corrupt.\n>   */\n> -struct object *parse_object(const struct object_id *oid);\n> +extern struct object *object_store_parse_object(struct repository *r,\n> +\t\t\t\t\t\tconst struct object_id *oid);\n> +#define parse_object(sha1) \\\n> +\tobject_store_parse_object(the_repository, sha1)\n>  \n>  /*\n>   * Like parse_object, but will die() instead of returning NULL. If the\n>   * \"name\" parameter is not NULL, it is included in the error message\n>   * (otherwise, the hex object ID is given).\n>   */\n> -struct object *parse_object_or_die(const struct object_id *oid, const char *name);\n> +extern struct object *object_store_parse_object_or_die(struct repository *r,\n> +\t\t\t\t\t\t       const struct object_id *oid,\n> +\t\t\t\t\t\t       const char *name);\n> +#define parse_object_or_die(oid, name) \\\n> +\tobject_store_parse_object_or_die(the_repository, oid, name)\n>  \n>  /* Given the result of read_sha1_file(), returns the object after\n>   * parsing it.  eaten_p indicates if the object has a borrowed copy\n>   * of buffer and the caller should not free() it.\n>   */\n> -struct object *parse_object_buffer(const struct object_id *oid, enum object_type type, unsigned long size, void *buffer, int *eaten_p);\n> +struct object *object_store_parse_object_buffer(struct repository *r,\n> +\t\t\t\t\t\tconst struct object_id *oid,\n> +\t\t\t\t\t\tenum object_type type,\n> +\t\t\t\t\t\tunsigned long size,\n> +\t\t\t\t\t\tvoid *buffer,\n> +\t\t\t\t\t\tint *eaten_p);\n> +#define parse_object_buffer(oid, type, size, buffer, eaten_p) \\\n> +\tobject_store_parse_object_buffer(the_repository, oid, type, \\\n> +\t\t\t\t\t size, buffer, eaten_p)\n>  \n>  /** Returns the object, with potentially excess memory allocated. **/\n> -struct object *lookup_unknown_object(const unsigned  char *sha1);\n> +struct object *object_store_lookup_unknown_object(struct repository *r,\n> +\t\t\t\t\t\t  const unsigned char *sha1);\n> +#define lookup_unknown_object(sha1) \\\n> +\tobject_store_lookup_unknown_object(the_repository, sha1)\n>  \n>  struct object_list *object_list_insert(struct object *item,\n>  \t\t\t\t       struct object_list **list_p);\n> @@ -138,6 +168,7 @@ void object_array_remove_duplicates(struct object_array *array);\n>   */\n>  void object_array_clear(struct object_array *array);\n>  \n> -void clear_object_flags(unsigned flags);\n> +extern void object_store_clear_object_flags(struct repository *r, unsigned flags);\n> +#define clear_object_flags(flags) object_store_clear_object_flags(the_repository, flags);\n>  \n>  #endif /* OBJECT_H */\n> diff --git a/repository.h b/repository.h\n> index 417787f3ef..aeb50a19bc 100644\n> --- a/repository.h\n> +++ b/repository.h\n> @@ -1,6 +1,8 @@\n>  #ifndef REPOSITORY_H\n>  #define REPOSITORY_H\n>  \n> +#include \"object.h\"\n> +\n>  struct config_set;\n>  struct index_state;\n>  struct submodule_cache;\n> @@ -24,6 +26,10 @@ struct repository {\n>  \t * Cannot be NULL after initialization.\n>  \t */\n>  \tchar *objectdir;\n> +\tstruct object_store {\n> +\t\tstruct object **obj_hash;\n> +\t\tint nr_objs, obj_hash_size;\n> +\t} objects;\n>  \n>  \t/*\n>  \t * Path to the repository's graft file.\n\n"},{"id":"323951","messageId":"c9a6f3f0-e338-a200-6b57-48b5d926d31c@gmail.com","threadId":"46319","inReplyTo":"xmqq7ezldlhe.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC/WIP PATCH] object store classification","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2017-07-07T14:56:30Z","receivedAt":"2017-07-07T14:56:55Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"\n\nOn 7/6/2017 10:33 PM, Junio C Hamano wrote:\n> Stefan Beller <sbeller@google.com> writes:\n> \n>> Subject: Re: [RFC/WIP PATCH] object store classification\n> \n> I thought you are writing different object-store backends and\n> classifying them into many categories (e.g. local, networked,\n> telepathic, etc.)\n> \n> It is a logical next step to put per-repository things into\n> the_repository.  I skimmed the patch and the changes smelled sane,\n> but I didn't read it really carefully with fine toothed comb.\n> \n> Thanks.  The remainder kept for reference, but no additional\n> comments in there.\n> \n>> This continues the efforts of bw/repo-object, making our code base\n>> more object oriented by adding the object store to the the repository struct.\n>>\n\nIt's great to see this effort continuing.\n\n>> Long term goal of the series that would evolve from this discussion:\n>> * Easier to implement submodule features as it can be in the same process.\n>>\n>> Short term goal:\n>> * get rid of 'add_submodule_odb' in submodule.c\n>>    When fetching or pushing we need to determine if a submodule needs processing\n>>    by finding out if certain commits are present.  To do this we add the submodule\n>>    objects as an alternate object database and do the processing in the same\n>>    process. In case of multiple submodules, this would pollute the object store\n>>    hence being slower and increasing memory usage, too.\n>>\n>> This patch only changes the object store code to be object oriented based\n>> on the repository struct.\n>>\n>> To go for the short term goal we'd need to convert a few more places, mostly\n>> all the construction (blob.c, tree.c, commit.c)\n>>\n\nFor more API/state design purity, I wonder if there should be an \nobject_store structure that is passed to each of the object store APIs \ninstead of passing the repository object. The repository object could \nthen contain an instance of the object_store structure.\n\nThat said, I haven't take a close look at all the code in object.c to \nsee if all the data needed can be cleanly abstracted into an \nobject_store structure.\n\nOne concern I have is that the global state refactoring effort will just \nresult in all the global state getting moved into a single (global) \nrepository object thus limiting it's usefulness.\n\nI'd also love to see this followed up with additional patches that would \nremove the back compat macros.  Should be a relatively straight forward \n\"search and replace\" series of patches.  That will eliminate the code \nreadability/maintenance issues that comes along with macros in addition \nto the APIs themselves.\n\nNo additional comments below...\n\n>> This is marked RFC as I'd want to gather feedback if the approach, presented\n>> in the header files is sound.\n>>\n>> Thanks!\n>>\n>> Signed-off-by: Stefan Beller <sbeller@google.com>\n>> ---\n>>\n>>    Peff, this area of the code seems performance sensitive for a variety of\n>>    use cases, specifically yours. So I cc'd you here. :)\n>>\n>>   object.c     | 77 +++++++++++++++++++++++++++++++++++-------------------------\n>>   object.h     | 51 ++++++++++++++++++++++++++++++++--------\n>>   repository.h |  6 +++++\n>>   3 files changed, 92 insertions(+), 42 deletions(-)\n>>\n>> diff --git a/object.c b/object.c\n>> index 06ba3a11d8..b5ec0bb2f9 100644\n>> --- a/object.c\n>> +++ b/object.c\n>> @@ -5,17 +5,15 @@\n>>   #include \"commit.h\"\n>>   #include \"tag.h\"\n>>   \n>> -static struct object **obj_hash;\n>> -static int nr_objs, obj_hash_size;\n>> -\n>> -unsigned int get_max_object_index(void)\n>> +unsigned int object_store_get_max_index(struct repository *r)\n>>   {\n>> -\treturn obj_hash_size;\n>> +\treturn r->objects.obj_hash_size;\n>>   }\n>>   \n>> -struct object *get_indexed_object(unsigned int idx)\n>> +struct object *object_store_get_indexed(struct repository *r,\n>> +\t\t\t\t\tunsigned int idx)\n>>   {\n>> -\treturn obj_hash[idx];\n>> +\treturn r->objects.obj_hash[idx];\n>>   }\n>>   \n>>   static const char *object_type_strings[] = {\n>> @@ -82,11 +80,15 @@ static void insert_obj_hash(struct object *obj, struct object **hash, unsigned i\n>>    * Look up the record for the given sha1 in the hash map stored in\n>>    * obj_hash.  Return NULL if it was not found.\n>>    */\n>> -struct object *lookup_object(const unsigned char *sha1)\n>> +struct object *object_store_lookup(struct repository *r,\n>> +\t\t\t\t   const unsigned char *sha1)\n>>   {\n>>   \tunsigned int i, first;\n>>   \tstruct object *obj;\n>>   \n>> +\tstruct object **obj_hash = r->objects.obj_hash;\n>> +\tint obj_hash_size = r->objects.obj_hash_size;\n>> +\n>>   \tif (!obj_hash)\n>>   \t\treturn NULL;\n>>   \n>> @@ -114,29 +116,31 @@ struct object *lookup_object(const unsigned char *sha1)\n>>    * power of 2 (but at least 32).  Copy the existing values to the new\n>>    * hash map.\n>>    */\n>> -static void grow_object_hash(void)\n>> +static void grow_object_hash(struct repository *r)\n>>   {\n>>   \tint i;\n>>   \t/*\n>>   \t * Note that this size must always be power-of-2 to match hash_obj\n>>   \t * above.\n>>   \t */\n>> -\tint new_hash_size = obj_hash_size < 32 ? 32 : 2 * obj_hash_size;\n>> +\tint new_hash_size = r->objects.obj_hash_size < 32 ?\n>> +\t\t\t32 : 2 * r->objects.obj_hash_size;\n>>   \tstruct object **new_hash;\n>>   \n>>   \tnew_hash = xcalloc(new_hash_size, sizeof(struct object *));\n>> -\tfor (i = 0; i < obj_hash_size; i++) {\n>> -\t\tstruct object *obj = obj_hash[i];\n>> +\tfor (i = 0; i < r->objects.obj_hash_size; i++) {\n>> +\t\tstruct object *obj = r->objects.obj_hash[i];\n>>   \t\tif (!obj)\n>>   \t\t\tcontinue;\n>>   \t\tinsert_obj_hash(obj, new_hash, new_hash_size);\n>>   \t}\n>> -\tfree(obj_hash);\n>> -\tobj_hash = new_hash;\n>> -\tobj_hash_size = new_hash_size;\n>> +\tfree(r->objects.obj_hash);\n>> +\tr->objects.obj_hash = new_hash;\n>> +\tr->objects.obj_hash_size = new_hash_size;\n>>   }\n>>   \n>> -void *create_object(const unsigned char *sha1, void *o)\n>> +void *object_store_create(struct repository *r,\n>> +\t\t\t  const unsigned char *sha1, void *o)\n>>   {\n>>   \tstruct object *obj = o;\n>>   \n>> @@ -145,15 +149,17 @@ void *create_object(const unsigned char *sha1, void *o)\n>>   \tobj->flags = 0;\n>>   \thashcpy(obj->oid.hash, sha1);\n>>   \n>> -\tif (obj_hash_size - 1 <= nr_objs * 2)\n>> -\t\tgrow_object_hash();\n>> +\tif (r->objects.obj_hash_size - 1 <= r->objects.nr_objs * 2)\n>> +\t\tgrow_object_hash(r);\n>>   \n>> -\tinsert_obj_hash(obj, obj_hash, obj_hash_size);\n>> -\tnr_objs++;\n>> +\tinsert_obj_hash(obj, r->objects.obj_hash, r->objects.obj_hash_size);\n>> +\tr->objects.nr_objs++;\n>>   \treturn obj;\n>>   }\n>>   \n>> -void *object_as_type(struct object *obj, enum object_type type, int quiet)\n>> +void *object_as_type(struct object *obj,\n>> +\t\t     enum object_type type,\n>> +\t\t     int quiet)\n>>   {\n>>   \tif (obj->type == type)\n>>   \t\treturn obj;\n>> @@ -172,15 +178,20 @@ void *object_as_type(struct object *obj, enum object_type type, int quiet)\n>>   \t}\n>>   }\n>>   \n>> -struct object *lookup_unknown_object(const unsigned char *sha1)\n>> +struct object *object_store_lookup_unknown_object(struct repository *r,\n>> +\t\t\t\t\t\t  const unsigned char *sha1)\n>>   {\n>> -\tstruct object *obj = lookup_object(sha1);\n>> +\tstruct object *obj = object_store_lookup(r, sha1);\n>>   \tif (!obj)\n>> -\t\tobj = create_object(sha1, alloc_object_node());\n>> +\t\tobj = object_store_create(r, sha1, alloc_object_node());\n>>   \treturn obj;\n>>   }\n>>   \n>> -struct object *parse_object_buffer(const struct object_id *oid, enum object_type type, unsigned long size, void *buffer, int *eaten_p)\n>> +struct object *object_store_parse_object_buffer(struct repository *r,\n>> +\t\t\t\t\t\tconst struct object_id *oid,\n>> +\t\t\t\t\t\tenum object_type type,\n>> +\t\t\t\t\t\tunsigned long size,\n>> +\t\t\t\t\t\tvoid *buffer, int *eaten_p)\n>>   {\n>>   \tstruct object *obj;\n>>   \t*eaten_p = 0;\n>> @@ -230,17 +241,19 @@ struct object *parse_object_buffer(const struct object_id *oid, enum object_type\n>>   \treturn obj;\n>>   }\n>>   \n>> -struct object *parse_object_or_die(const struct object_id *oid,\n>> -\t\t\t\t   const char *name)\n>> +struct object *object_store_parse_object_or_die(struct repository *r,\n>> +\t\t\t\t\t\tconst struct object_id *oid,\n>> +\t\t\t\t\t\tconst char *name)\n>>   {\n>> -\tstruct object *o = parse_object(oid);\n>> +\tstruct object *o = object_store_parse_object(r, oid);\n>>   \tif (o)\n>>   \t\treturn o;\n>>   \n>>   \tdie(_(\"unable to parse object: %s\"), name ? name : oid_to_hex(oid));\n>>   }\n>>   \n>> -struct object *parse_object(const struct object_id *oid)\n>> +struct object *object_store_parse_object(struct repository *r,\n>> +\t\t\t\t\t const struct object_id *oid)\n>>   {\n>>   \tunsigned long size;\n>>   \tenum object_type type;\n>> @@ -413,12 +426,12 @@ void object_array_remove_duplicates(struct object_array *array)\n>>   \t}\n>>   }\n>>   \n>> -void clear_object_flags(unsigned flags)\n>> +void object_store_clear_object_flags(struct repository *r, unsigned flags)\n>>   {\n>>   \tint i;\n>>   \n>> -\tfor (i=0; i < obj_hash_size; i++) {\n>> -\t\tstruct object *obj = obj_hash[i];\n>> +\tfor (i = 0; i < r->objects.obj_hash_size; i++) {\n>> +\t\tstruct object *obj = r->objects.obj_hash[i];\n>>   \t\tif (obj)\n>>   \t\t\tobj->flags &= ~flags;\n>>   \t}\n>> diff --git a/object.h b/object.h\n>> index 33e5cc9943..1b2ab8ee88 100644\n>> --- a/object.h\n>> +++ b/object.h\n>> @@ -1,6 +1,8 @@\n>>   #ifndef OBJECT_H\n>>   #define OBJECT_H\n>>   \n>> +#include \"repository.h\"\n>> +\n>>   struct object_list {\n>>   \tstruct object *item;\n>>   \tstruct object_list *next;\n>> @@ -59,12 +61,15 @@ extern int type_from_string_gently(const char *str, ssize_t, int gentle);\n>>   /*\n>>    * Return the current number of buckets in the object hashmap.\n>>    */\n>> -extern unsigned int get_max_object_index(void);\n>> +extern unsigned int object_store_get_max_index(struct repository *r);\n>> +#define get_max_object_index() object_store_get_max_index(the_repository)\n>>   \n>>   /*\n>>    * Return the object from the specified bucket in the object hashmap.\n>>    */\n>> -extern struct object *get_indexed_object(unsigned int);\n>> +extern struct object *object_store_get_indexed(struct repository *r,\n>> +\t\t\t\t\t       unsigned int);\n>> +#define get_indexed_object(i) object_store_get_indexed(the_repository, i)\n>>   \n>>   /*\n>>    * This can be used to see if we have heard of the object before, but\n>> @@ -78,34 +83,59 @@ extern struct object *get_indexed_object(unsigned int);\n>>    * half-initialised objects, the caller is expected to initialize them\n>>    * by calling parse_object() on them.\n>>    */\n>> -struct object *lookup_object(const unsigned char *sha1);\n>> +extern struct object *object_store_lookup(struct repository *r,\n>> +\t\t\t\t\t  const unsigned char *sha1);\n>> +#define lookup_object(hash) object_store_lookup(the_repository, hash)\n>>   \n>> -extern void *create_object(const unsigned char *sha1, void *obj);\n>> +extern void *object_store_create(struct repository *r,\n>> +\t\t\t\t const unsigned char *sha1,\n>> +\t\t\t\t void *obj);\n>> +#define create_object(sha1, obj) object_store_create(the_repository, sha1, obj)\n>>   \n>> -void *object_as_type(struct object *obj, enum object_type type, int quiet);\n>> +extern void *object_as_type(struct object *obj,\n>> +\t\t\t    enum object_type type,\n>> +\t\t\t    int quiet);\n>>   \n>>   /*\n>>    * Returns the object, having parsed it to find out what it is.\n>>    *\n>>    * Returns NULL if the object is missing or corrupt.\n>>    */\n>> -struct object *parse_object(const struct object_id *oid);\n>> +extern struct object *object_store_parse_object(struct repository *r,\n>> +\t\t\t\t\t\tconst struct object_id *oid);\n>> +#define parse_object(sha1) \\\n>> +\tobject_store_parse_object(the_repository, sha1)\n>>   \n>>   /*\n>>    * Like parse_object, but will die() instead of returning NULL. If the\n>>    * \"name\" parameter is not NULL, it is included in the error message\n>>    * (otherwise, the hex object ID is given).\n>>    */\n>> -struct object *parse_object_or_die(const struct object_id *oid, const char *name);\n>> +extern struct object *object_store_parse_object_or_die(struct repository *r,\n>> +\t\t\t\t\t\t       const struct object_id *oid,\n>> +\t\t\t\t\t\t       const char *name);\n>> +#define parse_object_or_die(oid, name) \\\n>> +\tobject_store_parse_object_or_die(the_repository, oid, name)\n>>   \n>>   /* Given the result of read_sha1_file(), returns the object after\n>>    * parsing it.  eaten_p indicates if the object has a borrowed copy\n>>    * of buffer and the caller should not free() it.\n>>    */\n>> -struct object *parse_object_buffer(const struct object_id *oid, enum object_type type, unsigned long size, void *buffer, int *eaten_p);\n>> +struct object *object_store_parse_object_buffer(struct repository *r,\n>> +\t\t\t\t\t\tconst struct object_id *oid,\n>> +\t\t\t\t\t\tenum object_type type,\n>> +\t\t\t\t\t\tunsigned long size,\n>> +\t\t\t\t\t\tvoid *buffer,\n>> +\t\t\t\t\t\tint *eaten_p);\n>> +#define parse_object_buffer(oid, type, size, buffer, eaten_p) \\\n>> +\tobject_store_parse_object_buffer(the_repository, oid, type, \\\n>> +\t\t\t\t\t size, buffer, eaten_p)\n>>   \n>>   /** Returns the object, with potentially excess memory allocated. **/\n>> -struct object *lookup_unknown_object(const unsigned  char *sha1);\n>> +struct object *object_store_lookup_unknown_object(struct repository *r,\n>> +\t\t\t\t\t\t  const unsigned char *sha1);\n>> +#define lookup_unknown_object(sha1) \\\n>> +\tobject_store_lookup_unknown_object(the_repository, sha1)\n>>   \n>>   struct object_list *object_list_insert(struct object *item,\n>>   \t\t\t\t       struct object_list **list_p);\n>> @@ -138,6 +168,7 @@ void object_array_remove_duplicates(struct object_array *array);\n>>    */\n>>   void object_array_clear(struct object_array *array);\n>>   \n>> -void clear_object_flags(unsigned flags);\n>> +extern void object_store_clear_object_flags(struct repository *r, unsigned flags);\n>> +#define clear_object_flags(flags) object_store_clear_object_flags(the_repository, flags);\n>>   \n>>   #endif /* OBJECT_H */\n>> diff --git a/repository.h b/repository.h\n>> index 417787f3ef..aeb50a19bc 100644\n>> --- a/repository.h\n>> +++ b/repository.h\n>> @@ -1,6 +1,8 @@\n>>   #ifndef REPOSITORY_H\n>>   #define REPOSITORY_H\n>>   \n>> +#include \"object.h\"\n>> +\n>>   struct config_set;\n>>   struct index_state;\n>>   struct submodule_cache;\n>> @@ -24,6 +26,10 @@ struct repository {\n>>   \t * Cannot be NULL after initialization.\n>>   \t */\n>>   \tchar *objectdir;\n>> +\tstruct object_store {\n>> +\t\tstruct object **obj_hash;\n>> +\t\tint nr_objs, obj_hash_size;\n>> +\t} objects;\n>>   \n>>   \t/*\n>>   \t * Path to the repository's graft file.\n> \n"},{"id":"323963","messageId":"xmqq4luochtv.fsf@gitster.mtv.corp.google.com","threadId":"46319","inReplyTo":"c9a6f3f0-e338-a200-6b57-48b5d926d31c@gmail.com","subject":"Re: [RFC/WIP PATCH] object store classification","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-07T16:50:04Z","receivedAt":"2017-07-07T16:50:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ben Peart <peartben@gmail.com> writes:\n\n> For more API/state design purity, I wonder if there should be an\n> object_store structure that is passed to each of the object store APIs\n> instead of passing the repository object. The repository object could\n> then contain an instance of the object_store structure.\n>\n> That said, I haven't take a close look at all the code in object.c to\n> see if all the data needed can be cleanly abstracted into an\n> object_store structure.\n\nMy gut feeling was it is just the large hashtable that keeps track of\nobjects we have seen, but the object replacement/grafts and other\nthings may also want to become per-repository.\n\n> One concern I have is that the global state refactoring effort will\n> just result in all the global state getting moved into a single\n> (global) repository object thus limiting it's usefulness.\n\nI actually am not worried about it that much, and I say this with\nthe background of having done the same \"grouping a set of global\nstate variables into a single structure and turning them into a\nsingle default instance\" for the_index.  Whether you like it or not,\nthe majority of operations do work on the default instance---that\nwas why the operations could live with just \"a set of global state\nvariables\" in the first place, and the convenience compatibility\nmacros that allow you to operate on the fields of the default\ninstance as if they were separate variables have been a huge\ntypesaver that also reduces the cognitive burden.  I'd expect that\nthe same will hold for the new \"repository\" and the \"object_store\"\nabstractions.\n\n"},{"id":"324145","messageId":"CAGZ79kbTzZLD5FDidDG8SUrKpgRGvA7f9HAu77w+iW8A8zLMAw@mail.gmail.com","threadId":"46319","inReplyTo":"xmqq4luochtv.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC/WIP PATCH] object store classification","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-07-11T01:17:52Z","receivedAt":"2017-07-11T01:17:58Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Jul 7, 2017 at 9:50 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Ben Peart <peartben@gmail.com> writes:\n>\n>> For more API/state design purity, I wonder if there should be an\n>> object_store structure that is passed to each of the object store APIs\n>> instead of passing the repository object. The repository object could\n>> then contain an instance of the object_store structure.\n>>\n>> That said, I haven't take a close look at all the code in object.c to\n>> see if all the data needed can be cleanly abstracted into an\n>> object_store structure.\n>\n> My gut feeling was it is just the large hashtable that keeps track of\n> objects we have seen, but the object replacement/grafts and other\n> things may also want to become per-repository.\n\nThis is similar to the_index which is referenced by the_repository.\nBut as we do not have anything like the_object_store already, we are\nfree to design it, as the required work that needs to be put in is the\nsame.\n\nWith the object replacements/grafts coming up as well as alternates,\nwe definitely want that to be per repository, the question is if we rather\nwant\n\n  the_repository -> many object_stores (one for each, alternate, grafts,\n      and the usual place at $GIT_DIR/objects\n  where the object_store is a hashmap, maybe an additional describing\n  string or path.\n\nor\n\n  the_repository -> the_object_store\n  but the object store is a complex beast having different hash tables\n  for the different alternates.\n\nor\n\n  the_repository -> the_object_store_hash_map\n  which is this patch that would try to put any object related to this\n  repository into the same hashmap and the hashmap is not special\n  for each of the different object locations.\n\n\n>\n>> One concern I have is that the global state refactoring effort will\n>> just result in all the global state getting moved into a single\n>> (global) repository object thus limiting it's usefulness.\n>\n> I actually am not worried about it that much, and I say this with\n> the background of having done the same \"grouping a set of global\n> state variables into a single structure and turning them into a\n> single default instance\" for the_index.  Whether you like it or not,\n> the majority of operations do work on the default instance---that\n> was why the operations could live with just \"a set of global state\n> variables\" in the first place, and the convenience compatibility\n> macros that allow you to operate on the fields of the default\n> instance as if they were separate variables have been a huge\n> typesaver that also reduces the cognitive burden.  I'd expect that\n> the same will hold for the new \"repository\" and the \"object_store\"\n> abstractions.\n\nSounds reasonable to expect.\n\nThanks,\nStefan\n"},{"id":"324211","messageId":"20170711180122.GG161700@google.com","threadId":"46319","inReplyTo":"CAGZ79kbTzZLD5FDidDG8SUrKpgRGvA7f9HAu77w+iW8A8zLMAw@mail.gmail.com","subject":"Re: [RFC/WIP PATCH] object store classification","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-07-11T18:01:22Z","receivedAt":"2017-07-11T18:01:31Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 07/10, Stefan Beller wrote:\n> On Fri, Jul 7, 2017 at 9:50 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> > Ben Peart <peartben@gmail.com> writes:\n> >\n> >> For more API/state design purity, I wonder if there should be an\n> >> object_store structure that is passed to each of the object store APIs\n> >> instead of passing the repository object. The repository object could\n> >> then contain an instance of the object_store structure.\n> >>\n> >> That said, I haven't take a close look at all the code in object.c to\n> >> see if all the data needed can be cleanly abstracted into an\n> >> object_store structure.\n> >\n> > My gut feeling was it is just the large hashtable that keeps track of\n> > objects we have seen, but the object replacement/grafts and other\n> > things may also want to become per-repository.\n> \n> This is similar to the_index which is referenced by the_repository.\n> But as we do not have anything like the_object_store already, we are\n> free to design it, as the required work that needs to be put in is the\n> same.\n> \n> With the object replacements/grafts coming up as well as alternates,\n> we definitely want that to be per repository, the question is if we rather\n> want\n> \n>   the_repository -> many object_stores (one for each, alternate, grafts,\n>       and the usual place at $GIT_DIR/objects\n>   where the object_store is a hashmap, maybe an additional describing\n>   string or path.\n> \n> or\n> \n>   the_repository -> the_object_store\n>   but the object store is a complex beast having different hash tables\n>   for the different alternates.\n\nAfter looking at the patch and some of the comments here I think that\nthis is probably the best approach with a few tweaks (which may be\ncompletely unfounded because I'm not familiar with all the object store\ncode).\n\nIn an OO world I would envision a single object (let's say 'struct\nobject_store') which is responsible for managing a repository's objects\nno matter where the individual objects came from (main object store or\nan alternate for that repository).  And if I understand correctly the\nsingle hash table that exists now caches objects like this.\n\nI also think that such a 'struct object_store' should probably be an\nopaque type to a majority of the code base.  This means that it probably\nshouldn't have its definition in 'repository.h'.\n\nAs far as API, I think it should be similar to the new repo_config (old\none too, though it was implicit) API in that the code base doesn't need\nto know about 'struct configset', it just passes a pointer to the\nrepository and then the 'struct configset' which is stored in the\nrepository is operated on under the hood.  This way the code base would\njust query for an object using the repository as a handle like:\n\n  get_object(repo, OID);\n\n  and not:\n\n  get_object(repo->object_store, OID);\n\nOf course under the hood it would be preferable to have the functions\noperate on the object_store struct explicitly.\n\n> \n> or\n> \n>   the_repository -> the_object_store_hash_map\n>   which is this patch that would try to put any object related to this\n>   repository into the same hashmap and the hashmap is not special\n>   for each of the different object locations.\n> \n> \n> >\n> >> One concern I have is that the global state refactoring effort will\n> >> just result in all the global state getting moved into a single\n> >> (global) repository object thus limiting it's usefulness.\n\nI think we do need to think about this, but it shouldn't be too much of\na concern right now.  The first step is to get enough of the object\nstore object oriented such that you can have two object stores\ncorresponding to two different repositories working in parallel.\n\n> >\n> > I actually am not worried about it that much, and I say this with\n> > the background of having done the same \"grouping a set of global\n> > state variables into a single structure and turning them into a\n> > single default instance\" for the_index.  Whether you like it or not,\n> > the majority of operations do work on the default instance---that\n> > was why the operations could live with just \"a set of global state\n> > variables\" in the first place, and the convenience compatibility\n> > macros that allow you to operate on the fields of the default\n> > instance as if they were separate variables have been a huge\n> > typesaver that also reduces the cognitive burden.  I'd expect that\n> > the same will hold for the new \"repository\" and the \"object_store\"\n> > abstractions.\n> \n> Sounds reasonable to expect.\n> \n> Thanks,\n> Stefan\n\n-- \nBrandon Williams\n"},{"id":"324218","messageId":"xmqqd1963j84.fsf@gitster.mtv.corp.google.com","threadId":"46319","inReplyTo":"20170711180122.GG161700@google.com","subject":"Re: [RFC/WIP PATCH] object store classification","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-11T18:46:03Z","receivedAt":"2017-07-11T18:46:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Williams <bmwill@google.com> writes:\n\n>>   the_repository -> the_object_store\n>>   but the object store is a complex beast having different hash tables\n>>   for the different alternates.\n>\n> After looking at the patch and some of the comments here I think that\n> this is probably the best approach with a few tweaks (which may be\n> completely unfounded because I'm not familiar with all the object store\n> code).\n>\n> In an OO world I would envision a single object (let's say 'struct\n> object_store') which is responsible for managing a repository's objects\n> no matter where the individual objects came from (main object store or\n> an alternate for that repository).  And if I understand correctly the\n> single hash table that exists now caches objects like this.\n\nI would say that conceptually an object-store object has an\ninterface to answer \"give me info on the object I can refer to with\nthis object name\".  At the implementation level, it should have a\nhashtable (lazily populated) for all the objects in a single\n$GIT_OBJECT_DIRECTORY, grafts/replace info, and a set of pointers to\nother object-store instances that are its alternate object stores.\nYou'd need to have a mechanism to avoid creating cycles in these\npointers, of course.\n\n\n"},{"id":"324227","messageId":"CAGZ79kaCXLtauL5UNT-8gCpzs5A+LU6Ed41+_LdBO7zDyW1Dyg@mail.gmail.com","threadId":"46319","inReplyTo":"xmqqd1963j84.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC/WIP PATCH] object store classification","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-07-11T19:49:43Z","receivedAt":"2017-07-11T19:49:49Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Jul 11, 2017 at 11:46 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Brandon Williams <bmwill@google.com> writes:\n>\n>>>   the_repository -> the_object_store\n>>>   but the object store is a complex beast having different hash tables\n>>>   for the different alternates.\n>>\n>> After looking at the patch and some of the comments here I think that\n>> this is probably the best approach with a few tweaks (which may be\n>> completely unfounded because I'm not familiar with all the object store\n>> code).\n>>\n>> In an OO world I would envision a single object (let's say 'struct\n>> object_store') which is responsible for managing a repository's objects\n>> no matter where the individual objects came from (main object store or\n>> an alternate for that repository).  And if I understand correctly the\n>> single hash table that exists now caches objects like this.\n>\n> I would say that conceptually an object-store object has an\n> interface to answer \"give me info on the object I can refer to with\n> this object name\".\n\nok.\n\n> At the implementation level, it should have a\n> hashtable (lazily populated) for all the objects in a single\n> $GIT_OBJECT_DIRECTORY, grafts/replace info, and a set of pointers to\n> other object-store instances that are its alternate object stores.\n\nSo one repository has one or more object stores?\n\nI would expect that most of the time the question from above\n\"give me info on the object I can refer to with this object name\"\nis asked with the additional information: \"and I know it is in this\nrepository\", so we rather want to have\n\n  lookup_object(struct *repo, char *name);\n\ninstead of\n\n  lookup_object(struct *object_store, char *name);\n\nbecause the caller most likely would not care if the object\ncomes from the alternate or from the main object store?\n(There may be special cases in e.g. repacking/gc where\nwe want to instruct the repacker to only repack the main\nobject store, ignoring any alternates or such, but any other\ncommand would not care, AFAICT)\n\nSo we could have the convenience function\n\n    lookup_object(struct *repo, char *name)\n    {\n        foreach_objectstore(repo, lookup_object_in_single_store, object)\n            if (object)\n                return object;\n        return NULL;\n    }\n\nbut such code is related to storing objects, that I would prefer to see\nit in the object store (object.{h,c}) instead of the repository code.\nAlso my initial plan was to have all objects (including from any alternate)\nin a single hashmap per repository as that seems to be most efficient\nassuming all alternates fit into memory.\n\nThis whole object store object orientation is only started to not have\nthe memory pressure from lots of submodule objects polluting the\nmain object store. When we have its own hashmap for each alternate\nour performance would degrade with the number of alternates.\n(Assuming the hypothetical worst case of one alternate per object,\nthen the lookup time would be linear in time given the above function,\nI wonder if there are users that heavily use lots of alternates such that\nthis performance characteristics would be measurable in the code to\nbe written)\n\n> You'd need to have a mechanism to avoid creating cycles in these\n> pointers, of course.\n\nThis is another complication with many hashmaps (=object stores).\nIn the future, is it likely that we would want to drop an alternate\nstore from within a running process? If so we'd want to have\nhashmaps per alternate, otherwise I only see disadvantages\nfor more than one hashmap (-> more than one object store)\nper repository. And with that said, I think we can make a wrapper\nstruct object_store, that would encapsulate all needed variables.\n\n+       struct object_store {\n+               struct object **obj_hash;\n+               int nr_objs, obj_hash_size;\n+       } objects;\n\nBut instead of defining it at the repository, we'd rather define it\nin object.h.\n\nStefan\n"},{"id":"324231","messageId":"xmqq4lui3ei9.fsf@gitster.mtv.corp.google.com","threadId":"46319","inReplyTo":"CAGZ79kaCXLtauL5UNT-8gCpzs5A+LU6Ed41+_LdBO7zDyW1Dyg@mail.gmail.com","subject":"Re: [RFC/WIP PATCH] object store classification","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-11T20:27:58Z","receivedAt":"2017-07-11T20:28:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n>> At the implementation level, it should have a\n>> hashtable (lazily populated) for all the objects in a single\n>> $GIT_OBJECT_DIRECTORY, grafts/replace info, and a set of pointers to\n>> other object-store instances that are its alternate object stores.\n>\n> So one repository has one or more object stores?\n\nOne repository foo/.git/ has one foo/.git/objects/ directory, so it\nhas its own single object store.  That object store may refer to\nanother object store by having foo/.git/objects/info/alternates.\n\nSimilarly, foo/.git/objects/info/grafts and foo/.git/refs/replace/\nwould belong to the single object store repository foo/.git/ has.\n\n> I would expect that most of the time the question from above\n> \"give me info on the object I can refer to with this object name\"\n> is asked with the additional information: \"and I know it is in this\n> repository\", so we rather want to have\n>\n>   lookup_object(struct *repo, char *name);\n>\n> instead of\n>\n>   lookup_object(struct *object_store, char *name);\n\nAbsolutely.  That is why repository has its own single object_store,\nwhich may refer to other object_stores.\n"}]}