add: speed up cmd_add() by utilizing read_cache_preload()
PATCH 1/1 add: speed up cmd_add() by utilizing read_cache_preload()
From: Ben Peart <[email protected]>
During an "add", a call is made to run_diff_files() which calls check_remove() for each index-entry. The preload_index() code distributes some of the costs across multiple threads.
Because the files checked are restricted to pathspec, adding individual files makes no measurable impact but on a Windows repo with ~200K files, 'git add .' drops from 6.3 seconds to 3.3 seconds for a 47% savings.
---
Notes:
Base Ref: master
Web-Diff: https://github.com/benpeart/git/commit/fc4830b545
Checkout: git fetch https://github.com/benpeart/git add-preload-index-v1 && git checkout fc4830b545builtin/add.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)
builtin/add.c +4 -5
@@ -445,11 +445,6 @@ int cmd_add(int argc, const char **argv, const char *prefix)return 0 ; } -if (read_cache () <0 ) -die (_ ("index file corrupt" )); - -die_in_unpopulated_submodule (&the_index, prefix); -/* * Check the "pathspec '%s' did not match any files" block * below before enabling new magic.
@@ -459,6 +454,10 @@ int cmd_add(int argc, const char **argv, const char *prefix)PATHSPEC_SYMLINK_LEADING_PATH , prefix, argv); +if (read_cache_preload (&pathspec) <0 ) +die (_ ("index file corrupt" )); + +die_in_unpopulated_submodule (&the_index, prefix);die_path_inside_submodule (&the_index, &pathspec);if (add_new_files) {
base-commit: 4ede3d42dfb57f9a41ac96a1f216c62eb7566cc2
Signature · 1 line
2.18.0.windows.1
Quoted 5 lines · Ben Peart
From: Ben Peart <[email protected]>
During an "add", a call is made to
run_diff_files()which callscheck_remove()for each index-entry. Thepreload_index()code distributes some of the costs across multiple threads.
Nice. I peeked around and noticed that we already do this in builtin_diff_index() before running run_diff_index() when !cached, and builtin_diff_files(), of course.
Quoted 3 lines
Because the files checked are restricted to pathspec, adding individual files makes no measurable impact but on a Windows repo with ~200K files, 'git add .' drops from 6.3 seconds to 3.3 seconds for a 47% savings.
;-)
Quoted 6 lines
builtin/add.c +0
-0@@ -445,11 +445,6 @@ int cmd_add(int argc, const char **argv, const char *prefix)return 0 ;
builtin/add.c int cmd_add(int argc, const char **argv, const char *prefix) · line 450
Quoted 10 lines
builtin/add.c +0
-5} -if (read_cache () <0 ) -die (_ ("index file corrupt" )); - -die_in_unpopulated_submodule (&the_index, prefix); -/* * Check the "pathspec '%s' did not match any files" block * below before enabling new magic.
It is not explained why this is not a mere s/read_cache/&_preload/ in the log message. I can see it is because you wanted to make the pathspec available to preload to further cut down the preloaded paths, and I do not think it has any unintended (negatie) side effect to parse the pathspec before populating the in-core index, but that would have been a good thing to mention in the proposed log message.
Quoted 13 lines
builtin/add.c +4
-0@@ -459,6 +454,10 @@ int cmd_add(int argc, const char **argv, const char *prefix)PATHSPEC_SYMLINK_LEADING_PATH , prefix, argv); +if (read_cache_preload (&pathspec) <0 ) +die (_ ("index file corrupt" )); + +die_in_unpopulated_submodule (&the_index, prefix);die_path_inside_submodule (&the_index, &pathspec);if (add_new_files) {base-commit:
4ede3d42dfb57f9a41ac96a1f216c62eb7566cc2
Quoted 4 lines · Junio C Hamano
Quoted 2 lines · Ben Peart
From: Ben Peart <[email protected]>
Quoted 8 lines
Quoted 3 lines
During an "add", a call is made to
run_diff_files()which callscheck_remove()for each index-entry. Thepreload_index()code distributes some of the costs across multiple threads.Nice. I peeked around and noticed that we already do this in
builtin_diff_index()before runningrun_diff_index()when !cached, andbuiltin_diff_files(), of course.
Thanks for double checking!
Quoted 14 lines
Quoted 3 lines
Because the files checked are restricted to pathspec, adding individual files makes no measurable impact but on a Windows repo with ~200K files, 'git add .' drops from 6.3 seconds to 3.3 seconds for a 47% savings.
;-)
Quoted 8 lines
builtin/add.c +0
-0@@ -445,11 +445,6 @@ int cmd_add(int argc, const char **argv, const char *prefix)return 0 ; }
Quoted 17 lines
builtin/add.c int cmd_add(int argc, const char **argv, const char *prefix) · line 450
Quoted 8 lines
builtin/add.c +0
-5-if (read_cache () <0 ) -die (_ ("index file corrupt" )); - -die_in_unpopulated_submodule (&the_index, prefix); -/* * Check the "pathspec '%s' did not match any files" block * below before enabling new magic. It is not explained why this is not a mere s/read_cache/&_preload/ in the log message. I can see it is because you wanted to make the pathspec available to preload to further cut down the preloaded paths, and I do not think it has any unintended (negatie) side effect to parse the pathspec before populating the in-core index, but that would have been a good thing to mention in the proposed log message.
That is correct. parse_pathspec() was after read_cache() because it _used_ to use the index to determine whether a pathspec is in a submodule. That was fixed by Brandon in Aug 2017 when he cleaned up all submodule config code so it is safe to move read_cache_preload() after the call to parse_pathspec().
How about this for a revised commit message?
During an "add", a call is made to run_diff_files() which callscheck_remove() for each index-entry. The preload_index() code distributes some of the costs across multiple threads.
Limit read_cache_preload() to pathspec, so that it doesn't process more entries than are needed and end up slowing things down instead of speeding them up.
Because the files checked are restricted to pathspec, adding individual files makes no measurable impact but on a Windows repo with ~200K files, 'git add .' drops from 6.3 seconds to 3.3 seconds for a 47% savings.
Quoted 13 lines
Quoted 13 lines
builtin/add.c +4
-0@@ -459,6 +454,10 @@ int cmd_add(int argc, const char **argv, const char *prefix)PATHSPEC_SYMLINK_LEADING_PATH , prefix, argv); +if (read_cache_preload (&pathspec) <0 ) +die (_ ("index file corrupt" )); + +die_in_unpopulated_submodule (&the_index, prefix);die_path_inside_submodule (&the_index, &pathspec);if (add_new_files) {base-commit:
4ede3d42dfb57f9a41ac96a1f216c62eb7566cc2
Quoted 6 lines · Ben Peart
From: Ben Peart <[email protected]>
During an "add", a call is made to
run_diff_files()which callscheck_remove()for each index-entry. Thepreload_index()code distributes some of the costs across multiple threads.
Instead of doing this site by site. How about we make read_cache() always do multithread preload?
The only downside I see is preload may actually harm when there are too few cache entries (but more than 500), but this needs to be verified. If the penalty is small enough, I think we could live with it since everything is fast when you have few entries anyway.
But if that's not true, we could add a threshold to activate preload. Something like "if you have 50k files or more, then activate preload" would do. I notice THREAD_COST in preload code, but I don't think it's the same thing.
Quoted 48 lines
Because the files checked are restricted to pathspec, adding individual files makes no measurable impact but on a Windows repo with ~200K files, 'git add .' drops from 6.3 seconds to 3.3 seconds for a 47% savings.
Signed-off-by Ben Peart <[email protected]>---
Notes:
Base Ref: master Web-Diff: https://github.com/benpeart/git/commit/fc4830b545 Checkout: git fetch https://github.com/benpeart/git add-preload-index-v1 && git checkout fc4830b545builtin/add.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)builtin/add.c +4
-5@@ -445,11 +445,6 @@ int cmd_add(int argc, const char **argv, const char *prefix)return 0 ; } -if (read_cache () <0 ) -die (_ ("index file corrupt" )); - -die_in_unpopulated_submodule (&the_index, prefix); -/* * Check the "pathspec '%s' did not match any files" block * below before enabling new magic. @@ -459,6 +454,10 @@ int cmd_add(int argc, const char **argv, const char *prefix)PATHSPEC_SYMLINK_LEADING_PATH , prefix, argv); +if (read_cache_preload (&pathspec) <0 ) +die (_ ("index file corrupt" )); + +die_in_unpopulated_submodule (&the_index, prefix);die_path_inside_submodule (&the_index, &pathspec);if (add_new_files) {base-commit:
4ede3d42dfb57f9a41ac96a1f216c62eb7566cc2--
2.18.0.windows.1
Signature · 1 line
Duy
Quoted 10 lines · Duy Nguyen
Quoted 6 lines · Ben Peart
From: Ben Peart <[email protected]>
During an "add", a call is made to
run_diff_files()which callscheck_remove()for each index-entry. Thepreload_index()code distributes some of the costs across multiple threads.Instead of doing this site by site. How about we make
read_cache()always do multithread preload?
I suspect that it would be a huge performance killer.
Many codepaths do not even want to know if the working tree files have been modified, even though they need to know what's in the index. Think "git commit-tree", "git diff --cached", etc.
Quoted 11 lines · Junio C Hamano
Quoted 8 lines · Duy Nguyen
Quoted 6 lines · Ben Peart
From: Ben Peart <[email protected]>
During an "add", a call is made to
run_diff_files()which callscheck_remove()for each index-entry. Thepreload_index()code distributes some of the costs across multiple threads.
Quoted 8 lines
Quoted 2 lines
Instead of doing this site by site. How about we make
read_cache()always do multithread preload?I suspect that it would be a huge performance killer.
Many codepaths do not even want to know if the working tree files have been modified, even though they need to know what's in the index. Think "git commit-tree", "git diff --cached", etc.
Ah. I keep forgetting read_cache_preload is loading the index _and_ refreshing. I thought the two had some different semantics but failed to see it last time.
Signature · 1 line
Duy