{"thread":{"id":"50266","subject":"[PATCH v2] pack-objects: Use packing_data lock instead of read_mutex","startedAt":"2019-01-19T15:45:07Z","lastAt":"2019-01-23T17:51:49Z","messageCount":10,"participants":["Patrick Hogg","Duy Nguyen","Jeff King","Elijah Newren","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"367173","messageId":"20190119154337.6556-1-phogg@novamoon.net","threadId":"50266","inReplyTo":null,"subject":"[PATCH v2] pack-objects: Use packing_data lock instead of read_mutex","fromName":"Patrick Hogg","fromEmail":"phogg@novamoon.net","sentAt":"2019-01-19T15:43:38Z","receivedAt":"2019-01-19T15:45:07Z","isPatch":true,"sender":{"key":"phogg@novamoon.net","avatar":"https://avatars.githubusercontent.com/u/7890416?v=4"},"body":"ac77d0c37 (\"pack-objects: shrink size field in struct object_entry\",\n2018-04-14) added an extra usage of read_lock/read_unlock in the newly\nintroduced oe_get_size_slow for thread safety in parallel calls to\ntry_delta(). Unfortunately oe_get_size_slow is also used in serial\ncode, some of which is called before the first invocation of\nll_find_deltas. As such the read mutex is not guaranteed to be\ninitialized.\n\nResolve this by using the existing lock in packing_data which is\ninitialized early in cmd_pack_objects instead of read_mutex.\nAdditionally, upgrade the packing_data lock to a recursive mutex to\nmake it a suitable replacement for read_mutex.\n\nSigned-off-by: Patrick Hogg <phogg@novamoon.net>\n---\n\nAs I mentioned in the prior thread I think that it will be simpler\nto simply use the existing lock in packing_data instead of moving\nread_mutex. I can go back to simply moving read_mutex to the\npacking_data struct if that that is preferable, though.\n\nI also removed the #ifndef NO_PTHREADS in prepare_packing_data around\nthe initialization of &pdata->lock since I had to upgrade the lock to\na recursive mutex. As far as I can tell init_recursive_mutex (and\npthread_mutex_init for that matter) have that protection already so it\nappears to be redundant.\n\n builtin/pack-objects.c | 27 ++++++++++++---------------\n pack-objects.c         |  4 +---\n 2 files changed, 13 insertions(+), 18 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 411aefd68..5439b434c 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -1954,9 +1954,8 @@ static int delta_cacheable(unsigned long src_size, unsigned long trg_size,\n }\n \n /* Protect access to object database */\n-static pthread_mutex_t read_mutex;\n-#define read_lock()\t\tpthread_mutex_lock(&read_mutex)\n-#define read_unlock()\t\tpthread_mutex_unlock(&read_mutex)\n+#define pack_lock()\t\tpacking_data_lock(&to_pack)\n+#define pack_unlock()\t\tpacking_data_unlock(&to_pack)\n \n /* Protect delta_cache_size */\n static pthread_mutex_t cache_mutex;\n@@ -1993,11 +1992,11 @@ unsigned long oe_get_size_slow(struct packing_data *pack,\n \tunsigned long used, avail, size;\n \n \tif (e->type_ != OBJ_OFS_DELTA && e->type_ != OBJ_REF_DELTA) {\n-\t\tread_lock();\n+\t\tpack_lock();\n \t\tif (oid_object_info(the_repository, &e->idx.oid, &size) < 0)\n \t\t\tdie(_(\"unable to get size of %s\"),\n \t\t\t    oid_to_hex(&e->idx.oid));\n-\t\tread_unlock();\n+\t\tpack_unlock();\n \t\treturn size;\n \t}\n \n@@ -2005,7 +2004,7 @@ unsigned long oe_get_size_slow(struct packing_data *pack,\n \tif (!p)\n \t\tBUG(\"when e->type is a delta, it must belong to a pack\");\n \n-\tread_lock();\n+\tpack_lock();\n \tw_curs = NULL;\n \tbuf = use_pack(p, &w_curs, e->in_pack_offset, &avail);\n \tused = unpack_object_header_buffer(buf, avail, &type, &size);\n@@ -2014,7 +2013,7 @@ unsigned long oe_get_size_slow(struct packing_data *pack,\n \t\t    oid_to_hex(&e->idx.oid));\n \n \tunuse_pack(&w_curs);\n-\tread_unlock();\n+\tpack_unlock();\n \treturn size;\n }\n \n@@ -2076,9 +2075,9 @@ static int try_delta(struct unpacked *trg, struct unpacked *src,\n \n \t/* Load data if not already done */\n \tif (!trg->data) {\n-\t\tread_lock();\n+\t\tpack_lock();\n \t\ttrg->data = read_object_file(&trg_entry->idx.oid, &type, &sz);\n-\t\tread_unlock();\n+\t\tpack_unlock();\n \t\tif (!trg->data)\n \t\t\tdie(_(\"object %s cannot be read\"),\n \t\t\t    oid_to_hex(&trg_entry->idx.oid));\n@@ -2089,9 +2088,9 @@ static int try_delta(struct unpacked *trg, struct unpacked *src,\n \t\t*mem_usage += sz;\n \t}\n \tif (!src->data) {\n-\t\tread_lock();\n+\t\tpack_lock();\n \t\tsrc->data = read_object_file(&src_entry->idx.oid, &type, &sz);\n-\t\tread_unlock();\n+\t\tpack_unlock();\n \t\tif (!src->data) {\n \t\t\tif (src_entry->preferred_base) {\n \t\t\t\tstatic int warned = 0;\n@@ -2337,9 +2336,9 @@ static void find_deltas(struct object_entry **list, unsigned *list_size,\n \n static void try_to_free_from_threads(size_t size)\n {\n-\tread_lock();\n+\tpack_lock();\n \trelease_pack_memory(size);\n-\tread_unlock();\n+\tpack_unlock();\n }\n \n static try_to_free_t old_try_to_free_routine;\n@@ -2381,7 +2380,6 @@ static pthread_cond_t progress_cond;\n  */\n static void init_threaded_search(void)\n {\n-\tinit_recursive_mutex(&read_mutex);\n \tpthread_mutex_init(&cache_mutex, NULL);\n \tpthread_mutex_init(&progress_mutex, NULL);\n \tpthread_cond_init(&progress_cond, NULL);\n@@ -2392,7 +2390,6 @@ static void cleanup_threaded_search(void)\n {\n \tset_try_to_free_routine(old_try_to_free_routine);\n \tpthread_cond_destroy(&progress_cond);\n-\tpthread_mutex_destroy(&read_mutex);\n \tpthread_mutex_destroy(&cache_mutex);\n \tpthread_mutex_destroy(&progress_mutex);\n }\ndiff --git a/pack-objects.c b/pack-objects.c\nindex b6cdbb016..6f32a7ba0 100644\n--- a/pack-objects.c\n+++ b/pack-objects.c\n@@ -148,9 +148,7 @@ void prepare_packing_data(struct packing_data *pdata)\n \t\t\t\t\t     1U << OE_SIZE_BITS);\n \tpdata->oe_delta_size_limit = git_env_ulong(\"GIT_TEST_OE_DELTA_SIZE\",\n \t\t\t\t\t\t   1UL << OE_DELTA_SIZE_BITS);\n-#ifndef NO_PTHREADS\n-\tpthread_mutex_init(&pdata->lock, NULL);\n-#endif\n+\tinit_recursive_mutex(&pdata->lock);\n }\n \n struct object_entry *packlist_alloc(struct packing_data *pdata,\n-- \n2.20.1.windows.1\n\n"},{"id":"367228","messageId":"CACsJy8AWCP+enBVVVga7jJZ-gxD=fxcushrk0D+xGSRAcZw_qg@mail.gmail.com","threadId":"50266","inReplyTo":"20190119154337.6556-1-phogg@novamoon.net","subject":"Re: [PATCH v2] pack-objects: Use packing_data lock instead of read_mutex","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-01-21T10:02:33Z","receivedAt":"2019-01-21T10:03:01Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Jan 19, 2019 at 10:45 PM Patrick Hogg <phogg@novamoon.net> wrote:\n>\n> ac77d0c37 (\"pack-objects: shrink size field in struct object_entry\",\n> 2018-04-14) added an extra usage of read_lock/read_unlock in the newly\n> introduced oe_get_size_slow for thread safety in parallel calls to\n> try_delta(). Unfortunately oe_get_size_slow is also used in serial\n> code, some of which is called before the first invocation of\n> ll_find_deltas. As such the read mutex is not guaranteed to be\n> initialized.\n>\n> Resolve this by using the existing lock in packing_data which is\n> initialized early in cmd_pack_objects instead of read_mutex.\n> Additionally, upgrade the packing_data lock to a recursive mutex to\n> make it a suitable replacement for read_mutex.\n>\n> Signed-off-by: Patrick Hogg <phogg@novamoon.net>\n> ---\n>\n> As I mentioned in the prior thread I think that it will be simpler\n> to simply use the existing lock in packing_data instead of moving\n> read_mutex. I can go back to simply moving read_mutex to the\n> packing_data struct if that that is preferable, though.\n\nIn early iterations of these changes, I think we hit high contention\nwhen sharing the mutex [1]. I don't know if we will hit the same\nperformance problem again with this patch. It would be great if Elijah\nwith his zillion core machine could test this out. Otherwise it may be\njust safer to keep the two mutexes separate.\n\n[1] http://public-inbox.org/git/20180720052829.GA3852@sigill.intra.peff.net/\n\n>\n> I also removed the #ifndef NO_PTHREADS in prepare_packing_data around\n> the initialization of &pdata->lock since I had to upgrade the lock to\n> a recursive mutex. As far as I can tell init_recursive_mutex (and\n> pthread_mutex_init for that matter) have that protection already so it\n> appears to be redundant.\n>\n>  builtin/pack-objects.c | 27 ++++++++++++---------------\n>  pack-objects.c         |  4 +---\n>  2 files changed, 13 insertions(+), 18 deletions(-)\n>\n> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> index 411aefd68..5439b434c 100644\n> --- a/builtin/pack-objects.c\n> +++ b/builtin/pack-objects.c\n> @@ -1954,9 +1954,8 @@ static int delta_cacheable(unsigned long src_size, unsigned long trg_size,\n>  }\n>\n>  /* Protect access to object database */\n> -static pthread_mutex_t read_mutex;\n> -#define read_lock()            pthread_mutex_lock(&read_mutex)\n> -#define read_unlock()          pthread_mutex_unlock(&read_mutex)\n> +#define pack_lock()            packing_data_lock(&to_pack)\n> +#define pack_unlock()          packing_data_unlock(&to_pack)\n>\n>  /* Protect delta_cache_size */\n>  static pthread_mutex_t cache_mutex;\n> @@ -1993,11 +1992,11 @@ unsigned long oe_get_size_slow(struct packing_data *pack,\n>         unsigned long used, avail, size;\n>\n>         if (e->type_ != OBJ_OFS_DELTA && e->type_ != OBJ_REF_DELTA) {\n> -               read_lock();\n> +               pack_lock();\n>                 if (oid_object_info(the_repository, &e->idx.oid, &size) < 0)\n>                         die(_(\"unable to get size of %s\"),\n>                             oid_to_hex(&e->idx.oid));\n> -               read_unlock();\n> +               pack_unlock();\n>                 return size;\n>         }\n>\n> @@ -2005,7 +2004,7 @@ unsigned long oe_get_size_slow(struct packing_data *pack,\n>         if (!p)\n>                 BUG(\"when e->type is a delta, it must belong to a pack\");\n>\n> -       read_lock();\n> +       pack_lock();\n>         w_curs = NULL;\n>         buf = use_pack(p, &w_curs, e->in_pack_offset, &avail);\n>         used = unpack_object_header_buffer(buf, avail, &type, &size);\n> @@ -2014,7 +2013,7 @@ unsigned long oe_get_size_slow(struct packing_data *pack,\n>                     oid_to_hex(&e->idx.oid));\n>\n>         unuse_pack(&w_curs);\n> -       read_unlock();\n> +       pack_unlock();\n>         return size;\n>  }\n>\n> @@ -2076,9 +2075,9 @@ static int try_delta(struct unpacked *trg, struct unpacked *src,\n>\n>         /* Load data if not already done */\n>         if (!trg->data) {\n> -               read_lock();\n> +               pack_lock();\n>                 trg->data = read_object_file(&trg_entry->idx.oid, &type, &sz);\n> -               read_unlock();\n> +               pack_unlock();\n>                 if (!trg->data)\n>                         die(_(\"object %s cannot be read\"),\n>                             oid_to_hex(&trg_entry->idx.oid));\n> @@ -2089,9 +2088,9 @@ static int try_delta(struct unpacked *trg, struct unpacked *src,\n>                 *mem_usage += sz;\n>         }\n>         if (!src->data) {\n> -               read_lock();\n> +               pack_lock();\n>                 src->data = read_object_file(&src_entry->idx.oid, &type, &sz);\n> -               read_unlock();\n> +               pack_unlock();\n>                 if (!src->data) {\n>                         if (src_entry->preferred_base) {\n>                                 static int warned = 0;\n> @@ -2337,9 +2336,9 @@ static void find_deltas(struct object_entry **list, unsigned *list_size,\n>\n>  static void try_to_free_from_threads(size_t size)\n>  {\n> -       read_lock();\n> +       pack_lock();\n>         release_pack_memory(size);\n> -       read_unlock();\n> +       pack_unlock();\n>  }\n>\n>  static try_to_free_t old_try_to_free_routine;\n> @@ -2381,7 +2380,6 @@ static pthread_cond_t progress_cond;\n>   */\n>  static void init_threaded_search(void)\n>  {\n> -       init_recursive_mutex(&read_mutex);\n>         pthread_mutex_init(&cache_mutex, NULL);\n>         pthread_mutex_init(&progress_mutex, NULL);\n>         pthread_cond_init(&progress_cond, NULL);\n> @@ -2392,7 +2390,6 @@ static void cleanup_threaded_search(void)\n>  {\n>         set_try_to_free_routine(old_try_to_free_routine);\n>         pthread_cond_destroy(&progress_cond);\n> -       pthread_mutex_destroy(&read_mutex);\n>         pthread_mutex_destroy(&cache_mutex);\n>         pthread_mutex_destroy(&progress_mutex);\n>  }\n> diff --git a/pack-objects.c b/pack-objects.c\n> index b6cdbb016..6f32a7ba0 100644\n> --- a/pack-objects.c\n> +++ b/pack-objects.c\n> @@ -148,9 +148,7 @@ void prepare_packing_data(struct packing_data *pdata)\n>                                              1U << OE_SIZE_BITS);\n>         pdata->oe_delta_size_limit = git_env_ulong(\"GIT_TEST_OE_DELTA_SIZE\",\n>                                                    1UL << OE_DELTA_SIZE_BITS);\n> -#ifndef NO_PTHREADS\n> -       pthread_mutex_init(&pdata->lock, NULL);\n> -#endif\n> +       init_recursive_mutex(&pdata->lock);\n>  }\n>\n>  struct object_entry *packlist_alloc(struct packing_data *pdata,\n> --\n> 2.20.1.windows.1\n>\n\n\n-- \nDuy\n"},{"id":"367287","messageId":"20190122072809.GA28975@sigill.intra.peff.net","threadId":"50266","inReplyTo":"CACsJy8AWCP+enBVVVga7jJZ-gxD=fxcushrk0D+xGSRAcZw_qg@mail.gmail.com","subject":"Re: [PATCH v2] pack-objects: Use packing_data lock instead of read_mutex","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-01-22T07:28:09Z","receivedAt":"2019-01-22T07:28:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 21, 2019 at 05:02:33PM +0700, Duy Nguyen wrote:\n\n> > As I mentioned in the prior thread I think that it will be simpler\n> > to simply use the existing lock in packing_data instead of moving\n> > read_mutex. I can go back to simply moving read_mutex to the\n> > packing_data struct if that that is preferable, though.\n> \n> In early iterations of these changes, I think we hit high contention\n> when sharing the mutex [1]. I don't know if we will hit the same\n> performance problem again with this patch. It would be great if Elijah\n> with his zillion core machine could test this out. Otherwise it may be\n> just safer to keep the two mutexes separate.\n> \n> [1] http://public-inbox.org/git/20180720052829.GA3852@sigill.intra.peff.net/\n\nI haven't been following this thread closely, but I still have access to\na 40-core machine if you'd like me to time anything.\n\nIt sounds like _this_ patch is the more fine-grained one. Is the more\ncoarse-grained one already written?\n\n-Peff\n"},{"id":"367298","messageId":"CACsJy8Cdc-AcPDRquxHu-LmHUjzHmR2GEmARbnPcwXgjp8+HMQ@mail.gmail.com","threadId":"50266","inReplyTo":"20190122072809.GA28975@sigill.intra.peff.net","subject":"Re: [PATCH v2] pack-objects: Use packing_data lock instead of read_mutex","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-01-22T10:25:52Z","receivedAt":"2019-01-22T10:26:20Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Jan 22, 2019 at 2:28 PM Jeff King <peff@peff.net> wrote:\n>\n> On Mon, Jan 21, 2019 at 05:02:33PM +0700, Duy Nguyen wrote:\n>\n> > > As I mentioned in the prior thread I think that it will be simpler\n> > > to simply use the existing lock in packing_data instead of moving\n> > > read_mutex. I can go back to simply moving read_mutex to the\n> > > packing_data struct if that that is preferable, though.\n> >\n> > In early iterations of these changes, I think we hit high contention\n> > when sharing the mutex [1]. I don't know if we will hit the same\n> > performance problem again with this patch. It would be great if Elijah\n> > with his zillion core machine could test this out. Otherwise it may be\n> > just safer to keep the two mutexes separate.\n> >\n> > [1] http://public-inbox.org/git/20180720052829.GA3852@sigill.intra.peff.net/\n>\n> I haven't been following this thread closely, but I still have access to\n> a 40-core machine if you'd like me to time anything.\n>\n> It sounds like _this_ patch is the more fine-grained one. Is the more\n> coarse-grained one already written?\n\nA more fine-grained one would be 'master' where we use two separate\nmutexes for different code. I guess if repack performance with this\npatch is still the same as 'master', we're good to go. You may need to\nlower $GIT_TEST_OE_SIZE and $GIT_TEST_OE_DELTA_SIZE to force more lock\ncontention.\n-- \nDuy\n"},{"id":"367300","messageId":"CAFOcBzmYd8gHNHEioFbGJiJ3i1xq0Les=r+irVx-o8gCoyYKKw@mail.gmail.com","threadId":"50266","inReplyTo":"CACsJy8Cdc-AcPDRquxHu-LmHUjzHmR2GEmARbnPcwXgjp8+HMQ@mail.gmail.com","subject":"Re: [PATCH v2] pack-objects: Use packing_data lock instead of read_mutex","fromName":"Patrick Hogg","fromEmail":"phogg@novamoon.net","sentAt":"2019-01-22T13:13:37Z","receivedAt":"2019-01-22T13:14:20Z","isPatch":true,"sender":{"key":"phogg@novamoon.net","avatar":"https://avatars.githubusercontent.com/u/7890416?v=4"},"body":"On Tue, Jan 22, 2019 at 5:26 AM Duy Nguyen <pclouds@gmail.com> wrote:\n>\n> On Tue, Jan 22, 2019 at 2:28 PM Jeff King <peff@peff.net> wrote:\n> >\n> > On Mon, Jan 21, 2019 at 05:02:33PM +0700, Duy Nguyen wrote:\n> >\n> > > > As I mentioned in the prior thread I think that it will be simpler\n> > > > to simply use the existing lock in packing_data instead of moving\n> > > > read_mutex. I can go back to simply moving read_mutex to the\n> > > > packing_data struct if that that is preferable, though.\n> > >\n> > > In early iterations of these changes, I think we hit high contention\n> > > when sharing the mutex [1]. I don't know if we will hit the same\n> > > performance problem again with this patch. It would be great if Elijah\n> > > with his zillion core machine could test this out. Otherwise it may be\n> > > just safer to keep the two mutexes separate.\n> > >\n> > > [1] http://public-inbox.org/git/20180720052829.GA3852@sigill.intra.peff.net/\n> >\n> > I haven't been following this thread closely, but I still have access to\n> > a 40-core machine if you'd like me to time anything.\n> >\n> > It sounds like _this_ patch is the more fine-grained one. Is the more\n> > coarse-grained one already written?\n>\n> A more fine-grained one would be 'master' where we use two separate\n> mutexes for different code. I guess if repack performance with this\n> patch is still the same as 'master', we're good to go. You may need to\n> lower $GIT_TEST_OE_SIZE and $GIT_TEST_OE_DELTA_SIZE to force more lock\n> contention.\n\nI do have a patch prepared which simply moves read_mutex to the\npacking_data struct instead (and renames it read_lock for consistency\nwith the exiting mutex named \"lock\"), but I wanted to wait for the\ntesting regarding lock contention first. I'm prepared either way it\ngoes.\n\n-Patrick\n"},{"id":"367328","messageId":"CABPp-BFmHkf3ftgKxEA5tx_fngPu7WypP_aYyYUvNVmrAibqtw@mail.gmail.com","threadId":"50266","inReplyTo":"CACsJy8AWCP+enBVVVga7jJZ-gxD=fxcushrk0D+xGSRAcZw_qg@mail.gmail.com","subject":"Re: [PATCH v2] pack-objects: Use packing_data lock instead of read_mutex","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-01-22T17:52:49Z","receivedAt":"2019-01-22T17:53:05Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Jan 21, 2019 at 2:02 AM Duy Nguyen <pclouds@gmail.com> wrote:\n>\n> On Sat, Jan 19, 2019 at 10:45 PM Patrick Hogg <phogg@novamoon.net> wrote:\n> >\n> > ac77d0c37 (\"pack-objects: shrink size field in struct object_entry\",\n> > 2018-04-14) added an extra usage of read_lock/read_unlock in the newly\n> > introduced oe_get_size_slow for thread safety in parallel calls to\n> > try_delta(). Unfortunately oe_get_size_slow is also used in serial\n> > code, some of which is called before the first invocation of\n> > ll_find_deltas. As such the read mutex is not guaranteed to be\n> > initialized.\n> >\n> > Resolve this by using the existing lock in packing_data which is\n> > initialized early in cmd_pack_objects instead of read_mutex.\n> > Additionally, upgrade the packing_data lock to a recursive mutex to\n> > make it a suitable replacement for read_mutex.\n> >\n> > Signed-off-by: Patrick Hogg <phogg@novamoon.net>\n> > ---\n> >\n> > As I mentioned in the prior thread I think that it will be simpler\n> > to simply use the existing lock in packing_data instead of moving\n> > read_mutex. I can go back to simply moving read_mutex to the\n> > packing_data struct if that that is preferable, though.\n>\n> In early iterations of these changes, I think we hit high contention\n> when sharing the mutex [1]. I don't know if we will hit the same\n> performance problem again with this patch. It would be great if Elijah\n> with his zillion core machine could test this out. Otherwise it may be\n> just safer to keep the two mutexes separate.\n\nTesting...\n"},{"id":"367355","messageId":"CABPp-BHG24Xv=5hNoBJeYCaJKuUS98bymfCMQVUz+hXnZKGMHA@mail.gmail.com","threadId":"50266","inReplyTo":"CABPp-BFmHkf3ftgKxEA5tx_fngPu7WypP_aYyYUvNVmrAibqtw@mail.gmail.com","subject":"Re: [PATCH v2] pack-objects: Use packing_data lock instead of read_mutex","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-01-22T20:37:11Z","receivedAt":"2019-01-22T20:37:25Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Duy, Patrick,\n\nOn Tue, Jan 22, 2019 at 9:52 AM Elijah Newren <newren@gmail.com> wrote:\n>\n> On Mon, Jan 21, 2019 at 2:02 AM Duy Nguyen <pclouds@gmail.com> wrote:\n> >\n> > On Sat, Jan 19, 2019 at 10:45 PM Patrick Hogg <phogg@novamoon.net> wrote:\n> > >\n> > > ac77d0c37 (\"pack-objects: shrink size field in struct object_entry\",\n> > > 2018-04-14) added an extra usage of read_lock/read_unlock in the newly\n> > > introduced oe_get_size_slow for thread safety in parallel calls to\n> > > try_delta(). Unfortunately oe_get_size_slow is also used in serial\n> > > code, some of which is called before the first invocation of\n> > > ll_find_deltas. As such the read mutex is not guaranteed to be\n> > > initialized.\n> > >\n> > > Resolve this by using the existing lock in packing_data which is\n> > > initialized early in cmd_pack_objects instead of read_mutex.\n> > > Additionally, upgrade the packing_data lock to a recursive mutex to\n> > > make it a suitable replacement for read_mutex.\n> > >\n> > > Signed-off-by: Patrick Hogg <phogg@novamoon.net>\n> > > ---\n> > >\n> > > As I mentioned in the prior thread I think that it will be simpler\n> > > to simply use the existing lock in packing_data instead of moving\n> > > read_mutex. I can go back to simply moving read_mutex to the\n> > > packing_data struct if that that is preferable, though.\n> >\n> > In early iterations of these changes, I think we hit high contention\n> > when sharing the mutex [1]. I don't know if we will hit the same\n> > performance problem again with this patch. It would be great if Elijah\n> > with his zillion core machine could test this out. Otherwise it may be\n> > just safer to keep the two mutexes separate.\n>\n\nBefore this patch, repacking an old clone of linux.git on a 40-core\nbox (an m4.10xlarge) using the command\n   /usr/bin/time -f 'MaxRSS:%M Time:%e' git gc --aggressive\nbased on commit 16a465bc01 (\"Third batch after 2.20\", 2019-01-18), resulted in:\n\nMaxRSS:10959072 Time:650.63\n\nSince that was a cold cache, might have had loose objects and what\nnot, I threw that one out and ran three more times:\n\nMaxRSS: 9886284 Time:571.40\nMaxRSS:10441716 Time:566.47\nMaxRSS:10157536 Time:569.79\n\nThen I applied this patch and ran three more times:\n\nMaxRSS:10611444 Time:574.60\nMaxRSS:10429732 Time:571.48\nMaxRSS:10657160 Time:575.58\n\n(Yeah, we talked about MaxRSS not being the most useful last time, but\nI'm just copying the command I used last time for consistency; I'm\nonly really paying attention to Time.)\n\nSo, it's within 1% of the timing of the current version.  Seems fine to me.\n\n\nHope that helps,\nElijah\n"},{"id":"367387","messageId":"xmqq1s54e2ju.fsf@gitster-ct.c.googlers.com","threadId":"50266","inReplyTo":"20190119154337.6556-1-phogg@novamoon.net","subject":"Re: [PATCH v2] pack-objects: Use packing_data lock instead of read_mutex","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-22T22:43:33Z","receivedAt":"2019-01-22T22:43:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Hogg <phogg@novamoon.net> writes:\n\n> As I mentioned in the prior thread I think that it will be simpler\n> to simply use the existing lock in packing_data instead of moving\n> read_mutex. I can go back to simply moving read_mutex to the\n> packing_data struct if that that is preferable, though.\n\nI'll let others comment on this to show preference between the two\napproaches.\n\n> I also removed the #ifndef NO_PTHREADS in prepare_packing_data around\n> the initialization of &pdata->lock since I had to upgrade the lock to\n> a recursive mutex. As far as I can tell init_recursive_mutex (and\n> pthread_mutex_init for that matter) have that protection already so it\n> appears to be redundant.\n\nIf you can defer \"I also\" to a separate patch, please do so.\nKeeping the fix alone as small as possible and not tangled with\nother changes would make it easier for people to cherry-pick the fix\nto older maintenance tracks if they choose to.\n"},{"id":"367396","messageId":"CAFOcBz==BunV48U94ZSnc2gmBTOcOpY4XO-dPugKMUVNZGjzZA@mail.gmail.com","threadId":"50266","inReplyTo":"xmqq1s54e2ju.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2] pack-objects: Use packing_data lock instead of read_mutex","fromName":"Patrick Hogg","fromEmail":"phogg@novamoon.net","sentAt":"2019-01-22T23:54:24Z","receivedAt":"2019-01-22T23:55:07Z","isPatch":true,"sender":{"key":"phogg@novamoon.net","avatar":"https://avatars.githubusercontent.com/u/7890416?v=4"},"body":"On Tue, Jan 22, 2019 at 5:43 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Patrick Hogg <phogg@novamoon.net> writes:\n>\n> > As I mentioned in the prior thread I think that it will be simpler\n> > to simply use the existing lock in packing_data instead of moving\n> > read_mutex. I can go back to simply moving read_mutex to the\n> > packing_data struct if that that is preferable, though.\n>\n> I'll let others comment on this to show preference between the two\n> approaches.\n>\n> > I also removed the #ifndef NO_PTHREADS in prepare_packing_data around\n> > the initialization of &pdata->lock since I had to upgrade the lock to\n> > a recursive mutex. As far as I can tell init_recursive_mutex (and\n> > pthread_mutex_init for that matter) have that protection already so it\n> > appears to be redundant.\n>\n> If you can defer \"I also\" to a separate patch, please do so.\n> Keeping the fix alone as small as possible and not tangled with\n> other changes would make it easier for people to cherry-pick the fix\n> to older maintenance tracks if they choose to.\n\nThat's a fair point.  To confirm (as I'm rather new to submitting git\npatches), do you mean to submit a two-patch series or to just leave\nout the #ifndef removal altogether for now?\n\nIf this does become a two patch series I could simply move the\nread_mutex to packing_data in the first patch and merge the two\nmutexes (and remove the #ifndef) in the second.  That would keep the\nfix alone even smaller (just the first patch) to simplify\ncherry-picking.\n\n(There is also the option of going back to the v1 change and\ncorrecting the cleanup in the early return.)\n\nI just want to confirm preferences first before submitting a v3 to\navoid spamming patches; I'll go whichever way the experts think is\nbest.\n\nThanks,\n-Patrick\n"},{"id":"367445","messageId":"xmqqh8dzcle7.fsf@gitster-ct.c.googlers.com","threadId":"50266","inReplyTo":"CAFOcBz==BunV48U94ZSnc2gmBTOcOpY4XO-dPugKMUVNZGjzZA@mail.gmail.com","subject":"Re: [PATCH v2] pack-objects: Use packing_data lock instead of read_mutex","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-23T17:51:44Z","receivedAt":"2019-01-23T17:51:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Hogg <phogg@novamoon.net> writes:\n\n> On Tue, Jan 22, 2019 at 5:43 PM Junio C Hamano <gitster@pobox.com> wrote:\n>>\n>> Patrick Hogg <phogg@novamoon.net> writes:\n>>\n>> > As I mentioned in the prior thread I think that it will be simpler\n>> > to simply use the existing lock in packing_data instead of moving\n>> > read_mutex. I can go back to simply moving read_mutex to the\n>> > packing_data struct if that that is preferable, though.\n>>\n>> I'll let others comment on this to show preference between the two\n>> approaches.\n>>\n>> > I also removed the #ifndef NO_PTHREADS in prepare_packing_data around\n>> > the initialization of &pdata->lock since I had to upgrade the lock to\n>> > a recursive mutex. As far as I can tell init_recursive_mutex (and\n>> > pthread_mutex_init for that matter) have that protection already so it\n>> > appears to be redundant.\n>>\n>> If you can defer \"I also\" to a separate patch, please do so.\n>> Keeping the fix alone as small as possible and not tangled with\n>> other changes would make it easier for people to cherry-pick the fix\n>> to older maintenance tracks if they choose to.\n>\n> That's a fair point.  To confirm (as I'm rather new to submitting git\n> patches), do you mean to submit a two-patch series or to just leave\n> out the #ifndef removal altogether for now?\n\nEither would work ;-)\n\n> If this does become a two patch series I could simply move the\n> read_mutex to packing_data in the first patch and merge the two\n> mutexes (and remove the #ifndef) in the second.  That would keep the\n> fix alone even smaller (just the first patch) to simplify\n> cherry-picking.\n>\n> (There is also the option of going back to the v1 change and\n> correcting the cleanup in the early return.)\n\nYes, but as I said, I'll let others show their preferences between\napproaches v1/v2.\n"}]}