{"thread":{"id":"47291","subject":"[PATCH] defer expensive load_ref_decorations until needed","startedAt":"2017-11-21T23:44:15Z","lastAt":"2017-11-23T04:00:27Z","messageCount":9,"participants":["Phil Hord","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"333199","messageId":"20171121234336.10209-1-phil.hord@gmail.com","threadId":"47291","inReplyTo":null,"subject":"[PATCH] defer expensive load_ref_decorations until needed","fromName":"Phil Hord","fromEmail":"phil.hord@gmail.com","sentAt":"2017-11-21T23:43:36Z","receivedAt":"2017-11-21T23:44:15Z","isPatch":true,"sender":{"key":"phil.hord@gmail.com","avatar":"https://avatars.githubusercontent.com/u/123908?v=4"},"body":"With many thousands of references, a simple `git rev-parse HEAD` may take\nmore than a second to return because it first loads all the refs into\nmemory even though it will never use them.\n\nDefer loading any references until we actually need them.\n\nSigned-off-by: Phil Hord <phil.hord@gmail.com>\n---\n log-tree.c | 9 +++++++--\n 1 file changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/log-tree.c b/log-tree.c\nindex 3b904f037..c1509f8b9 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -84,8 +84,10 @@ void add_name_decoration(enum decoration_type type, const char *name, struct obj\n \tres->next = add_decoration(&name_decoration, obj, res);\n }\n \n+static void maybe_load_ref_decorations();\n const struct name_decoration *get_name_decoration(const struct object *obj)\n {\n+\tmaybe_load_ref_decorations();\n \treturn lookup_decoration(&name_decoration, obj);\n }\n \n@@ -150,10 +152,13 @@ static int add_graft_decoration(const struct commit_graft *graft, void *cb_data)\n \n void load_ref_decorations(int flags)\n {\n-\tif (!decoration_loaded) {\n+\tdecoration_flags = flags;\n+}\n \n+static void maybe_load_ref_decorations()\n+{\n+\tif (!decoration_loaded) {\n \t\tdecoration_loaded = 1;\n-\t\tdecoration_flags = flags;\n \t\tfor_each_ref(add_ref_decoration, NULL);\n \t\thead_ref(add_ref_decoration, NULL);\n \t\tfor_each_commit_graft(add_graft_decoration, NULL);\n-- \n2.15.0.471.g17a719cfe.dirty\n\n"},{"id":"333241","messageId":"xmqqbmjuvrab.fsf@gitster.mtv.corp.google.com","threadId":"47291","inReplyTo":"20171121234336.10209-1-phil.hord@gmail.com","subject":"Re: [PATCH] defer expensive load_ref_decorations until needed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-22T05:03:24Z","receivedAt":"2017-11-22T05:03:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phil Hord <phil.hord@gmail.com> writes:\n\n> With many thousands of references, a simple `git rev-parse HEAD` may take\n> more than a second to return because it first loads all the refs into\n> memory even though it will never use them.\n>\n> Defer loading any references until we actually need them.\n>\n> Signed-off-by: Phil Hord <phil.hord@gmail.com>\n> ---\n>  log-tree.c | 9 +++++++--\n>  1 file changed, 7 insertions(+), 2 deletions(-)\n>\n> diff --git a/log-tree.c b/log-tree.c\n> index 3b904f037..c1509f8b9 100644\n> --- a/log-tree.c\n> +++ b/log-tree.c\n> @@ -84,8 +84,10 @@ void add_name_decoration(enum decoration_type type, const char *name, struct obj\n>  \tres->next = add_decoration(&name_decoration, obj, res);\n>  }\n>  \n> +static void maybe_load_ref_decorations();\n\nI'll tweak that \"()\" and the other one we see below to \"(void)\"\nwhile queuing.\n\nI am not sure if \"maybe_\" is a good name here, though.  If anything,\nyou are making the semantics of \"load_ref_decorations()\" to \"maybe\"\n(but I do not suggest renaming that one).\n\nHow about calling it to load_ref_decorations_lazily() or something?\n\nI also wonder if decoration_loaded should now become function-scope\nstatic in this new helper, but that can be left outside of the\ntopic.\n\nOther than that, I like what this patch attempts to do.  A nicely\nidentified low-hanging fruit ;-).\n\nThanks.\n\n>  const struct name_decoration *get_name_decoration(const struct object *obj)\n>  {\n> +\tmaybe_load_ref_decorations();\n>  \treturn lookup_decoration(&name_decoration, obj);\n>  }\n>  \n> @@ -150,10 +152,13 @@ static int add_graft_decoration(const struct commit_graft *graft, void *cb_data)\n>  \n>  void load_ref_decorations(int flags)\n>  {\n> -\tif (!decoration_loaded) {\n> +\tdecoration_flags = flags;\n> +}\n>  \n> +static void maybe_load_ref_decorations()\n> +{\n> +\tif (!decoration_loaded) {\n>  \t\tdecoration_loaded = 1;\n> -\t\tdecoration_flags = flags;\n>  \t\tfor_each_ref(add_ref_decoration, NULL);\n>  \t\thead_ref(add_ref_decoration, NULL);\n>  \t\tfor_each_commit_graft(add_graft_decoration, NULL);\n"},{"id":"333249","messageId":"xmqqk1yiu9fo.fsf@gitster.mtv.corp.google.com","threadId":"47291","inReplyTo":"xmqqbmjuvrab.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] defer expensive load_ref_decorations until needed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-22T06:14:19Z","receivedAt":"2017-11-22T06:14:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Other than that, I like what this patch attempts to do.  A nicely\n> identified low-hanging fruit ;-).\n\nHaving said that, this will have a bad interaction with another\ntopic in flight: <20171121213341.13939-1-rafa.almas@gmail.com>\n\nPerhaps this should wait until the other topic lands and stabilizes.\nWe'd need to rethink if the approach taken by this patch, i.e. to\nstill pass the info to load() but holding onto it until the time\nlazy_load() actually uses it, is a sensible way forward, or we would\nwant to change the calling convention to help making it easier to\nimplement the lazy loading.\n\nThanks.\n\n\n\n"},{"id":"333304","messageId":"CABURp0rNfdXaXH-meRvM+mjf+ucHKfePjDU7ZGKt0ug3wOanhA@mail.gmail.com","threadId":"47291","inReplyTo":"xmqqk1yiu9fo.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] defer expensive load_ref_decorations until needed","fromName":"Phil Hord","fromEmail":"phil.hord@gmail.com","sentAt":"2017-11-22T17:45:21Z","receivedAt":"2017-11-22T17:45:48Z","isPatch":true,"sender":{"key":"phil.hord@gmail.com","avatar":"https://avatars.githubusercontent.com/u/123908?v=4"},"body":"On Tue, Nov 21, 2017, Junio C Hamano <gitster@pobox.com> wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n> I am not sure if \"maybe_\" is a good name here, though.  If anything,\n> you are making the semantics of \"load_ref_decorations()\" to \"maybe\"\n> (but I do not suggest renaming that one).\n>\n> How about calling it to load_ref_decorations_lazily() or something?\n\nI groped about for something conventional, but \"..._gently\" didn't fit\nthe bill, so I went with \"maybe\".  I like \"lazily\" better for this\ncase. I will change it for v2.\n\n>> Other than that, I like what this patch attempts to do.  A nicely\n>> identified low-hanging fruit ;-).\n>\n> Having said that, this will have a bad interaction with another\n> topic in flight: <20171121213341.13939-1-rafa.almas@gmail.com>\n>\n> Perhaps this should wait until the other topic lands and stabilizes.\n> We'd need to rethink if the approach taken by this patch, i.e. to\n> still pass the info to load() but holding onto it until the time\n> lazy_load() actually uses it, is a sensible way forward, or we would\n> want to change the calling convention to help making it easier to\n> implement the lazy loading.\n\nI noticed that after just after cleaning this one up, but didn't look\nclosely yet.  I'll hold this in my local queue until ra lands.\n\nP\n"},{"id":"333325","messageId":"20171122212710.GB2854@sigill","threadId":"47291","inReplyTo":"20171121234336.10209-1-phil.hord@gmail.com","subject":"Re: [PATCH] defer expensive load_ref_decorations until needed","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-11-22T21:27:11Z","receivedAt":"2017-11-22T21:27:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 21, 2017 at 03:43:36PM -0800, Phil Hord wrote:\n\n> With many thousands of references, a simple `git rev-parse HEAD` may take\n> more than a second to return because it first loads all the refs into\n> memory even though it will never use them.\n\nThe overall goal of lazy-loading seems reasonable, but I'm slightly\nconfused: how and why does \"git rev-parse HEAD\" load ref decorations?\n\nGrepping around I find that we mostly load them only when appropriate\n(when the \"log\" family sees a decorate option, when we see %d/%D in a\npretty format, or with --simplify-by-decoration in a traversal). And\npoking at \"rev-parse HEAD\" in gdb seems to confirm that it does not hit\nthat function.\n\nI have definitely seen \"rev-parse HEAD\" be O(# of refs), but that is\nmostly attributable to having all the refs packed (and until v2.15.0,\nthe packed-refs code would read the whole file into memory). I've also\nseen unnecessary ref lookups due to replace refs (we load al of the\npacked refs to find out that no, there's nothing in refs/replace).\n\n-Peff\n"},{"id":"333347","messageId":"CABURp0rq9pwFWuBbrSB-FNUQ6B-7V8uL=Drw6O1-151u_cRKww@mail.gmail.com","threadId":"47291","inReplyTo":"20171122212710.GB2854@sigill","subject":"Re: [PATCH] defer expensive load_ref_decorations until needed","fromName":"Phil Hord","fromEmail":"phil.hord@gmail.com","sentAt":"2017-11-22T23:21:06Z","receivedAt":"2017-11-22T23:21:39Z","isPatch":true,"sender":{"key":"phil.hord@gmail.com","avatar":"https://avatars.githubusercontent.com/u/123908?v=4"},"body":"On Wed, Nov 22, 2017 at 1:27 PM, Jeff King <peff@peff.net> wrote:\n> On Tue, Nov 21, 2017 at 03:43:36PM -0800, Phil Hord wrote:\n>\n>> With many thousands of references, a simple `git rev-parse HEAD` may take\n>> more than a second to return because it first loads all the refs into\n>> memory even though it will never use them.\n>\n> The overall goal of lazy-loading seems reasonable, but I'm slightly\n> confused: how and why does \"git rev-parse HEAD\" load ref decorations?\n>\n> Grepping around I find that we mostly load them only when appropriate\n> (when the \"log\" family sees a decorate option, when we see %d/%D in a\n> pretty format, or with --simplify-by-decoration in a traversal). And\n> poking at \"rev-parse HEAD\" in gdb seems to confirm that it does not hit\n> that function.\n\nHm. I think I was confused.\n\nI wrote v1 of this patch a few months ago. Clearly I was wrong about\nrev-parse being afflicted.  We have a script that was suffering and it\nuses both \"git log --format=%h\" and \"git rev-parse\" to get hashes; I\nremember testing both, but I can't find it in my $zsh_history; my\nmemory and my commit-message must be faulty.\n\nHowever, \"git log\" does not need any --decorate option to trigger this lag.\n\n    $ git for-each-ref| wc -l\n    24172\n    $ time git log --format=%h -1\n    git log --format=%h -1   0.47s user 0.04s system 99% cpu 0.509 total\n\nI grepped the code just now, too, and I see the same as you, though;\nit seems to hold off unless !!decoration_style.  Nevertheless, gdb\nshows me decoration_style=1 with this command:\n\n    GIT_CONFIG=/dev/null cgdb --args git log -1 --format=\"%h\"\n\nHere are timing tests on this repo without this change:\n\n    git log --format=%h -1             0.54s user 0.05s system 99% cpu\n0.597 total\n    git log --format=%h -1 --decorate  0.54s user 0.04s system 98% cpu\n0.590 total\n    git log --format=%h%d -1           0.53s user 0.05s system 99% cpu\n0.578 total\n\nAnd the same commands with this change:\n\n    git log --format=%h -1              0.01s user 0.01s system 71%\ncpu 0.017 total\n    git log --format=%h -1 --decorate   0.00s user 0.01s system 92%\ncpu 0.009 total\n    git log --format=%h%d -1            0.53s user 0.09s system 88%\ncpu 0.699 total\n\n> I have definitely seen \"rev-parse HEAD\" be O(# of refs), but that is\n> mostly attributable to having all the refs packed (and until v2.15.0,\n> the packed-refs code would read the whole file into memory).\n\nHm.  Could this be why rev-parse was slow for me?  My original problem\nshowed up on v1.9 (build machine) and I patched it on v2.14.0-rc1.\nBut, no; testing on 1.9, 2.11 and 2.14 still doesn't show me the lag\nin rev-parse.  I remain befuddled.\n\n> I've also\n> seen unnecessary ref lookups due to replace refs (we load al of the\n> packed refs to find out that no, there's nothing in refs/replace).\n\nI haven't seen this in the code, but I have had refs/replace hacks in\nthe past. Is that enough to wake this up?\n\nPhil\n"},{"id":"333355","messageId":"20171122234841.GD8577@sigill","threadId":"47291","inReplyTo":"CABURp0rq9pwFWuBbrSB-FNUQ6B-7V8uL=Drw6O1-151u_cRKww@mail.gmail.com","subject":"Re: [PATCH] defer expensive load_ref_decorations until needed","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-11-22T23:48:41Z","receivedAt":"2017-11-22T23:48:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 22, 2017 at 03:21:06PM -0800, Phil Hord wrote:\n\n> Hm. I think I was confused.\n> \n> I wrote v1 of this patch a few months ago. Clearly I was wrong about\n> rev-parse being afflicted.  We have a script that was suffering and it\n> uses both \"git log --format=%h\" and \"git rev-parse\" to get hashes; I\n> remember testing both, but I can't find it in my $zsh_history; my\n> memory and my commit-message must be faulty.\n\nOK, that makes more sense (that log would see it).\n\n> However, \"git log\" does not need any --decorate option to trigger this lag.\n> \n>     $ git for-each-ref| wc -l\n>     24172\n>     $ time git log --format=%h -1\n>     git log --format=%h -1   0.47s user 0.04s system 99% cpu 0.509 total\n>\n> I grepped the code just now, too, and I see the same as you, though;\n> it seems to hold off unless !!decoration_style.  Nevertheless, gdb\n> shows me decoration_style=1 with this command:\n> \n>     GIT_CONFIG=/dev/null cgdb --args git log -1 --format=\"%h\"\n> \n\nRight, the default these days is \"auto decorate\", so it's enabled if\nyour output is to a terminal. So \"git log --no-decorate\" should be cheap\nagain (or you may want to set log.decorate=false in your config).\n\nAnd lazy-load wouldn't help you there for a normal:\n\n  git log\n\nBut what's interesting in your command is the pretty-format. Even though\ndecoration is turned on, your format doesn't show any. So we never\nactually ask \"is this commit decorated\" and the lazy-load helps.\n\nSo I think your patch is doing the right thing, but the explanation\nshould probably cover that it is really helping non-decorating formats.\n\n> Here are timing tests on this repo without this change:\n> \n>     git log --format=%h -1             0.54s user 0.05s system 99% cpu\n> 0.597 total\n>     git log --format=%h -1 --decorate  0.54s user 0.04s system 98% cpu\n> 0.590 total\n>     git log --format=%h%d -1           0.53s user 0.05s system 99% cpu\n> 0.578 total\n> \n> And the same commands with this change:\n> \n>     git log --format=%h -1              0.01s user 0.01s system 71%\n> cpu 0.017 total\n>     git log --format=%h -1 --decorate   0.00s user 0.01s system 92%\n> cpu 0.009 total\n>     git log --format=%h%d -1            0.53s user 0.09s system 88%\n> cpu 0.699 total\n\nYeah, that's consistent with what I'd expect.\n\n> > I have definitely seen \"rev-parse HEAD\" be O(# of refs), but that is\n> > mostly attributable to having all the refs packed (and until v2.15.0,\n> > the packed-refs code would read the whole file into memory).\n> \n> Hm.  Could this be why rev-parse was slow for me?  My original problem\n> showed up on v1.9 (build machine) and I patched it on v2.14.0-rc1.\n> But, no; testing on 1.9, 2.11 and 2.14 still doesn't show me the lag\n> in rev-parse.  I remain befuddled.\n\nDoing \"rev-parse HEAD\" would still have to load the packed refs if the\nthing that HEAD points to is in there. Perhaps your current HEAD is\ndetached, or you have a loose ref for the current branch? Try \"git\npack-refs --all --prune\" and then re-time.\n\n-Peff\n"},{"id":"333366","messageId":"xmqqy3mxrb29.fsf@gitster.mtv.corp.google.com","threadId":"47291","inReplyTo":"20171122234841.GD8577@sigill","subject":"Re: [PATCH] defer expensive load_ref_decorations until needed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-23T02:19:42Z","receivedAt":"2017-11-23T02:19:48Z","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> And lazy-load wouldn't help you there for a normal:\n>\n>   git log\n>\n> But what's interesting in your command is the pretty-format. Even though\n> decoration is turned on, your format doesn't show any. So we never\n> actually ask \"is this commit decorated\" and the lazy-load helps.\n\nHmph, I wonder if we can detect this case and not make a call to\nload decorations in the first place.  That would remove the need to\nremember the options when load is called so that we can use it when\nwe load decorations lazily later.\n"},{"id":"333371","messageId":"20171123040018.GA21706@sigill","threadId":"47291","inReplyTo":"xmqqy3mxrb29.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] defer expensive load_ref_decorations until needed","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-11-23T04:00:19Z","receivedAt":"2017-11-23T04:00:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 23, 2017 at 11:19:42AM +0900, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > And lazy-load wouldn't help you there for a normal:\n> >\n> >   git log\n> >\n> > But what's interesting in your command is the pretty-format. Even though\n> > decoration is turned on, your format doesn't show any. So we never\n> > actually ask \"is this commit decorated\" and the lazy-load helps.\n> \n> Hmph, I wonder if we can detect this case and not make a call to\n> load decorations in the first place.  That would remove the need to\n> remember the options when load is called so that we can use it when\n> we load decorations lazily later.\n\nProbably userformat_find_requirements() could (and should) be taught to\nreport on decoration flags, like it does for notes. As it is now we call\nload_ref_decorations() repeatedly while processing the commits (which\nworks because it's a noop after the first call).\n\nAnd once we can do that, it would be easy to do something like:\n\n  if (decoration_style && !want->decorations)\n\tdecoration_style = 0;\n\nBut I think that may only be part of the story. Do all of the output\nformats show decorations? I think --format=email, for instance, does\nnot.\n\nSo the real question is not just \"does the user format want\ndecorations\", but \"does the pretty format want decorations\". Which\nrequires knowing things about each format that might get out of sync\nwith the rest of the code. That might be OK if it lives in pretty.c. But\nthe lazy-load thing make sit just work without having to duplicate the\nlogic at all.\n\n-Peff\n"}]}