{"thread":{"id":"49636","subject":"[PATCH v1] load_cache_entries_threaded: remove unused src_offset parameter","startedAt":"2018-10-22T15:05:26Z","lastAt":"2018-10-23T20:07:34Z","messageCount":5,"participants":["Ben Peart","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"361166","messageId":"20181022150513.18028-1-peartben@gmail.com","threadId":"49636","inReplyTo":null,"subject":"[PATCH v1] load_cache_entries_threaded: remove unused src_offset parameter","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2018-10-22T15:05:13Z","receivedAt":"2018-10-22T15:05:26Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"From: Ben Peart <benpeart@microsoft.com>\n\nRemove the src_offset parameter which is unused as a result of switching\nto the IEOT table of offsets.  Also stop incrementing src_offset in the\nmulti-threaded codepath as it is no longer used and could cause confusion.\n\nSigned-off-by: Ben Peart <benpeart@microsoft.com>\n---\n\nNotes:\n    Base Ref:\n    Web-Diff: https://github.com/benpeart/git/commit/7360721408\n    Checkout: git fetch https://github.com/benpeart/git read-index-multithread-no-src-offset-v1 && git checkout 7360721408\n\n read-cache.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex f9fa6a7979..6db6f0f220 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2037,7 +2037,7 @@ static void *load_cache_entries_thread(void *_data)\n }\n \n static unsigned long load_cache_entries_threaded(struct index_state *istate, const char *mmap, size_t mmap_size,\n-\t\t\tunsigned long src_offset, int nr_threads, struct index_entry_offset_table *ieot)\n+\t\t\tint nr_threads, struct index_entry_offset_table *ieot)\n {\n \tint i, offset, ieot_blocks, ieot_start, err;\n \tstruct load_cache_entries_thread_data *data;\n@@ -2198,7 +2198,7 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)\n \t\tieot = read_ieot_extension(mmap, mmap_size, extension_offset);\n \n \tif (ieot) {\n-\t\tsrc_offset += load_cache_entries_threaded(istate, mmap, mmap_size, src_offset, nr_threads, ieot);\n+\t\tload_cache_entries_threaded(istate, mmap, mmap_size, nr_threads, ieot);\n \t\tfree(ieot);\n \t} else {\n \t\tsrc_offset += load_all_cache_entries(istate, mmap, mmap_size, src_offset);\n\nbase-commit: f58b85df6937e3f3d9ef26bb52a513c8ada17ffc\n-- \n2.18.0.windows.1\n\n"},{"id":"361180","messageId":"20181022201721.GD9917@sigill.intra.peff.net","threadId":"49636","inReplyTo":"20181022150513.18028-1-peartben@gmail.com","subject":"Re: [PATCH v1] load_cache_entries_threaded: remove unused src_offset parameter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-10-22T20:17:21Z","receivedAt":"2018-10-22T20:17:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 22, 2018 at 11:05:13AM -0400, Ben Peart wrote:\n\n> From: Ben Peart <benpeart@microsoft.com>\n> \n> Remove the src_offset parameter which is unused as a result of switching\n> to the IEOT table of offsets.  Also stop incrementing src_offset in the\n> multi-threaded codepath as it is no longer used and could cause confusion.\n\nHmm, OK. We only do threads if we have ieot:\n\n>  \tif (ieot) {\n> -\t\tsrc_offset += load_cache_entries_threaded(istate, mmap, mmap_size, src_offset, nr_threads, ieot);\n> +\t\tload_cache_entries_threaded(istate, mmap, mmap_size, nr_threads, ieot);\n>  \t\tfree(ieot);\n>  \t} else {\n>  \t\tsrc_offset += load_all_cache_entries(istate, mmap, mmap_size, src_offset);\n\nAnd we only have ieot if we had an extension_offset:\n\n          if (extension_offset && nr_threads > 1)\n                  ieot = read_ieot_extension(mmap, mmap_size, extension_offset);\n\nSo later, when we _do_ use src_offset, we know that this code should\nnever trigger in the threaded case:\n\n          if (!extension_offset) {\n                  p.src_offset = src_offset;\n                  load_index_extensions(&p);\n          }\n\nSo I think it's right, but it's rather subtle. I wonder if we could do\nit like this:\n\n\tunsigned long entry_offset;\n  [...]\n  #ifndef NO_PTHREADS\n\tif (ieot)\n\t\tload_cache_entries_threaded(...);\n\telse\n\t\tentry_offset = load_all_cache_entries(...);\n  #else\n\tentry_offset = load_all_cache_entries(...);\n  [...]\n\n  p.src_offset = src_offset + entry_offset;\n\nand then the compiler could warn us that entry_offset is used\nuninitialized (though I would not be surprised if the compiler gets\nconfused in this case).\n\nNot sure if it is worth the trouble or not.\n\n\n>  static unsigned long load_cache_entries_threaded(struct index_state *istate, const char *mmap, size_t mmap_size,\n> -\t\t\tunsigned long src_offset, int nr_threads, struct index_entry_offset_table *ieot)\n> +\t\t\tint nr_threads, struct index_entry_offset_table *ieot)\n\nIf nobody uses it, should we drop the return value, too? Like:\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 78c9516eb7..4b44a2eae5 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2052,12 +2052,11 @@ static void *load_cache_entries_thread(void *_data)\n \treturn NULL;\n }\n \n-static unsigned long load_cache_entries_threaded(struct index_state *istate, const char *mmap, size_t mmap_size,\n-\t\t\t\t\t\t int nr_threads, struct index_entry_offset_table *ieot)\n+static void load_cache_entries_threaded(struct index_state *istate, const char *mmap, size_t mmap_size,\n+\t\t\t\t\tint nr_threads, struct index_entry_offset_table *ieot)\n {\n \tint i, offset, ieot_blocks, ieot_start, err;\n \tstruct load_cache_entries_thread_data *data;\n-\tunsigned long consumed = 0;\n \n \t/* a little sanity checking */\n \tif (istate->name_hash_initialized)\n@@ -2115,12 +2114,9 @@ static unsigned long load_cache_entries_threaded(struct index_state *istate, con\n \t\tif (err)\n \t\t\tdie(_(\"unable to join load_cache_entries thread: %s\"), strerror(err));\n \t\tmem_pool_combine(istate->ce_mem_pool, p->ce_mem_pool);\n-\t\tconsumed += p->consumed;\n \t}\n \n \tfree(data);\n-\n-\treturn consumed;\n }\n #endif\n \n\n-Peff\n"},{"id":"361228","messageId":"xmqqo9bltwdy.fsf@gitster-ct.c.googlers.com","threadId":"49636","inReplyTo":"20181022201721.GD9917@sigill.intra.peff.net","subject":"Re: [PATCH v1] load_cache_entries_threaded: remove unused src_offset parameter","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-22T23:05:45Z","receivedAt":"2018-10-22T23:05:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> If nobody uses it, should we drop the return value, too? Like:\n\nYup.\n\n>\n> diff --git a/read-cache.c b/read-cache.c\n> index 78c9516eb7..4b44a2eae5 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -2052,12 +2052,11 @@ static void *load_cache_entries_thread(void *_data)\n>  \treturn NULL;\n>  }\n>  \n> -static unsigned long load_cache_entries_threaded(struct index_state *istate, const char *mmap, size_t mmap_size,\n> -\t\t\t\t\t\t int nr_threads, struct index_entry_offset_table *ieot)\n> +static void load_cache_entries_threaded(struct index_state *istate, const char *mmap, size_t mmap_size,\n> +\t\t\t\t\tint nr_threads, struct index_entry_offset_table *ieot)\n>  {\n>  \tint i, offset, ieot_blocks, ieot_start, err;\n>  \tstruct load_cache_entries_thread_data *data;\n> -\tunsigned long consumed = 0;\n>  \n>  \t/* a little sanity checking */\n>  \tif (istate->name_hash_initialized)\n> @@ -2115,12 +2114,9 @@ static unsigned long load_cache_entries_threaded(struct index_state *istate, con\n>  \t\tif (err)\n>  \t\t\tdie(_(\"unable to join load_cache_entries thread: %s\"), strerror(err));\n>  \t\tmem_pool_combine(istate->ce_mem_pool, p->ce_mem_pool);\n> -\t\tconsumed += p->consumed;\n>  \t}\n>  \n>  \tfree(data);\n> -\n> -\treturn consumed;\n>  }\n>  #endif\n>  \n>\n> -Peff\n"},{"id":"361315","messageId":"7a359876-7d36-5d01-5f47-76ef316b6386@gmail.com","threadId":"49636","inReplyTo":"xmqqo9bltwdy.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v1] load_cache_entries_threaded: remove unused src_offset parameter","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2018-10-23T19:13:06Z","receivedAt":"2018-10-23T19:13:11Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"\n\nOn 10/22/2018 7:05 PM, Junio C Hamano wrote:\n> Jeff King <peff@peff.net> writes:\n> \n>> If nobody uses it, should we drop the return value, too? Like:\n> \n> Yup.\n> \n\nI'm good with that.\n\nAt one point I also had the additional #ifndef NO_PTHREADS lines but it \nwas starting to get messy with the threaded vs non-threaded code paths \nso I removed them.  I'm fine with which ever people find more readable.\n\nIt does make me wonder if there are still platforms taking new build of \ngit that don't support threads.  Do we still need to \nwrite/test/debug/read through the single threaded code paths?\n\nIs the diff Peff sent enough or do you want me to send another iteration \non the patch?\n\nThanks,\n\nBen\n\n>>\n>> diff --git a/read-cache.c b/read-cache.c\n>> index 78c9516eb7..4b44a2eae5 100644\n>> --- a/read-cache.c\n>> +++ b/read-cache.c\n>> @@ -2052,12 +2052,11 @@ static void *load_cache_entries_thread(void *_data)\n>>   \treturn NULL;\n>>   }\n>>   \n>> -static unsigned long load_cache_entries_threaded(struct index_state *istate, const char *mmap, size_t mmap_size,\n>> -\t\t\t\t\t\t int nr_threads, struct index_entry_offset_table *ieot)\n>> +static void load_cache_entries_threaded(struct index_state *istate, const char *mmap, size_t mmap_size,\n>> +\t\t\t\t\tint nr_threads, struct index_entry_offset_table *ieot)\n>>   {\n>>   \tint i, offset, ieot_blocks, ieot_start, err;\n>>   \tstruct load_cache_entries_thread_data *data;\n>> -\tunsigned long consumed = 0;\n>>   \n>>   \t/* a little sanity checking */\n>>   \tif (istate->name_hash_initialized)\n>> @@ -2115,12 +2114,9 @@ static unsigned long load_cache_entries_threaded(struct index_state *istate, con\n>>   \t\tif (err)\n>>   \t\t\tdie(_(\"unable to join load_cache_entries thread: %s\"), strerror(err));\n>>   \t\tmem_pool_combine(istate->ce_mem_pool, p->ce_mem_pool);\n>> -\t\tconsumed += p->consumed;\n>>   \t}\n>>   \n>>   \tfree(data);\n>> -\n>> -\treturn consumed;\n>>   }\n>>   #endif\n>>   \n>>\n>> -Peff\n"},{"id":"361324","messageId":"20181023200730.GB15214@sigill.intra.peff.net","threadId":"49636","inReplyTo":"7a359876-7d36-5d01-5f47-76ef316b6386@gmail.com","subject":"Re: [PATCH v1] load_cache_entries_threaded: remove unused src_offset parameter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-10-23T20:07:30Z","receivedAt":"2018-10-23T20:07:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 23, 2018 at 03:13:06PM -0400, Ben Peart wrote:\n\n> At one point I also had the additional #ifndef NO_PTHREADS lines but it was\n> starting to get messy with the threaded vs non-threaded code paths so I\n> removed them.  I'm fine with which ever people find more readable.\n> \n> It does make me wonder if there are still platforms taking new build of git\n> that don't support threads.  Do we still need to write/test/debug/read\n> through the single threaded code paths?\n\nI think the classic offenders here were old Unix systems like AIX, etc.\n\nI've no idea what the current state is on those platforms. I would love\nit if we could drop NO_PTHREADS. There's a lot of gnarly code there, and\nI strongly suspect a lot of bugs lurk in the non-threaded halves (e.g.,\nespecially around bits like \"struct async\" which is \"maybe a thread, and\nmaybe a fork\" depending on your system, which introduces all kinds of\nsubtle process-state dependencies).\n\nBut I'm not really sure how to find out aside from adding a deprecation\nwarning and seeing if anybody screams.\n\nSee also this RFC from Duy, which might at least make the code itself a\nlittle easier to follow:\n\n\thttps://public-inbox.org/git/20181018180522.17642-1-pclouds@gmail.com/\n\n-Peff\n"}]}