{"thread":{"id":"1898","subject":"[PATCH 6/8] fetch.c: Remove redundant test of TO_SCAN in process()","startedAt":"2005-09-21T16:18:29Z","lastAt":"2005-09-22T08:50:09Z","messageCount":12,"participants":["Sergey Vlasov","Junio C Hamano","Daniel Barkalow"],"isPatch":true,"patchVersion":1,"patchTotal":8},"messages":[{"id":"9050","messageId":"20050921161829.GA20944@master.mivlgu.local","threadId":"1898","inReplyTo":null,"subject":"[PATCH 0/8] fetch.c optimizations","fromName":"Sergey Vlasov","fromEmail":"vsu@altlinux.ru","sentAt":"2005-09-21T16:18:29Z","receivedAt":"2005-09-21T16:18:29Z","isPatch":true,"sender":{"key":"vsu@altlinux.ru","avatar":"https://avatars.githubusercontent.com/u/616082?v=4"},"body":"Hello!\n\nI have noticed that git-*-fetch now uses much more CPU than it was\nbefore the modifications to fix recovery after interrupted fetch.\nHere is a series of small patches which fix the problems which I have\nfound (some of the fixes give pretty impressive results, like a 14x\ndecrease of CPU time); as a positive side effect, fetch.c becomes\nslightly smaller and hopefully simpler than before.\n\n-- \nSergey Vlasov\n"},{"id":"9049","messageId":"20050921161855.896F1E010FC@center4.mivlgu.local","threadId":"1898","inReplyTo":"20050921161829.GA20944@master.mivlgu.local","subject":"[PATCH 1/8] fetch.c: Remove useless lookup_object_type() call in process()","fromName":"Sergey Vlasov","fromEmail":"vsu@altlinux.ru","sentAt":"2005-09-21T16:18:55Z","receivedAt":"2005-09-21T16:18:55Z","isPatch":true,"sender":{"key":"vsu@altlinux.ru","avatar":"https://avatars.githubusercontent.com/u/616082?v=4"},"body":"In all places where process() is called except the one in pull() (which\nis executed only once) the pointer to the object is already available,\nso pass it as the argument to process() instead of sha1 and avoid an\nunneeded call to lookup_object_type().\n\n---\n\n fetch.c |   23 ++++++++++-------------\n 1 files changed, 10 insertions(+), 13 deletions(-)\n\n3a0ec5d22a2f9828bebae9b13e5848291685e4c5\ndiff --git a/fetch.c b/fetch.c\n--- a/fetch.c\n+++ b/fetch.c\n@@ -33,7 +33,7 @@ static void report_missing(const char *w\n \t\twhat, missing_hex, sha1_to_hex(current_commit_sha1));\n }\n \n-static int process(unsigned char *sha1, const char *type);\n+static int process(struct object *obj);\n \n static int process_tree(struct tree *tree)\n {\n@@ -46,8 +46,7 @@ static int process_tree(struct tree *tre\n \ttree->entries = NULL;\n \twhile (entry) {\n \t\tstruct tree_entry_list *next = entry->next;\n-\t\tif (process(entry->item.any->sha1,\n-\t\t\t    entry->directory ? tree_type : blob_type))\n+\t\tif (process(entry->item.any))\n \t\t\treturn -1;\n \t\tfree(entry);\n \t\tentry = next;\n@@ -79,7 +78,7 @@ static int process_commit(struct commit \n \tpull_say(\"walk %s\\n\", sha1_to_hex(commit->object.sha1));\n \n \tif (get_tree) {\n-\t\tif (process(commit->tree->object.sha1, tree_type))\n+\t\tif (process(&commit->tree->object))\n \t\t\treturn -1;\n \t\tif (!get_all)\n \t\t\tget_tree = 0;\n@@ -87,7 +86,7 @@ static int process_commit(struct commit \n \tif (get_history) {\n \t\tstruct commit_list *parents = commit->parents;\n \t\tfor (; parents; parents = parents->next) {\n-\t\t\tif (process(parents->item->object.sha1, commit_type))\n+\t\t\tif (process(&parents->item->object))\n \t\t\t\treturn -1;\n \t\t}\n \t}\n@@ -98,7 +97,7 @@ static int process_tag(struct tag *tag)\n {\n \tif (parse_tag(tag))\n \t\treturn -1;\n-\treturn process(tag->tagged->sha1, NULL);\n+\treturn process(tag->tagged);\n }\n \n static struct object_list *process_queue = NULL;\n@@ -133,12 +132,10 @@ static int process_object(struct object \n \t\t     obj->type, sha1_to_hex(obj->sha1));\n }\n \n-static int process(unsigned char *sha1, const char *type)\n+static int process(struct object *obj)\n {\n-\tstruct object *obj = lookup_object_type(sha1, type);\n-\n-\tif (has_sha1_file(sha1)) {\n-\t\tparse_object(sha1);\n+\tif (has_sha1_file(obj->sha1)) {\n+\t\tparse_object(obj->sha1);\n \t\t/* We already have it, so we should scan it now. */\n \t\tif (obj->flags & (SCANNED | TO_SCAN))\n \t\t\treturn 0;\n@@ -153,7 +150,7 @@ static int process(unsigned char *sha1, \n \tprocess_queue_end = &(*process_queue_end)->next;\n \tobj->flags |= TO_FETCH;\n \n-\tprefetch(sha1);\n+\tprefetch(obj->sha1);\n \t\t\n \treturn 0;\n }\n@@ -228,7 +225,7 @@ int pull(char *target)\n \tif (interpret_target(target, sha1))\n \t\treturn error(\"Could not interpret %s as something to pull\",\n \t\t\t     target);\n-\tif (process(sha1, NULL))\n+\tif (process(lookup_unknown_object(sha1)))\n \t\treturn -1;\n \tif (loop())\n \t\treturn -1;\n"},{"id":"9052","messageId":"20050921161857.96621E01C96@center4.mivlgu.local","threadId":"1898","inReplyTo":"20050921161829.GA20944@master.mivlgu.local","subject":"[PATCH 2/8] fetch.c: Make process() look at each object only once","fromName":"Sergey Vlasov","fromEmail":"vsu@altlinux.ru","sentAt":"2005-09-21T16:18:57Z","receivedAt":"2005-09-21T16:18:57Z","isPatch":true,"sender":{"key":"vsu@altlinux.ru","avatar":"https://avatars.githubusercontent.com/u/616082?v=4"},"body":"The process() function is very often called multiple times for the\nsame object (because lots of trees refer to the same blobs), but did\nnot have a fast check for this, therefore a lot of useless calls to\nhas_sha1_file() and parse_object() were made before discovering that\nnothing needs to be done.\n\nThis patch adds the SEEN flag which is used in process() to make it\nlook at each object only once.  When testing git-local-fetch on the\nrepository of GIT, this gives a 14x improvement in CPU usage (mainly\nbecause the redundant calls to parse_object() are now avoided -\nparse_object() always unpacks and parses the object data, even if it\nwas already parsed before).\n\n---\n\n fetch.c |    5 +++++\n 1 files changed, 5 insertions(+), 0 deletions(-)\n\n03abc6806a4433807bc21cf3f1aac2528c655c8f\ndiff --git a/fetch.c b/fetch.c\n--- a/fetch.c\n+++ b/fetch.c\n@@ -58,6 +58,7 @@ static int process_tree(struct tree *tre\n #define TO_FETCH\t2U\n #define TO_SCAN\t\t4U\n #define SCANNED\t\t8U\n+#define SEEN\t\t16U\n \n static struct commit_list *complete = NULL;\n \n@@ -134,6 +135,10 @@ static int process_object(struct object \n \n static int process(struct object *obj)\n {\n+\tif (obj->flags & SEEN)\n+\t\treturn 0;\n+\tobj->flags |= SEEN;\n+\n \tif (has_sha1_file(obj->sha1)) {\n \t\tparse_object(obj->sha1);\n \t\t/* We already have it, so we should scan it now. */\n"},{"id":"9051","messageId":"20050921161859.A29A0E2FFC7@center4.mivlgu.local","threadId":"1898","inReplyTo":"20050921161829.GA20944@master.mivlgu.local","subject":"[PATCH 3/8] fetch.c: Remove redundant SCANNED flag","fromName":"Sergey Vlasov","fromEmail":"vsu@altlinux.ru","sentAt":"2005-09-21T16:18:59Z","receivedAt":"2005-09-21T16:18:59Z","isPatch":true,"sender":{"key":"vsu@altlinux.ru","avatar":"https://avatars.githubusercontent.com/u/616082?v=4"},"body":"After adding the SEEN flag, the SCANNED flag became obviously\nredundant - each object can get into process_queue through process()\nonly once, and therefore multiple calls to process_object() for the\nsame object are not possible.\n\n---\n\n fetch.c |    7 +------\n 1 files changed, 1 insertions(+), 6 deletions(-)\n\n320120295a258c4b66c43ee3b4ff6af962333950\ndiff --git a/fetch.c b/fetch.c\n--- a/fetch.c\n+++ b/fetch.c\n@@ -57,7 +57,6 @@ static int process_tree(struct tree *tre\n #define COMPLETE\t1U\n #define TO_FETCH\t2U\n #define TO_SCAN\t\t4U\n-#define SCANNED\t\t8U\n #define SEEN\t\t16U\n \n static struct commit_list *complete = NULL;\n@@ -106,10 +105,6 @@ static struct object_list **process_queu\n \n static int process_object(struct object *obj)\n {\n-\tif (obj->flags & SCANNED)\n-\t\treturn 0;\n-\tobj->flags |= SCANNED;\n-\n \tif (obj->type == commit_type) {\n \t\tif (process_commit((struct commit *)obj))\n \t\t\treturn -1;\n@@ -142,7 +137,7 @@ static int process(struct object *obj)\n \tif (has_sha1_file(obj->sha1)) {\n \t\tparse_object(obj->sha1);\n \t\t/* We already have it, so we should scan it now. */\n-\t\tif (obj->flags & (SCANNED | TO_SCAN))\n+\t\tif (obj->flags & TO_SCAN)\n \t\t\treturn 0;\n \t\tobject_list_insert(obj, process_queue_end);\n \t\tprocess_queue_end = &(*process_queue_end)->next;\n"},{"id":"9047","messageId":"20050921161901.A512FE010FC@center4.mivlgu.local","threadId":"1898","inReplyTo":"20050921161829.GA20944@master.mivlgu.local","subject":"[PATCH 4/8] fetch.c: Remove redundant TO_FETCH flag","fromName":"Sergey Vlasov","fromEmail":"vsu@altlinux.ru","sentAt":"2005-09-21T16:19:01Z","receivedAt":"2005-09-21T16:19:01Z","isPatch":true,"sender":{"key":"vsu@altlinux.ru","avatar":"https://avatars.githubusercontent.com/u/616082?v=4"},"body":"The TO_FETCH flag also became redundant after adding the SEEN flag -\nit was set and checked in process() to prevent adding the same object\nto process_queue multiple times, but now SEEN guards against this.\n\n---\n\n fetch.c |    4 +---\n 1 files changed, 1 insertions(+), 3 deletions(-)\n\n92ef6ee593199b80e01ebe93a48c03508941a340\ndiff --git a/fetch.c b/fetch.c\n--- a/fetch.c\n+++ b/fetch.c\n@@ -55,7 +55,6 @@ static int process_tree(struct tree *tre\n }\n \n #define COMPLETE\t1U\n-#define TO_FETCH\t2U\n #define TO_SCAN\t\t4U\n #define SEEN\t\t16U\n \n@@ -144,11 +143,10 @@ static int process(struct object *obj)\n \t\tobj->flags |= TO_SCAN;\n \t\treturn 0;\n \t}\n-\tif (obj->flags & (COMPLETE | TO_FETCH))\n+\tif (obj->flags & COMPLETE)\n \t\treturn 0;\n \tobject_list_insert(obj, process_queue_end);\n \tprocess_queue_end = &(*process_queue_end)->next;\n-\tobj->flags |= TO_FETCH;\n \n \tprefetch(obj->sha1);\n \t\t\n"},{"id":"9048","messageId":"20050921161903.AC3D2E2FFC8@center4.mivlgu.local","threadId":"1898","inReplyTo":"20050921161829.GA20944@master.mivlgu.local","subject":"[PATCH 5/8] fetch.c: Remove some duplicated code in process()","fromName":"Sergey Vlasov","fromEmail":"vsu@altlinux.ru","sentAt":"2005-09-21T16:19:03Z","receivedAt":"2005-09-21T16:19:03Z","isPatch":true,"sender":{"key":"vsu@altlinux.ru","avatar":"https://avatars.githubusercontent.com/u/616082?v=4"},"body":"It does not matter if we call prefetch() or set the TO_SCAN flag before\nor after adding the object to process_queue.  However, doing it before\nobject_list_insert() allows us to kill 3 lines of duplicated code.\n\n---\n\n fetch.c |   13 +++++--------\n 1 files changed, 5 insertions(+), 8 deletions(-)\n\ndc255341f62596b0808c383ef8f3eff044be5515\ndiff --git a/fetch.c b/fetch.c\n--- a/fetch.c\n+++ b/fetch.c\n@@ -138,18 +138,15 @@ static int process(struct object *obj)\n \t\t/* We already have it, so we should scan it now. */\n \t\tif (obj->flags & TO_SCAN)\n \t\t\treturn 0;\n-\t\tobject_list_insert(obj, process_queue_end);\n-\t\tprocess_queue_end = &(*process_queue_end)->next;\n \t\tobj->flags |= TO_SCAN;\n-\t\treturn 0;\n+\t} else {\n+\t\tif (obj->flags & COMPLETE)\n+\t\t\treturn 0;\n+\t\tprefetch(obj->sha1);\n \t}\n-\tif (obj->flags & COMPLETE)\n-\t\treturn 0;\n+\t\t\n \tobject_list_insert(obj, process_queue_end);\n \tprocess_queue_end = &(*process_queue_end)->next;\n-\n-\tprefetch(obj->sha1);\n-\t\t\n \treturn 0;\n }\n \n"},{"id":"9046","messageId":"20050921161905.B5462E01C96@center4.mivlgu.local","threadId":"1898","inReplyTo":"20050921161829.GA20944@master.mivlgu.local","subject":"[PATCH 6/8] fetch.c: Remove redundant test of TO_SCAN in process()","fromName":"Sergey Vlasov","fromEmail":"vsu@altlinux.ru","sentAt":"2005-09-21T16:19:05Z","receivedAt":"2005-09-21T16:19:05Z","isPatch":true,"sender":{"key":"vsu@altlinux.ru","avatar":"https://avatars.githubusercontent.com/u/616082?v=4"},"body":"If the SEEN flag was not set, the TO_SCAN flag cannot be set,\ntherefore testing it is pointless.\n\n---\n\n fetch.c |    2 --\n 1 files changed, 0 insertions(+), 2 deletions(-)\n\n6f33848a2f4aecd061d9499dfc88824235572695\ndiff --git a/fetch.c b/fetch.c\n--- a/fetch.c\n+++ b/fetch.c\n@@ -136,8 +136,6 @@ static int process(struct object *obj)\n \tif (has_sha1_file(obj->sha1)) {\n \t\tparse_object(obj->sha1);\n \t\t/* We already have it, so we should scan it now. */\n-\t\tif (obj->flags & TO_SCAN)\n-\t\t\treturn 0;\n \t\tobj->flags |= TO_SCAN;\n \t} else {\n \t\tif (obj->flags & COMPLETE)\n"},{"id":"9053","messageId":"20050921161907.BC297E010FC@center4.mivlgu.local","threadId":"1898","inReplyTo":"20050921161829.GA20944@master.mivlgu.local","subject":"[PATCH 7/8] fetch.c: Clean up object flag definitions","fromName":"Sergey Vlasov","fromEmail":"vsu@altlinux.ru","sentAt":"2005-09-21T16:19:07Z","receivedAt":"2005-09-21T16:19:07Z","isPatch":true,"sender":{"key":"vsu@altlinux.ru","avatar":"https://avatars.githubusercontent.com/u/616082?v=4"},"body":"Remove holes left after deleting flags, and use shifts to emphasize\nthat flags are single bits.\n\n---\n\n fetch.c |    6 +++---\n 1 files changed, 3 insertions(+), 3 deletions(-)\n\n42e3d6ef7c0a4eda60032815a36039240dc5cd0b\ndiff --git a/fetch.c b/fetch.c\n--- a/fetch.c\n+++ b/fetch.c\n@@ -54,9 +54,9 @@ static int process_tree(struct tree *tre\n \treturn 0;\n }\n \n-#define COMPLETE\t1U\n-#define TO_SCAN\t\t4U\n-#define SEEN\t\t16U\n+#define COMPLETE\t(1U << 0)\n+#define SEEN\t\t(1U << 1)\n+#define TO_SCAN\t\t(1U << 2)\n \n static struct commit_list *complete = NULL;\n \n"},{"id":"9054","messageId":"20050921161909.C39C5E2FFC7@center4.mivlgu.local","threadId":"1898","inReplyTo":"20050921161829.GA20944@master.mivlgu.local","subject":"[PATCH 8/8] fetch.c: Remove call to parse_object() from process()","fromName":"Sergey Vlasov","fromEmail":"vsu@altlinux.ru","sentAt":"2005-09-21T16:19:09Z","receivedAt":"2005-09-21T16:19:09Z","isPatch":true,"sender":{"key":"vsu@altlinux.ru","avatar":"https://avatars.githubusercontent.com/u/616082?v=4"},"body":"The call to parse_object() in process() is not actually needed - if\nthe object type is unknown, parse_object() will be called by loop();\nif the type is known, the object will be parsed by the appropriate\nprocess_*() function.\n\nAfter this change blobs which exist locally are no longer parsed,\nwhich gives about 2x CPU usage improvement; the downside is that there\nwill be no warnings for existing corrupted blobs, but detecting such\ncorruption is the job of git-fsck-objects, not the fetch programs.\nNewly fetched objects are still checked for corruption in http-fetch.c\nand ssh-fetch.c (local-fetch.c does not seem to do it, but the removed\nparse_object() call would not be reached for new objects anyway).\n\n---\n\n fetch.c |    1 -\n 1 files changed, 0 insertions(+), 1 deletions(-)\n\nb8a4d51743787be17f9572dce2eb2f4040ac241b\ndiff --git a/fetch.c b/fetch.c\n--- a/fetch.c\n+++ b/fetch.c\n@@ -134,7 +134,6 @@ static int process(struct object *obj)\n \tobj->flags |= SEEN;\n \n \tif (has_sha1_file(obj->sha1)) {\n-\t\tparse_object(obj->sha1);\n \t\t/* We already have it, so we should scan it now. */\n \t\tobj->flags |= TO_SCAN;\n \t} else {\n"},{"id":"9064","messageId":"7v3bnyurw6.fsf@assigned-by-dhcp.cox.net","threadId":"1898","inReplyTo":"20050921161855.896F1E010FC@center4.mivlgu.local","subject":"Re: [PATCH 1/8] fetch.c: Remove useless lookup_object_type() call in process()","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-09-21T19:45:13Z","receivedAt":"2005-09-21T19:45:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sergey Vlasov <vsu@altlinux.ru> writes:\n\n> In all places where process() is called except the one in pull() (which\n> is executed only once) the pointer to the object is already available,\n> so pass it as the argument to process() instead of sha1 and avoid an\n> unneeded call to lookup_object_type().\n\nAgreed, except we probably would want to pass the expected type\nto process() so that we can make sure the object is of that type,\nperhaps?\n\nHaving said that, I am really happy that you seem to have fixed\nit a lot better than my previous attempt, after which I was\nreally dissapointed that 'git clone' was still unusablly slow,\njust walking commits in huge packs.\n\nThanks.\n"},{"id":"9071","messageId":"Pine.LNX.4.63.0509211700161.23242@iabervon.org","threadId":"1898","inReplyTo":"20050921161829.GA20944@master.mivlgu.local","subject":"Re: [PATCH 0/8] fetch.c optimizations","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2005-09-21T21:03:31Z","receivedAt":"2005-09-21T21:03:31Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Wed, 21 Sep 2005, Sergey Vlasov wrote:\n\n> Hello!\n> \n> I have noticed that git-*-fetch now uses much more CPU than it was\n> before the modifications to fix recovery after interrupted fetch.\n> Here is a series of small patches which fix the problems which I have\n> found (some of the fixes give pretty impressive results, like a 14x\n> decrease of CPU time); as a positive side effect, fetch.c becomes\n> slightly smaller and hopefully simpler than before.\n\nThese all look like good changes. It would also be worth doing some \noptimization in the library, like making parse_object just return the \nobject if we already have it.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"9089","messageId":"20050922085009.GC20944@master.mivlgu.local","threadId":"1898","inReplyTo":"7v3bnyurw6.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH 1/8] fetch.c: Remove useless lookup_object_type() call in process()","fromName":"Sergey Vlasov","fromEmail":"vsu@altlinux.ru","sentAt":"2005-09-22T08:50:09Z","receivedAt":"2005-09-22T08:50:09Z","isPatch":true,"sender":{"key":"vsu@altlinux.ru","avatar":"https://avatars.githubusercontent.com/u/616082?v=4"},"body":"On Wed, Sep 21, 2005 at 12:45:13PM -0700, Junio C Hamano wrote:\n> Sergey Vlasov <vsu@altlinux.ru> writes:\n> \n> > In all places where process() is called except the one in pull() (which\n> > is executed only once) the pointer to the object is already available,\n> > so pass it as the argument to process() instead of sha1 and avoid an\n> > unneeded call to lookup_object_type().\n> \n> Agreed, except we probably would want to pass the expected type\n> to process() so that we can make sure the object is of that type,\n> perhaps?\n\nThis is not needed - all parse_*_buffer() functions, which fill in\npointers to referenced objects, specify required types themselves by\nusing lookup_commit(), lookup_tree(), etc.; even parse_tag_buffer()\nuses lookup_object_type().\n\nThe only way to get a \"struct object\" with an unspecified type is by\ncalling lookup_unknown_object() (or lookup_object_type() with NULL\ntype) - grep shows than nothing in GIT does this, except the pull()\nfunction in fetch.c (which obviously does not know type of the object\nto be fetched in advance).\n"}]}