{"thread":{"id":"62605","subject":"git add -p is slow for sparse checkout","startedAt":"2024-12-06T03:46:24Z","lastAt":"2024-12-09T08:18:55Z","messageCount":4,"participants":["Manoraj K","Shubham Kanodia"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"508703","messageId":"CACfnds=++C75pg8ojn5sJzjKzLcgnyPxgR2c6pOPCRmf2rDKvQ@mail.gmail.com","threadId":"62605","inReplyTo":null,"subject":"git add -p is slow for sparse checkout","fromName":"Manoraj K","fromEmail":"mkenchugonde@atlassian.com","sentAt":"2024-12-06T03:46:10Z","receivedAt":"2024-12-06T03:46:24Z","isPatch":false,"sender":{"key":"mkenchugonde@atlassian.com","avatar":null},"body":"Hi team,\n\nI am currently on a sparse checkout repository, and it looks like the\n`git add—p` command is much slower than in a full checkout repository.\n\nWhen run, the `git add—p` command expands to the full index and takes\nmore time to start showing the patch.\n\nAlso, we have git trace2 enabled, and the timings of the subcommands\ngave us an indication that the `git apply—-cached` subcommand is\ntaking way too long, with an average P50 of 3000ms against 300ms in\nthe full checkout repository. The other notable spikes are in `git\ndiff-files --color` (180ms vs 80ms) and `git diff-files --no-color`\n(180ms vs 80ms).\n\nI am really interested in understanding why `git add -p` expands to a\nfull index, which I believe is the issue with start-up regression.\nAlso, would be great to understand the difference in performance of\nthe subcommands.\n"},{"id":"508706","messageId":"CACfnds=J=Q+viagTEoiQ3edUxU8GSY_s7=biGUGY4NKbQpsTvw@mail.gmail.com","threadId":"62605","inReplyTo":"CACfnds=++C75pg8ojn5sJzjKzLcgnyPxgR2c6pOPCRmf2rDKvQ@mail.gmail.com","subject":"Re: git add -p is slow for sparse checkout","fromName":"Manoraj K","fromEmail":"mkenchugonde@atlassian.com","sentAt":"2024-12-06T04:23:03Z","receivedAt":"2024-12-06T04:23:16Z","isPatch":false,"sender":{"key":"mkenchugonde@atlassian.com","avatar":null},"body":"++ CC\n\nOn Fri, Dec 6, 2024 at 9:16 AM Manoraj K <mkenchugonde@atlassian.com> wrote:\n>\n> Hi team,\n>\n> I am currently on a sparse checkout repository, and it looks like the\n> `git add—p` command is much slower than in a full checkout repository.\n>\n> When run, the `git add—p` command expands to the full index and takes\n> more time to start showing the patch.\n>\n> Also, we have git trace2 enabled, and the timings of the subcommands\n> gave us an indication that the `git apply—-cached` subcommand is\n> taking way too long, with an average P50 of 3000ms against 300ms in\n> the full checkout repository. The other notable spikes are in `git\n> diff-files --color` (180ms vs 80ms) and `git diff-files --no-color`\n> (180ms vs 80ms).\n>\n> I am really interested in understanding why `git add -p` expands to a\n> full index, which I believe is the issue with start-up regression.\n> Also, would be great to understand the difference in performance of\n> the subcommands.\n"},{"id":"508711","messageId":"CAARMkVGYOKb0H3pmUdcDWTs2p3hZZMX0dsdU1rT=Hg5a6M7Z6g@mail.gmail.com","threadId":"62605","inReplyTo":"CACfnds=J=Q+viagTEoiQ3edUxU8GSY_s7=biGUGY4NKbQpsTvw@mail.gmail.com","subject":"Re: git add -p is slow for sparse checkout","fromName":"Shubham Kanodia","fromEmail":"skanodia@atlassian.com","sentAt":"2024-12-06T08:43:08Z","receivedAt":"2024-12-06T08:43:20Z","isPatch":false,"sender":{"key":"skanodia@atlassian.com","avatar":null},"body":"On Fri, Dec 6, 2024 at 9:53 AM Manoraj K <mkenchugonde@atlassian.com> wrote:\n>\n> ++ CC\n>\n> On Fri, Dec 6, 2024 at 9:16 AM Manoraj K <mkenchugonde@atlassian.com> wrote:\n> >\n> > Hi team,\n> >\n> > I am currently on a sparse checkout repository, and it looks like the\n> > `git add—p` command is much slower than in a full checkout repository.\n> >\n> > When run, the `git add—p` command expands to the full index and takes\n> > more time to start showing the patch.\n> >\n> > Also, we have git trace2 enabled, and the timings of the subcommands\n> > gave us an indication that the `git apply—-cached` subcommand is\n> > taking way too long, with an average P50 of 3000ms against 300ms in\n> > the full checkout repository. The other notable spikes are in `git\n> > diff-files --color` (180ms vs 80ms) and `git diff-files --no-color`\n> > (180ms vs 80ms).\n> >\n> > I am really interested in understanding why `git add -p` expands to a\n> > full index, which I believe is the issue with start-up regression.\n> > Also, would be great to understand the difference in performance of\n> > the subcommands.\n\nI started tracing execution of `git add -p`, it seems that `git add\n-p` seems to require a\nfull index whereas a regular `git add` does not, because `git add -p`\nends up calling\n `do_read_index` in `read_cache.c`.\n\n\n`do_read_index` seems to then call `prepare_repo_settings` that resets\nthe value of\n`istate->repo.settings.command_requires_full_index` to `1`, even though\n`istate->repo.settings.command_requires_full_index` passed to\n`do_read_index` was\ninitially `0` for `git add -p`.\n\n```\nint do_read_index(struct index_state *istate, const char *path, int must_exist)\n{\n....\n/*\n* If the command explicitly requires a full index, force it\n* to be full. Otherwise, correct the sparsity based on repository\n* settings and other properties of the index (if necessary).\n*/\nprepare_repo_settings(istate->repo);\n```\n\nThe comment here seems confusing to me — it seems like the intent here\nwas to preserve\nsparseness when as possible, but `prepare_repo_settings` seemsto do a\nblanket reset —\n\n```\nvoid prepare_repo_settings(struct repository *r)\n{\n....\n/*\n* This setting guards all index reads to require a full index\n* over a sparse index. After suitable guards are placed in the\n* codebase around uses of the index, this setting will be\n* removed.\n*/\nr->settings.command_requires_full_index = 1;\n```\n\nRetaining the original setting seems to fix the problem and makes `git\nadd -p` fast,\nhowever I don't know if there's a reason why it wasn't already done this way.\n\n```\nread-cache.c | 5 +++++\n1 file changed, 5 insertions(+)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 01d0b3ad22..92f7561a88 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2339,7 +2339,12 @@ int do_read_index(struct index_state *istate,\nconst char *path, int must_exist)\n* to be full. Otherwise, correct the sparsity based on repository\n* settings and other properties of the index (if necessary).\n*/\n+ int previous_full_index_setting =\n+ istate->repo->settings.command_requires_full_index;\n+\nprepare_repo_settings(istate->repo);\n+ istate->repo->settings.command_requires_full_index =\nprevious_full_index_setting;\n+\nif (istate->repo->settings.command_requires_full_index)\nensure_full_index(istate);\nelse\n```\n"},{"id":"508851","messageId":"CAARMkVGxduJZnxBAK9smL4JnBcUZ44tOsH7m-WgSa3ymtATt1w@mail.gmail.com","threadId":"62605","inReplyTo":"CAARMkVGYOKb0H3pmUdcDWTs2p3hZZMX0dsdU1rT=Hg5a6M7Z6g@mail.gmail.com","subject":"Re: git add -p is slow for sparse checkout","fromName":"Shubham Kanodia","fromEmail":"skanodia@atlassian.com","sentAt":"2024-12-09T08:18:43Z","receivedAt":"2024-12-09T08:18:55Z","isPatch":false,"sender":{"key":"skanodia@atlassian.com","avatar":null},"body":"On Fri, Dec 6, 2024 at 2:13 PM Shubham Kanodia <skanodia@atlassian.com> wrote:\n>\n> On Fri, Dec 6, 2024 at 9:53 AM Manoraj K <mkenchugonde@atlassian.com> wrote:\n> >\n> > ++ CC\n> >\n> > On Fri, Dec 6, 2024 at 9:16 AM Manoraj K <mkenchugonde@atlassian.com> wrote:\n> > >\n> > > Hi team,\n> > >\n> > > I am currently on a sparse checkout repository, and it looks like the\n> > > `git add—p` command is much slower than in a full checkout repository.\n> > >\n> > > When run, the `git add—p` command expands to the full index and takes\n> > > more time to start showing the patch.\n> > >\n> > > Also, we have git trace2 enabled, and the timings of the subcommands\n> > > gave us an indication that the `git apply—-cached` subcommand is\n> > > taking way too long, with an average P50 of 3000ms against 300ms in\n> > > the full checkout repository. The other notable spikes are in `git\n> > > diff-files --color` (180ms vs 80ms) and `git diff-files --no-color`\n> > > (180ms vs 80ms).\n> > >\n> > > I am really interested in understanding why `git add -p` expands to a\n> > > full index, which I believe is the issue with start-up regression.\n> > > Also, would be great to understand the difference in performance of\n> > > the subcommands.\n>\n> I started tracing execution of `git add -p`, it seems that `git add\n> -p` seems to require a\n> full index whereas a regular `git add` does not, because `git add -p`\n> ends up calling\n>  `do_read_index` in `read_cache.c`.\n>\n>\n> `do_read_index` seems to then call `prepare_repo_settings` that resets\n> the value of\n> `istate->repo.settings.command_requires_full_index` to `1`, even though\n> `istate->repo.settings.command_requires_full_index` passed to\n> `do_read_index` was\n> initially `0` for `git add -p`.\n>\n> ```\n> int do_read_index(struct index_state *istate, const char *path, int must_exist)\n> {\n> ....\n> /*\n> * If the command explicitly requires a full index, force it\n> * to be full. Otherwise, correct the sparsity based on repository\n> * settings and other properties of the index (if necessary).\n> */\n> prepare_repo_settings(istate->repo);\n> ```\n>\n> The comment here seems confusing to me — it seems like the intent here\n> was to preserve\n> sparseness when as possible, but `prepare_repo_settings` seemsto do a\n> blanket reset —\n>\n> ```\n> void prepare_repo_settings(struct repository *r)\n> {\n> ....\n> /*\n> * This setting guards all index reads to require a full index\n> * over a sparse index. After suitable guards are placed in the\n> * codebase around uses of the index, this setting will be\n> * removed.\n> */\n> r->settings.command_requires_full_index = 1;\n> ```\n>\n> Retaining the original setting seems to fix the problem and makes `git\n> add -p` fast,\n> however I don't know if there's a reason why it wasn't already done this way.\n>\n> ```\n> read-cache.c | 5 +++++\n> 1 file changed, 5 insertions(+)\n>\n> diff --git a/read-cache.c b/read-cache.c\n> index 01d0b3ad22..92f7561a88 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -2339,7 +2339,12 @@ int do_read_index(struct index_state *istate,\n> const char *path, int must_exist)\n> * to be full. Otherwise, correct the sparsity based on repository\n> * settings and other properties of the index (if necessary).\n> */\n> + int previous_full_index_setting =\n> + istate->repo->settings.command_requires_full_index;\n> +\n> prepare_repo_settings(istate->repo);\n> + istate->repo->settings.command_requires_full_index =\n> previous_full_index_setting;\n> +\n> if (istate->repo->settings.command_requires_full_index)\n> ensure_full_index(istate);\n> else\n> ```\n\nAdding Derick — in case you know if indeed a non-sparse index is\nnecessary for `git add -p` or if this could just have been an\noversight given support for sparse checkouts was added progressively\nto git.\n\n-- \nRegards,\nShubham Kanodia\n"}]}