{"thread":{"id":"20104","subject":"Fix extraneous lstat's in 'git checkout -f'","startedAt":"2009-07-14T04:01:59Z","lastAt":"2009-07-14T21:19:10Z","messageCount":5,"participants":["Linus Torvalds","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"117931","messageId":"alpine.LFD.2.01.0907132040530.13838@localhost.localdomain","threadId":"20104","inReplyTo":null,"subject":"Fix extraneous lstat's in 'git checkout -f'","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2009-07-14T04:01:59Z","receivedAt":"2009-07-14T04:01:59Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\nIn our 'oneway_merge()' we always do an 'lstat()' to see if we might need \nto mark the entry for updating. \n\nWe really shouldn't need to do that when the cache entry is already marked \nas being ce_uptodate(). However, since this function will actually match \nthe lstat() information with the CE_MATCH_IGNORE_VALID, we need to be a \nbit more careful: it's not sufficient to check ce_uptodate(), we need to \nalso double-check that the ce_uptodate() isn't because of CE_VALID (which \nwill be \"uptodate\" regardless of whether the lstat() matches or not).\n\nThis explains why it's not sufficient to check _just_ the CE_UPTODATE bit.\n\nBut if both CE_UPTODATE is set, and CE_VALID is clear, then we know that \nthe lstat() will always match the cache entry information, and there's no \npoint in doing another one.\n\nSigned-off-by: Linus Torvalds <torvalds@linux-foundation.org>\n---\n\nQuite frankly, I'd like for us to at least think about removing CE_VALID. \n\nDoes anybody use it (and \"core.ignorestat\" that turns it on, along with \nthe \"--assume-unchanged\" flag to update-index)\n\nI wonder if we have other places where we have optimized away the lstat() \njust because we decided that it was already up-to-date - without \nrealizing that something could have been marked up-to-date just because \nit was marked CE_VALID.\n\nThe original reasoning behind CE_VALID was to avoid the cost of lstat's on \noperating systems where it is slow, but we are now so good at optimizing \nthem away (thanks to CE_UPTODATE) that I wonder whether it's not better to \njust always rely on CE_UPTODATE.\n\nIn fact, a quick grep made me just look at \"verify_uptodate()\", and how it \ndoes ie_match_stat(CE_MATCH_IGNORE_VALID). But look at how we've already \nshort-circuited this if ce_uptodate() is true, and we never get there if \nwe've preloaded the index or done any other operation that might have \nalready validated the entry?\n\nI dunno. This at least doesn't make things worse, since I started thinking \nabout this _after_ having first done a patch that just did the \n\"ce_uptodate()\" test.\n\n unpack-trees.c |   14 +++++++++++++-\n 1 files changed, 13 insertions(+), 1 deletions(-)\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex f9d12aa..9920a6f 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -985,6 +985,18 @@ int bind_merge(struct cache_entry **src,\n }\n \n /*\n+ * Do we _know_ the stat information will match?\n+ *\n+ * Note that if CE_VALID is set, then we might have a\n+ * uptodate cache entry even if the stat info might not\n+ * match.\n+ */\n+static inline int known_uptodate(struct cache_entry *ce)\n+{\n+\treturn ce_uptodate(ce) && !(ce->ce_flags & CE_VALID);\n+}\n+\n+/*\n  * One-way merge.\n  *\n  * The rule is:\n@@ -1004,7 +1016,7 @@ int oneway_merge(struct cache_entry **src, struct unpack_trees_options *o)\n \n \tif (old && same(old, a)) {\n \t\tint update = 0;\n-\t\tif (o->reset) {\n+\t\tif (o->reset && !known_uptodate(old)) {\n \t\t\tstruct stat st;\n \t\t\tif (lstat(old->name, &st) ||\n \t\t\t    ie_match_stat(o->src_index, old, &st, CE_MATCH_IGNORE_VALID))\n"},{"id":"117932","messageId":"alpine.LFD.2.01.0907132107130.13838@localhost.localdomain","threadId":"20104","inReplyTo":"alpine.LFD.2.01.0907132040530.13838@localhost.localdomain","subject":"Re: Fix extraneous lstat's in 'git checkout -f'","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2009-07-14T04:18:04Z","receivedAt":"2009-07-14T04:18:04Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Mon, 13 Jul 2009, Linus Torvalds wrote:\n> \n> But if both CE_UPTODATE is set, and CE_VALID is clear, then we know that \n> the lstat() will always match the cache entry information, and there's no \n> point in doing another one.\n\nPerf data on the kernel tree and my machine with index preloading:\n\n - run-of-ten \"git checkout -f\" before:\n\treal\t0m0.131s\n\treal\t0m0.130s\n\treal\t0m0.128s\n\treal\t0m0.127s\n\treal\t0m0.129s\n\treal\t0m0.135s\n\treal\t0m0.129s\n\treal\t0m0.126s\n\treal\t0m0.128s\n\treal\t0m0.127s\n\n - run-of-ten \"git checkout -f\" after:\n\treal\t0m0.105s\n\treal\t0m0.098s\n\treal\t0m0.098s\n\treal\t0m0.116s\n\treal\t0m0.099s\n\treal\t0m0.104s\n\treal\t0m0.097s\n\treal\t0m0.107s\n\treal\t0m0.105s\n\treal\t0m0.101s\n\nso this makes about a 25% performance difference.\n\nHOWEVER! Without preloading, there is bsically no difference what-so-ever, \nbecause the index won't have been preloaded (duh), so the CE_UPTODATE bit \nwon't be set, so we'll do the lstat() in that oneway_merge() function \nregardless.\n\nS for that non-preloading case, I get\n\n - run-of-ten \"git checkout -f\" without preloading:\n\treal\t0m0.115s\n\treal\t0m0.116s\n\treal\t0m0.117s\n\treal\t0m0.118s\n\treal\t0m0.120s\n\treal\t0m0.116s\n\treal\t0m0.110s\n\treal\t0m0.110s\n\treal\t0m0.119s\n\treal\t0m0.117s\n\nso what happens is that with that patch the preloading is finally actually \nfaster, because now it really helps do the lstat() in parallel (and I have \nlots of cores).\n\nBefore the patch, it would do the lstat first in parallel, but then it \nwould do _another_ lstat() in the serial code anyway due to the silly \noneway_merge() thing wasting time on the lstat() even if it didn't need \nto.\n\nI guess I should have made this issue with preloading clear in the commit \nmessage. \n\nIn other words - the patch should be _much_ more noticeable if you're \ndoing something like NFS and are using core.preloadindex=true. My numbers \nabove are all hot-cache, so there's no IO, just the fast multi-core CPU.\n\n\t\t\tLinus\n"},{"id":"117944","messageId":"7vzlb7hc7y.fsf@alter.siamese.dyndns.org","threadId":"20104","inReplyTo":"alpine.LFD.2.01.0907132040530.13838@localhost.localdomain","subject":"Re: Fix extraneous lstat's in 'git checkout -f'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-07-14T08:59:13Z","receivedAt":"2009-07-14T08:59:13Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@linux-foundation.org> writes:\n\n> Quite frankly, I'd like for us to at least think about removing CE_VALID. \n\nThis is heavy.  I personally do not use this flag, nor know anybody who\ndoes, but deviating from the original purpose of CE_VALID, which was to\navoid lstat() on slow filesystems, people have advised/advocated its use\nfor \"narrow checkout\".  These people may not even have a file checked out\nto the CE_VALID path in the work tree, and they depend on us not running\nlstat() on them and instead always answering that the work tree has the\nnecessary blob.\n\n> I wonder if we have other places where we have optimized away the lstat() \n> just because we decided that it was already up-to-date - without \n> realizing that something could have been marked up-to-date just because \n> it was marked CE_VALID.\n\nThat is a very valid concern, but I think fixing them may break the\n\"narrow checkout\" people.\n\nWe may need to add ce_uptodate(ce) check instead of doing lstat() in\nsome places (like the one you modified in this patch), not because we want\nto avoid lstat(), but because we do not want to lstat() paths that are\nmarked as CE_VALID.\n\nThere are some mechanisms, such as REFRESH_REALLY flag, to give an escape\nhatch to break out of the CE_VALID illusion, but I have to admit that when\nwe did CE_VALID we did not quite clarified its ramifications with respect\nto merging and branch switching.\n"},{"id":"117965","messageId":"alpine.LFD.2.01.0907140750290.13838@localhost.localdomain","threadId":"20104","inReplyTo":"7vzlb7hc7y.fsf@alter.siamese.dyndns.org","subject":"Re: Fix extraneous lstat's in 'git checkout -f'","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2009-07-14T14:50:56Z","receivedAt":"2009-07-14T14:50:56Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Tue, 14 Jul 2009, Junio C Hamano wrote:\n\n> Linus Torvalds <torvalds@linux-foundation.org> writes:\n> \n> > Quite frankly, I'd like for us to at least think about removing CE_VALID. \n> \n> This is heavy.  I personally do not use this flag, nor know anybody who\n> does, but deviating from the original purpose of CE_VALID, which was to\n> avoid lstat() on slow filesystems, people have advised/advocated its use\n> for \"narrow checkout\".\n\nAhh. I kind of was aware of that, but had totally forgotten.\n\nAnd in that case, I guess it's also fine. In fact, for that case CE_VALID \nwould tend to really mean \"always assume CE_UPTODATE\", so then the patch I \nsent out doesn't really necessarily need the whole \"known_uptodate()\" \nthing, and could just use\n\n\tif (o->reset && !ce_valid(old)) {\n\ninstead.\n\nWhich also makes my other worries go away.\n\n\t\t\tLinus\n"},{"id":"117984","messageId":"alpine.LFD.2.01.0907141417160.13838@localhost.localdomain","threadId":"20104","inReplyTo":"alpine.LFD.2.01.0907140750290.13838@localhost.localdomain","subject":"[PATCH v2] Fix extraneous lstat's in 'git checkout -f'","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2009-07-14T21:19:10Z","receivedAt":"2009-07-14T21:19:10Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nIn our 'oneway_merge()' we always do an 'lstat()' to see if we might\nneed to mark the entry for updating.\n\nBut we really shouldn't need to do that when the cache entry is already\nmarked as being ce_uptodate(), and this makes us do unnecessary lstat()\ncalls if we have index preloading enabled.\n\nSigned-off-by: Linus Torvalds <torvalds@linux-foundation.org>\n---\n\nAs Junio noticed, I meant \"ce_uptodate()\", not \"ce_valid()\", but this is \nbasically the simplified version that replaces the one that cared about \nCE_VALID.\n\nOn Tue, 14 Jul 2009, Linus Torvalds wrote:\n> \n> And in that case, I guess it's also fine. In fact, for that case CE_VALID \n> would tend to really mean \"always assume CE_UPTODATE\", so then the patch I \n> sent out doesn't really necessarily need the whole \"known_uptodate()\" \n> thing, and could just use\n> \n> \tif (o->reset && !ce_valid(old)) {\n> \n> instead.\n> \n> Which also makes my other worries go away.\n> \n> \t\t\tLinus\n> \n\n unpack-trees.c |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex f9d12aa..48d862d 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -1004,7 +1004,7 @@ int oneway_merge(struct cache_entry **src, struct unpack_trees_options *o)\n \n \tif (old && same(old, a)) {\n \t\tint update = 0;\n-\t\tif (o->reset) {\n+\t\tif (o->reset && !ce_uptodate(old)) {\n \t\t\tstruct stat st;\n \t\t\tif (lstat(old->name, &st) ||\n \t\t\t    ie_match_stat(o->src_index, old, &st, CE_MATCH_IGNORE_VALID))\n"}]}