read-cache: call verify_hdr() in a background thread
Subsystems (2)
v3 cover read-cache: call verify_hdr() in a background thread
From: Jeff Hostetler <[email protected]>
Version 3 of this patch series simplifies this effort to just turn on/off the hash verification using a "core.checksumindex" config variable.
I've preserved the original checksum validation code so that we can force it on in fsck if desired.
It eliminates the original threading model completely.
Jeff Hostetler (2):
read-cache: core.checksumindex
test-core-checksum-index: core.checksumindex test helper
Makefile | 1 + read-cache.c | 12 ++++++ t/helper/.gitignore | 1 + t/helper/test-core-checksum-index.c | 77 +++++++++++++++++++++++++++++++++++++ 4 files changed, 91 insertions(+) create mode 100644 t/helper/test-core-checksum-index.c
Signature · 1 line
2.9.3
PATCH v3 1/2 read-cache: core.checksumindex
From: Jeff Hostetler <[email protected]>
Teach git to skip verification of the index SHA in verify_hdr() in read_index().
This is a performance optimization. The index file SHA verification can be considered an ancient relic from the early days of git and only useful for detecting disk corruption. For small repositories, this SHA calculation is not that significant, but for gigantic repositories this calculation adds significant time to every command.
Added "core.checksumindex" to enable/disable the SHA verification.
--- read-cache.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
read-cache.c +12 -0
@@ -1376,12 +1376,24 @@ static int verify_hdr(struct cache_header *hdr, unsigned long size)git_SHA_CTX c;unsigned char sha1[20 ];int hdr_version; +int do_checksum =0 ;if (hdr->hdr_signature !=htonl (CACHE_SIGNATURE ))return error ("bad signature" ); hdr_version =ntohl (hdr->hdr_version);if (hdr_version <INDEX_FORMAT_LB ||INDEX_FORMAT_UB < hdr_version)return error ("bad index version %d" , hdr_version); + +/* +* Since we run very early in command startup, git_config() +* may not have been called yet and the various "core_*" +* global variables haven't been set. So look it up +* explicitly. +*/ +git_config_get_bool ("core.checksumindex" , &do_checksum); +if (!do_checksum) +return 0 ; +git_SHA1_Init (&c);git_SHA1_Update (&c, hdr, size -20 );git_SHA1_Final (sha1, &c);
Signature · 1 line
2.9.3
PATCH v3 2/2 test-core-checksum-index: core.checksumindex test helper
In reply to [email protected]From: Jeff Hostetler <[email protected]>
Created test helper to measure read_index() with and without core.checksumindex set and report performance.
--- Makefile | 1 + t/helper/.gitignore | 1 + t/helper/test-core-checksum-index.c | 77 +++++++++++++++++++++++++++++++++++++ 3 files changed, 79 insertions(+) create mode 100644 t/helper/test-core-checksum-index.c
Makefile +1 -0
@@ -607,6 +607,7 @@ PROGRAMS += $(patsubst %.o,git-%$X,$(PROGRAM_OBJS)) TEST_PROGRAMS_NEED_X += test-chmtime TEST_PROGRAMS_NEED_X += test-ctype TEST_PROGRAMS_NEED_X += test-config +TEST_PROGRAMS_NEED_X += test-core-checksum-index TEST_PROGRAMS_NEED_X += test-date TEST_PROGRAMS_NEED_X += test-delta TEST_PROGRAMS_NEED_X += test-dump-cache-tree
t/helper/.gitignore +1 -0
@@ -1,6 +1,7 @@ /test-chmtime /test-ctype /test-config +/test-core-checksum-index /test-date /test-delta /test-dump-cache-tree
t/helper/test-core-checksum-index.c new file +77 -0
77 lines not on this page · raw message
Signature · 1 line
2.9.3
Quoted 10 lines · On Tue, Mar 28, 2017 at 07:07:30PM +0000, [email protected] wrote:
From: Jeff Hostetler <[email protected]>
Version 3 of this patch series simplifies this effort to just turn on/off the hash verification using a "core.checksumindex" config variable.
I've preserved the original checksum validation code so that we can force it on in fsck if desired.
It eliminates the original threading model completely.
Quoted 8 lines
Jeff Hostetler (2):
read-cache: core.checksumindex
test-core-checksum-index: core.checksumindex test helperMakefile | 1 + read-cache.c | 12 ++++++ t/helper/.gitignore | 1 + t/helper/test-core-checksum-index.c | 77 +++++++++++++++++++++++++++++++++++++
Do we still need test-core-checksum-index? Can we just time ls-files or something in t/perf?
-Peff
Quoted 16 lines · Jeff King
Quoted 14 lines · On Tue, Mar 28, 2017 at 07:07:30PM +0000, [email protected] wrote:
From: Jeff Hostetler <[email protected]>
Version 3 of this patch series simplifies this effort to just turn on/off the hash verification using a "core.checksumindex" config variable.
I've preserved the original checksum validation code so that we can force it on in fsck if desired.
It eliminates the original threading model completely.
Jeff Hostetler (2):
read-cache: core.checksumindex
test-core-checksum-index: core.checksumindex test helper
Quoted 7 lines
Quoted 4 lines
Makefile | 1 + read-cache.c | 12 ++++++ t/helper/.gitignore | 1 + t/helper/test-core-checksum-index.c | 77 +++++++++++++++++++++++++++++++++++++Do we still need test-core-checksum-index? Can we just time ls-files or something in t/perf?
It was a convenient way to isolate, average, and compare read_index() times, but I suppose we could do something like that.
I did confirm that a ls-files does show a slight 0.008 second difference on the 58K file Linux tree when toggled on or off.
But I'm tempted to suggest that we just omit my helper exe and not worry about a test -- since we don't have any test repos large enough to really demonstrate the differences. My concern is that that 0.008 would be lost in the noise of the rest of the test and make for an unreliable result.
Jeff
Quoted 7 lines · Jeff Hostetler
It was a convenient way to isolate, average, and compare
read_index()times, but I suppose we could do something like that.I did confirm that a ls-files does show a slight 0.008 second difference on the 58K file Linux tree when toggled on or off.
Yeah, I agree it helps isolate the change. I'm just not sure we want to carry a bunch of function-specific perf-testing code. And one of the nice things about testing a real command is that it's...a real command. So it's an actual improvement a user might see.
Quoted 5 lines
But I'm tempted to suggest that we just omit my helper exe and not worry about a test -- since we don't have any test repos large enough to really demonstrate the differences. My concern is that that 0.008 would be lost in the noise of the rest of the test and make for an unreliable result.
Yeah, I think that would be fine. You _could_ write a t/perf test and then use your 400MB monstrosity as GIT_PERF_LARGE_REPO. But given that most people don't have such a thing, there's not much value over you just showing off the perf improvement in the commit message.
We could also have a t/perf test that generates a monstrous index and shows that it's faster. But frankly, I don't think this is all that interesting as a performance regression test. It's not like there's something subtle about the performance improvement; we stopped computing the SHA-1, and (gasp!) it takes exactly one SHA-1 computation's less time.
So just mentioning the test case and the improvement in the commit message is sufficient, IMHO.
-Peff
Quoted 2 lines · Jeff King
So just mentioning the test case and the improvement in the commit message is sufficient, IMHO.
So here is how I butchered [v3 1/2] to tentatively queue it on 'pu'.
Notable suggested changes I have in this one are:
* I stole the numbers from the cover letter of v2 and added them at the end of the log message.
* As the checksum is not a useless relic, but is an integrity check, I dropped the "ancient relic" from the proposed log message. It is just that the modern disks are reliable enough to make it worthwhile to think about a trade-off this patch makes between performance and integrity.
* As it is customary, the configuration variable starts as an opt in feature. In a few releases, perhaps we can flip the default, but we do not do so from day one.
* Updated the code around the call to config-get-bool to avoid asking the same question twice.
* Added minimum documentation.
By the way, are we sure we have something that validates the checksum regardless of the configuration setting? If not, we may want to tweak this further so that we can force the validation from "git fsck" or something. I am not going to do that myself, but it may be necessary before this graduates to 'master'.
Thanks.
-- >8 --
From: Jeff Hostetler <[email protected]>
Date: Tue, 28 Mar 2017 19:07:31 +0000
Subject: [PATCH] read-cache: core.checksumindex
Teach git to skip verification of the SHA-1 checksum at the end of the index file in verify_hdr() called from read_index() when the core.checksumIndex configuration variable is set to false.
The checksum verification is for detecting disk corruption, and for small projects, the time it takes to compute SHA-1 is not that significant, but for gigantic repositories this calculation adds significant time to every command.
On the Linux kernel repository, the effect is rather trivial. The time to reading its index with 58k entries drops from 0.0284 sec down to 0.0155 sec.
On my Windows source tree (450MB index), I'm seeing a savings of 0.6 seconds -- read_index() went from 1.2 to 0.6 seconds.
--- Documentation/config.txt | 8 ++++++++ read-cache.c | 16 ++++++++++++++++ 2 files changed, 24 insertions(+)
Documentation/config.txt +8 -0
@@ -329,6 +329,14 @@ advice.*:: show directions on how to proceed from the current state. -- +core.checksumIndex:: + Tell Git to validate the checksum at the end of the index + file to detect corruption. Defaults to `true`. Those who + work on a project with too many files may want to set this + variable to `false` to make it faster to load the index (in + exchange for reliability, but in general modern disks are + reliable enough for most people). + core.fileMode:: Tells Git if the executable bit of files in the working tree is to be honored.
read-cache.c +16 -0
@@ -1376,12 +1376,28 @@ static int verify_hdr(struct cache_header *hdr, unsigned long size)git_SHA_CTX c;unsigned char sha1[20 ];int hdr_version; +static int do_checksum =-1 ;if (hdr->hdr_signature !=htonl (CACHE_SIGNATURE ))return error ("bad signature" ); hdr_version =ntohl (hdr->hdr_version);if (hdr_version <INDEX_FORMAT_LB ||INDEX_FORMAT_UB < hdr_version)return error ("bad index version %d" , hdr_version); + +if (do_checksum <0 ) { +/* +* Since we run very early in command startup, git_config() +* may not have been called yet and the various "core_*" +* global variables haven't been set. So look it up +* explicitly. +*/ +git_config_get_bool ("core.checksumindex" , &do_checksum); +if (do_checksum <0 ) + do_checksum =0 ;/* default to false */ + } +if (!do_checksum) +return 0 ; +git_SHA1_Init (&c);git_SHA1_Update (&c, hdr, size -20 );git_SHA1_Final (sha1, &c);
Signature · 1 line
2.12.2-727-gf32eb5229d
Quoted 4 lines · Junio C Hamano
Notable suggested changes I have in this one are:
* I stole the numbers from the cover letter of v2 and added them at the end of the log message.
Yeah, good.
Quoted 5 lines
* As the checksum is not a useless relic, but is an integrity check, I dropped the "ancient relic" from the proposed log message. It is just that the modern disks are reliable enough to make it worthwhile to think about a trade-off this patch makes between performance and integrity.
Yeah, I'd agree this is more of a tradeoff than a cleanup.
Quoted 3 lines
* As it is customary, the configuration variable starts as an opt in feature. In a few releases, perhaps we can flip the default, but we do not do so from day one.
I think this is so user-invisible that it doesn't really matter much, but I'm OK with taking it slow.
Quoted 2 lines
* Updated the code around the call to config-get-bool to avoid asking the same question twice.
A few comments on that below.
Quoted 1 line
* Added minimum documentation.
Yep, looks good.
Quoted 5 lines
By the way, are we sure we have something that validates the checksum regardless of the configuration setting? If not, we may want to tweak this further so that we can force the validation from "git fsck" or something. I am not going to do that myself, but it may be necessary before this graduates to 'master'.
Yes, I'd agree this shouldn't graduate without the matching change to teach fsck to flip the switch back.
Quoted 5 lines
read-cache.c +4
-0+static int do_checksum =-1 ; +if (do_checksum <0 ) { +/* +* Since we run very early in command startup, git_config()
read-cache.c static int verify_hdr(struct cache_header *hdr, unsigned long size) · line 1399
Quoted 10 lines
read-cache.c +10
-0+* may not have been called yet and the various "core_*" +* global variables haven't been set. So look it up +* explicitly. +*/ +git_config_get_bool ("core.checksumindex" , &do_checksum); +if (do_checksum <0 ) + do_checksum =0 ;/* default to false */ + } +if (!do_checksum) +return 0 ;
This is basically introducing a new caching layer, but there's no way to invalidate it (if, say, we looked at the config once and then we knew that the config changed). I think it's probably OK, because the main reason for the config to change is reading it before/after repository setup. But since this is index code, it shouldn't be called at all before repository setup.
Still, I'm not sure the extra layer of cache is all that valuable. It should be a single hash lookup in the config cache (in an operation that otherwise reads the entire index).
-Peff
Quoted 3 lines · Jeff King
Still, I'm not sure the extra layer of cache is all that valuable. It should be a single hash lookup in the config cache (in an operation that otherwise reads the entire index).
OK, let's drop that part, then.
Quoted 8 lines · Junio C Hamano
Quoted 3 lines · Jeff King
Still, I'm not sure the extra layer of cache is all that valuable. It should be a single hash lookup in the config cache (in an operation that otherwise reads the entire index).
OK, let's drop that part, then.
Yes, let's omit the caching there. That way perf tests can run it multiple times with it on or off as they see fit. And in the normal case it will only be executed once anyway.
Quoted 21 lines · Jeff King
Quoted 7 lines · Jeff Hostetler
It was a convenient way to isolate, average, and compare
read_index()times, but I suppose we could do something like that.I did confirm that a ls-files does show a slight 0.008 second difference on the 58K file Linux tree when toggled on or off.
Yeah, I agree it helps isolate the change. I'm just not sure we want to carry a bunch of function-specific perf-testing code. And one of the nice things about testing a real command is that it's...a real command. So it's an actual improvement a user might see.
Quoted 5 lines
But I'm tempted to suggest that we just omit my helper exe and not worry about a test -- since we don't have any test repos large enough to really demonstrate the differences. My concern is that that 0.008 would be lost in the noise of the rest of the test and make for an unreliable result.
Quoted 4 lines
Yeah, I think that would be fine. You _could_ write a t/perf test and then use your 400MB monstrosity as
GIT_PERF_LARGE_REPO. But given that most people don't have such a thing, there's not much value over you just showing off the perf improvement in the commit message.
Sorry if this was already discussed, but we already do have a perf test for the index (p0002), and a corresponding helper program which just does read_cache() and discard_cache(). Maybe we could re-use that and add a second test running the same using the new config?
Quoted 11 lines
We could also have a t/perf test that generates a monstrous index and shows that it's faster. But frankly, I don't think this is all that interesting as a performance regression test. It's not like there's something subtle about the performance improvement; we stopped computing the SHA-1, and (gasp!) it takes exactly one SHA-1 computation's less time.
So just mentioning the test case and the improvement in the commit message is sufficient, IMHO.
-Peff
Quoted 9 lines · Thomas Gummerer
Quoted 4 lines
Yeah, I think that would be fine. You _could_ write a t/perf test and then use your 400MB monstrosity as
GIT_PERF_LARGE_REPO. But given that most people don't have such a thing, there's not much value over you just showing off the perf improvement in the commit message.Sorry if this was already discussed, but we already do have a perf test for the index (p0002), and a corresponding helper program which just does
read_cache()anddiscard_cache(). Maybe we could re-use that and add a second test running the same using the new config?
Oh, indeed. Yes, I would think the results of p0002 would probably show off Jeff's improvements.
-Peff
Quoted 12 lines · Jeff King
Quoted 9 lines · Thomas Gummerer
Quoted 4 lines
Yeah, I think that would be fine. You _could_ write a t/perf test and then use your 400MB monstrosity as
GIT_PERF_LARGE_REPO. But given that most people don't have such a thing, there's not much value over you just showing off the perf improvement in the commit message.Sorry if this was already discussed, but we already do have a perf test for the index (p0002), and a corresponding helper program which just does
read_cache()anddiscard_cache(). Maybe we could re-use that and add a second test running the same using the new config?
Quoted 5 lines
Oh, indeed. Yes, I would think the results of p0002 would probably show off Jeff's improvements.
-Peff
Let me re-roll it with Junio's cleanup, update fsck to force it on, and look at using p0002.
Thanks,
Jeff