{"thread":{"id":"55542","subject":"[PATCH] cache-tree: avoid needless promisor object fetch","startedAt":"2021-04-23T03:17:07Z","lastAt":"2021-04-26T19:49:34Z","messageCount":3,"participants":["Jonathan Tan","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"422708","messageId":"20210423031659.2362659-1-jonathantanmy@google.com","threadId":"55542","inReplyTo":null,"subject":"[PATCH] cache-tree: avoid needless promisor object fetch","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-04-23T03:16:59Z","receivedAt":"2021-04-23T03:17:07Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"In update_one() (used only by cache_tree_update()), there is an object\nexistence check that, if it fails, will automatically trigger a lazy\nfetch in a partial clone. But the fetch is not necessary - the object is\nnot actually being used.\n\nReplace that check with two checks: an object existence check that does\nnot fetch, and then a check that that object is a promisor object.\n\nDoing this avoids multiple lazy fetches when merging two trees in a\npartial clone, as noticed at $DAYJOB.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\nAnother alternative is to think about whether the object existence check\nhere is needed in the first place.\n\nThere might also be other places we can make a similar change in\nupdate_one(), but I limited myself to what's needed to solve the\nspecific case we discovered at $DAYJOB.\n---\n cache-tree.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/cache-tree.c b/cache-tree.c\nindex add1f07713..6728722597 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -6,6 +6,7 @@\n #include \"object-store.h\"\n #include \"replace-object.h\"\n #include \"promisor-remote.h\"\n+#include \"packfile.h\"\n \n #ifndef DEBUG_CACHE_TREE\n #define DEBUG_CACHE_TREE 0\n@@ -362,7 +363,9 @@ static int update_one(struct cache_tree *it,\n \t\t\t(has_promisor_remote() &&\n \t\t\t ce_skip_worktree(ce));\n \t\tif (is_null_oid(oid) ||\n-\t\t    (!ce_missing_ok && !has_object_file(oid))) {\n+\t\t    (!ce_missing_ok &&\n+\t\t     !has_object_file_with_flags(oid, OBJECT_INFO_SKIP_FETCH_OBJECT) &&\n+\t\t     !is_promisor_object(oid))) {\n \t\t\tstrbuf_release(&buffer);\n \t\t\tif (expected_missing)\n \t\t\t\treturn -1;\n-- \n2.31.1.498.g6c1eba8ee3d-goog\n\n"},{"id":"422749","messageId":"nycvar.QRO.7.76.6.2104231409500.54@tvgsbejvaqbjf.bet","threadId":"55542","inReplyTo":"20210423031659.2362659-1-jonathantanmy@google.com","subject":"Re: [PATCH] cache-tree: avoid needless promisor object fetch","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-04-23T12:28:13Z","receivedAt":"2021-04-23T12:28:20Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Jonathan,\n\nOn Thu, 22 Apr 2021, Jonathan Tan wrote:\n\n> In update_one() (used only by cache_tree_update()), there is an object\n> existence check that, if it fails, will automatically trigger a lazy\n> fetch in a partial clone. But the fetch is not necessary - the object is\n> not actually being used.\n\nI find it curious, though, that the `ce_missing_ok` variable is defined\nthusly (sadly, the context of your diff is too small to show it):\n\n                ce_missing_ok = mode == S_IFGITLINK || missing_ok ||\n                        (has_promisor_remote() &&\n                         ce_skip_worktree(ce));\n\nWhich means that the `has_object_file()` function is only called if the\nentry is not marked with the `skip-worktree` bit, i.e. if it is _not_\nexcluded from the sparse checkout.\n\nWouldn't that mean that the object _should_ be there?\n\nI guess what I am saying is that while the commit message focuses on the\n\"What?\" of the patch, I would love to hear more about the \"Why?\". And\nmaybe the \"When?\" as in: when does this actually matter?\n\nAnd since the bug was critical enough for you to spend time on crafting\nit, maybe it would make sense to add a regression test to ensure that this\nbug does not creep in again?\n\n>\n> Replace that check with two checks: an object existence check that does\n> not fetch, and then a check that that object is a promisor object.\n\nThis essentially repeats what the diff says, but it might make more sense\nto explain why the post-image of this diff is more correct (and maybe\ndiscuss performance implications).\n\n>\n> Doing this avoids multiple lazy fetches when merging two trees in a\n> partial clone, as noticed at $DAYJOB.\n\nAh. But where are those trees fetched, then?\n\nMaybe lead with the description of the bug?\n\n> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>\n> ---\n> Another alternative is to think about whether the object existence check\n> here is needed in the first place.\n>\n> There might also be other places we can make a similar change in\n> update_one(), but I limited myself to what's needed to solve the\n> specific case we discovered at $DAYJOB.\n\nI only see another `has_object_file()` call site at the very beginning,\nand I think this needs to fetch. Or maybe it is more efficient to\nconstruct the cache tree from scratch than fetch it?\n\nThere is also `cache_tree_fully_valid_1()`, where I think the same\nhandling could potentially make sense. (Or, if you target `seen`,\n`cache_tree_fully_valid()`.\n\nCiao,\nJohannes\n\n> ---\n>  cache-tree.c | 5 ++++-\n>  1 file changed, 4 insertions(+), 1 deletion(-)\n>\n> diff --git a/cache-tree.c b/cache-tree.c\n> index add1f07713..6728722597 100644\n> --- a/cache-tree.c\n> +++ b/cache-tree.c\n> @@ -6,6 +6,7 @@\n>  #include \"object-store.h\"\n>  #include \"replace-object.h\"\n>  #include \"promisor-remote.h\"\n> +#include \"packfile.h\"\n>\n>  #ifndef DEBUG_CACHE_TREE\n>  #define DEBUG_CACHE_TREE 0\n> @@ -362,7 +363,9 @@ static int update_one(struct cache_tree *it,\n>  \t\t\t(has_promisor_remote() &&\n>  \t\t\t ce_skip_worktree(ce));\n>  \t\tif (is_null_oid(oid) ||\n> -\t\t    (!ce_missing_ok && !has_object_file(oid))) {\n> +\t\t    (!ce_missing_ok &&\n> +\t\t     !has_object_file_with_flags(oid, OBJECT_INFO_SKIP_FETCH_OBJECT) &&\n> +\t\t     !is_promisor_object(oid))) {\n>  \t\t\tstrbuf_release(&buffer);\n>  \t\t\tif (expected_missing)\n>  \t\t\t\treturn -1;\n> --\n> 2.31.1.498.g6c1eba8ee3d-goog\n>\n>\n"},{"id":"423030","messageId":"20210426194928.326338-1-jonathantanmy@google.com","threadId":"55542","inReplyTo":"nycvar.QRO.7.76.6.2104231409500.54@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] cache-tree: avoid needless promisor object fetch","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-04-26T19:49:28Z","receivedAt":"2021-04-26T19:49:34Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> > In update_one() (used only by cache_tree_update()), there is an object\n> > existence check that, if it fails, will automatically trigger a lazy\n> > fetch in a partial clone. But the fetch is not necessary - the object is\n> > not actually being used.\n> \n> I find it curious, though, that the `ce_missing_ok` variable is defined\n> thusly (sadly, the context of your diff is too small to show it):\n> \n>                 ce_missing_ok = mode == S_IFGITLINK || missing_ok ||\n>                         (has_promisor_remote() &&\n>                          ce_skip_worktree(ce));\n> \n> Which means that the `has_object_file()` function is only called if the\n> entry is not marked with the `skip-worktree` bit, i.e. if it is _not_\n> excluded from the sparse checkout.\n> \n> Wouldn't that mean that the object _should_ be there?\n\nIn a partial clone, probably not?\n\n> I guess what I am saying is that while the commit message focuses on the\n> \"What?\" of the patch, I would love to hear more about the \"Why?\". And\n> maybe the \"When?\" as in: when does this actually matter?\n\nIn this case, that's something I'd like help in figuring out too.\nNormally this code path (unpack_trees()) prefetches everything through a\ncall to check_updates(), but the update flag is somehow not set so there\nis no prefetching happening.\n\n> And since the bug was critical enough for you to spend time on crafting\n> it, maybe it would make sense to add a regression test to ensure that this\n> bug does not creep in again?\n\nOK.\n\n> > Replace that check with two checks: an object existence check that does\n> > not fetch, and then a check that that object is a promisor object.\n> \n> This essentially repeats what the diff says, but it might make more sense\n> to explain why the post-image of this diff is more correct (and maybe\n> discuss performance implications).\n\nOK - I think this is the \"why\" and \"when\" you described above.\n\n> > Doing this avoids multiple lazy fetches when merging two trees in a\n> > partial clone, as noticed at $DAYJOB.\n> \n> Ah. But where are those trees fetched, then?\n> \n> Maybe lead with the description of the bug?\n\nThis was a partial clone excluding blobs only. I'll update the commit\nmessage to mention this detail.\n\n> > Another alternative is to think about whether the object existence check\n> > here is needed in the first place.\n> >\n> > There might also be other places we can make a similar change in\n> > update_one(), but I limited myself to what's needed to solve the\n> > specific case we discovered at $DAYJOB.\n> \n> I only see another `has_object_file()` call site at the very beginning,\n> and I think this needs to fetch. Or maybe it is more efficient to\n> construct the cache tree from scratch than fetch it?\n\nGood point - if we can construct it, we probably shouldn't fetch it.\n\n> There is also `cache_tree_fully_valid_1()`, where I think the same\n> handling could potentially make sense. (Or, if you target `seen`,\n> `cache_tree_fully_valid()`.\n\nTrue.\n"}]}