{"thread":{"id":"51872","subject":"[RFC PATCH 0/3] grep: don't add subrepos to in-memory alternates","startedAt":"2019-09-18T01:56:43Z","lastAt":"2019-09-28T04:23:36Z","messageCount":11,"participants":["Matheus Tavares","Junio C Hamano","Matheus Tavares Bernardino","Brandon Williams"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"382535","messageId":"cover.1568771073.git.matheus.bernardino@usp.br","threadId":"51872","inReplyTo":null,"subject":"[RFC PATCH 0/3] grep: don't add subrepos to in-memory alternates","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2019-09-18T01:56:02Z","receivedAt":"2019-09-18T01:56:43Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Make git-grep --recurse-submodules stop adding subrepos to the in-memory\nalternates list and, instead, pass a reference to the subrepo struct\ndown to the threads.\n\nThis series is based on ew/hashmap (it uses the added container_of\nmacro).\n\nThere're some possibly needed changes I'm still unsure about. All\nfeedback will be highly appreciated:\n\n- textconv cache is written to the_repository's object database even for\n  submodules. Should it perhaps be written to submodules' odb instead?\n\n- Considering the following call chain: grep_source_load_driver() >\n  userdiff_find_by_path() > git_check_attr() > collect_some_attrs() >\n  prepare_attr_stack() > bootstrap_attr_stack():\n\n  * The last function tries to read the attributes from the\n    .gitattributes and .git/info/attributes files of the_repository.\n    However, for paths inside the submodule, shouldn't it try to read\n    these files from the submodule?\n\n  * This function will also call: read_attr() > read_attr_from_index() >\n    read_blob_data_from_index() which might, in turn, call\n    read_object_file(). Shouldn't we pass the subrepo to it so that it\n    can call repo_read_object_file()? (Again, for paths inside the\n    submodule, read_object_file() won't be able to find the object as\n    we won't be adding to alternates anymore.)\n\nMatheus Tavares (3):\n  diff: use the given repo at diff_populate_filespec()\n  object: allow parse_object_or_die() to handle any repo\n  grep: don't add submodules to the alternates list\n\n builtin/grep.c  | 82 +++++++++++++++++++++++++++++++++++--------------\n builtin/prune.c |  4 +--\n bundle.c        | 13 +++++---\n diff.c          |  2 +-\n grep.c          | 26 ++++++++--------\n object.c        |  5 +--\n object.h        |  4 ++-\n pack-bitmap.c   |  5 +--\n reachable.c     |  4 +--\n upload-pack.c   |  2 +-\n 10 files changed, 94 insertions(+), 53 deletions(-)\n\n-- \n2.23.0\n\n"},{"id":"382536","messageId":"cac0c5a8979d45225436f310660f6fcf4d0bd7e7.1568771073.git.matheus.bernardino@usp.br","threadId":"51872","inReplyTo":"cover.1568771073.git.matheus.bernardino@usp.br","subject":"[RFC PATCH 1/3] diff: use the given repo at diff_populate_filespec()","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2019-09-18T01:56:03Z","receivedAt":"2019-09-18T01:56:52Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"diff_populate_filespec() accepts an struct repository \"r\" as parameter\nbut implicitly uses the_repository when calling read_object_file(). To\nbe more consistent, replace this call with\nrepo_read_object_file(r, ...).\n\nSigned-off-by: Matheus Tavares <matheus.bernardino@usp.br>\n---\n diff.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/diff.c b/diff.c\nindex 663b5d01f8..c743e4ce50 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4044,7 +4044,7 @@ int diff_populate_filespec(struct repository *r,\n \t\t\t\treturn 0;\n \t\t\t}\n \t\t}\n-\t\ts->data = read_object_file(&s->oid, &type, &s->size);\n+\t\ts->data = repo_read_object_file(r, &s->oid, &type, &s->size);\n \t\tif (!s->data)\n \t\t\tdie(\"unable to read %s\", oid_to_hex(&s->oid));\n \t\ts->should_free = 1;\n-- \n2.23.0\n\n"},{"id":"382537","messageId":"e819ba8a81b11db597bb33e94da869c9ca10febb.1568771073.git.matheus.bernardino@usp.br","threadId":"51872","inReplyTo":"cover.1568771073.git.matheus.bernardino@usp.br","subject":"[RFC PATCH 2/3] object: allow parse_object_or_die() to handle any repo","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2019-09-18T01:56:04Z","receivedAt":"2019-09-18T01:56:57Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"parse_object_or_die() was added before parse_object() learned to handle\narbitrary repositories. Now that it did, let's also bring this behavior\nimprovement to the former, adding a struct repository argument. Also\nadjust the callers and, while we are here, avoid using `oid_to_hex(oid)`\nfor the third parameter as it's the default when NULL is given.\n\nThis will be used in the following patch, to allow git-grep to parse\nsubmodule's objects.\n\nSigned-off-by: Matheus Tavares <matheus.bernardino@usp.br>\n---\n builtin/grep.c  |  4 ++--\n builtin/prune.c |  4 ++--\n bundle.c        | 13 ++++++++-----\n object.c        |  5 +++--\n object.h        |  4 +++-\n pack-bitmap.c   |  5 +++--\n reachable.c     |  4 ++--\n upload-pack.c   |  2 +-\n 8 files changed, 24 insertions(+), 17 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 2699001fbd..73ef00c426 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -455,7 +455,7 @@ static int grep_submodule(struct grep_opt *opt,\n \t\tunsigned long size;\n \t\tstruct strbuf base = STRBUF_INIT;\n \n-\t\tobject = parse_object_or_die(oid, oid_to_hex(oid));\n+\t\tobject = parse_object_or_die(the_repository, oid, NULL);\n \n \t\tgrep_read_lock();\n \t\tdata = read_object_with_reference(&subrepo,\n@@ -1037,7 +1037,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\t\tbreak;\n \t\t}\n \n-\t\tobject = parse_object_or_die(&oid, arg);\n+\t\tobject = parse_object_or_die(the_repository, &oid, arg);\n \t\tif (!seen_dashdash)\n \t\t\tverify_non_filename(prefix, arg);\n \t\tadd_object_array_with_path(object, arg, &list, oc.mode, oc.path);\ndiff --git a/builtin/prune.c b/builtin/prune.c\nindex 2b76872ad2..996defe284 100644\n--- a/builtin/prune.c\n+++ b/builtin/prune.c\n@@ -154,8 +154,8 @@ int cmd_prune(int argc, const char **argv, const char *prefix)\n \t\tconst char *name = *argv++;\n \n \t\tif (!get_oid(name, &oid)) {\n-\t\t\tstruct object *object = parse_object_or_die(&oid,\n-\t\t\t\t\t\t\t\t    name);\n+\t\t\tstruct object *object =\n+\t\t\t\tparse_object_or_die(the_repository, &oid, name);\n \t\t\tadd_pending_object(&revs, object, \"\");\n \t\t}\n \t\telse\ndiff --git a/bundle.c b/bundle.c\nindex b5d21cd80f..67552786a5 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -315,14 +315,16 @@ static int compute_and_write_prerequisites(int bundle_fd,\n \t\tif (buf.len > 0 && buf.buf[0] == '-') {\n \t\t\twrite_or_die(bundle_fd, buf.buf, buf.len);\n \t\t\tif (!get_oid_hex(buf.buf + 1, &oid)) {\n-\t\t\t\tstruct object *object = parse_object_or_die(&oid,\n-\t\t\t\t\t\t\t\t\t    buf.buf);\n+\t\t\t\tstruct object *object =\n+\t\t\t\t\tparse_object_or_die(the_repository,\n+\t\t\t\t\t\t\t    &oid, buf.buf);\n \t\t\t\tobject->flags |= UNINTERESTING;\n \t\t\t\tadd_pending_object(revs, object, buf.buf);\n \t\t\t}\n \t\t} else if (!get_oid_hex(buf.buf, &oid)) {\n-\t\t\tstruct object *object = parse_object_or_die(&oid,\n-\t\t\t\t\t\t\t\t    buf.buf);\n+\t\t\tstruct object *object =\n+\t\t\t\tparse_object_or_die(the_repository,\n+\t\t\t\t\t\t    &oid, buf.buf);\n \t\t\tobject->flags |= SHOWN;\n \t\t}\n \t}\n@@ -406,7 +408,8 @@ static int write_bundle_refs(int bundle_fd, struct rev_info *revs)\n \t\t\t\t * end up triggering \"empty bundle\"\n \t\t\t\t * error.\n \t\t\t\t */\n-\t\t\t\tobj = parse_object_or_die(&oid, e->name);\n+\t\t\t\tobj = parse_object_or_die(the_repository, &oid,\n+\t\t\t\t\t\t\t  e->name);\n \t\t\t\tobj->flags |= SHOWN;\n \t\t\t\tadd_pending_object(revs, obj, e->name);\n \t\t\t}\ndiff --git a/object.c b/object.c\nindex 07bdd5b26e..54c4861f4c 100644\n--- a/object.c\n+++ b/object.c\n@@ -237,10 +237,11 @@ struct object *parse_object_buffer(struct repository *r, const struct object_id\n \treturn obj;\n }\n \n-struct object *parse_object_or_die(const struct object_id *oid,\n+struct object *parse_object_or_die(struct repository *r,\n+\t\t\t\t   const struct object_id *oid,\n \t\t\t\t   const char *name)\n {\n-\tstruct object *o = parse_object(the_repository, oid);\n+\tstruct object *o = parse_object(r, oid);\n \tif (o)\n \t\treturn o;\n \ndiff --git a/object.h b/object.h\nindex 0120892bbd..d8f02f53cd 100644\n--- a/object.h\n+++ b/object.h\n@@ -134,7 +134,9 @@ struct object *parse_object(struct repository *r, const struct object_id *oid);\n  * \"name\" parameter is not NULL, it is included in the error message\n  * (otherwise, the hex object ID is given).\n  */\n-struct object *parse_object_or_die(const struct object_id *oid, const char *name);\n+struct object *parse_object_or_die(struct repository *r,\n+\t\t\t\t   const struct object_id *oid,\n+\t\t\t\t   const char *name);\n \n /* Given the result of read_sha1_file(), returns the object after\n  * parsing it.  eaten_p indicates if the object has a borrowed copy\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex ed2befaac6..383b2351db 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -699,7 +699,7 @@ struct bitmap_index *prepare_bitmap_walk(struct rev_info *revs)\n \t\tstruct object *object = revs->pending.objects[i].item;\n \n \t\tif (object->type == OBJ_NONE)\n-\t\t\tparse_object_or_die(&object->oid, NULL);\n+\t\t\tparse_object_or_die(the_repository, &object->oid, NULL);\n \n \t\twhile (object->type == OBJ_TAG) {\n \t\t\tstruct tag *tag = (struct tag *) object;\n@@ -711,7 +711,8 @@ struct bitmap_index *prepare_bitmap_walk(struct rev_info *revs)\n \n \t\t\tif (!tag->tagged)\n \t\t\t\tdie(\"bad tag\");\n-\t\t\tobject = parse_object_or_die(&tag->tagged->oid, NULL);\n+\t\t\tobject = parse_object_or_die(the_repository,\n+\t\t\t\t\t\t     &tag->tagged->oid, NULL);\n \t\t}\n \n \t\tif (object->flags & UNINTERESTING)\ndiff --git a/reachable.c b/reachable.c\nindex 8f50235b28..7c5f7a0ba0 100644\n--- a/reachable.c\n+++ b/reachable.c\n@@ -37,7 +37,7 @@ static int add_one_ref(const char *path, const struct object_id *oid,\n \t\treturn 0;\n \t}\n \n-\tobject = parse_object_or_die(oid, path);\n+\tobject = parse_object_or_die(the_repository, oid, path);\n \tadd_pending_object(revs, object, \"\");\n \n \treturn 0;\n@@ -86,7 +86,7 @@ static void add_recent_object(const struct object_id *oid,\n \tswitch (type) {\n \tcase OBJ_TAG:\n \tcase OBJ_COMMIT:\n-\t\tobj = parse_object_or_die(oid, NULL);\n+\t\tobj = parse_object_or_die(the_repository, oid, NULL);\n \t\tbreak;\n \tcase OBJ_TREE:\n \t\tobj = (struct object *)lookup_tree(the_repository, oid);\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 222cd3ad89..648ff3355b 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -1220,7 +1220,7 @@ static int parse_want_ref(struct packet_writer *writer, const char *line,\n \t\titem = string_list_append(wanted_refs, arg);\n \t\titem->util = oiddup(&oid);\n \n-\t\to = parse_object_or_die(&oid, arg);\n+\t\to = parse_object_or_die(the_repository, &oid, arg);\n \t\tif (!(o->flags & WANTED)) {\n \t\t\to->flags |= WANTED;\n \t\t\tadd_object_array(o, NULL, want_obj);\n-- \n2.23.0\n\n"},{"id":"382538","messageId":"fc5f5fe95c10ac66d1234a025122c8866965021f.1568771073.git.matheus.bernardino@usp.br","threadId":"51872","inReplyTo":"cover.1568771073.git.matheus.bernardino@usp.br","subject":"[RFC PATCH 3/3] grep: don't add submodules to the alternates list","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2019-09-18T01:56:05Z","receivedAt":"2019-09-18T01:56:59Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"When grepping with --recurse-submodules, the object directory of the\nsubmodule is added to the in-memory alternates list. This makes git need\nto watch out for more packfiles which might bring bad consequences for\nmemory and performance.\n\nNow that raw_object_store is a member of the struct repository, it's\npossible to use the submodules' instances directly, without the need to\nadd its odbs to the alternates list. So let's instead pass the subrepo\ndown to the threads and replace function calls which use\nthe_repository by default for their more general counterparts. Also,\nsubmodule cleanup must be refactored as calling repo_clear() at the end\nof grep_submodules(), might free the subrepo's resources before threads\nhave finished working on it. To do that, let's keep track of the\nworkers progress and only call repo_clear() when it's safe to do so.\n\nThis change will also help future patches to re-enable threads in\nnon-worktree grep as less mutual exclusions will be needed. E.g.\ngrep_submodule()'s call to parse_object_or_die() won't conflict with\nother calls to the same function by worker threads (as they should be\nreferencing different 'struct repository's).\n\nSigned-off-by: Matheus Tavares <matheus.bernardino@usp.br>\n---\n builtin/grep.c | 80 ++++++++++++++++++++++++++++++++++++--------------\n grep.c         | 26 ++++++++--------\n 2 files changed, 70 insertions(+), 36 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 73ef00c426..0bb58c456b 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -43,6 +43,15 @@ static pthread_t *threads;\n  */\n struct work_item {\n \tstruct grep_source source;\n+\n+\t/*\n+\t * Each worker thread is initialized with a 'struct grep_opt *opt' where\n+\t * opt->repo points to the_repository. But a work item may refeer to a\n+\t * subrepo. So the repository relative to each task is also passed to\n+\t * the assigned thread and its opt->repo is updated when retrieving the\n+\t * task.\n+\t */\n+\tstruct repository *repo;\n \tchar done;\n \tstruct strbuf out;\n };\n@@ -65,6 +74,13 @@ static int todo_done;\n /* Has all work items been added? */\n static int all_work_added;\n \n+/* Tracks the work progress in subrepos to correctly free them in the end */\n+struct progress_in_subrepo {\n+\tstruct repository subrepo;\n+\tint all_work_added;\n+\tint work_counter;\n+};\n+\n /* This lock protects all the variables above. */\n static pthread_mutex_t grep_mutex;\n \n@@ -93,6 +109,8 @@ static int skip_first_line;\n \n static void add_work(struct grep_opt *opt, const struct grep_source *gs)\n {\n+\tstruct progress_in_subrepo *p;\n+\n \tgrep_lock();\n \n \twhile ((todo_end+1) % ARRAY_SIZE(todo) == todo_done) {\n@@ -104,9 +122,15 @@ static void add_work(struct grep_opt *opt, const struct grep_source *gs)\n \t\tgrep_source_load_driver(&todo[todo_end].source,\n \t\t\t\t\topt->repo->index);\n \ttodo[todo_end].done = 0;\n+\ttodo[todo_end].repo = opt->repo;\n \tstrbuf_reset(&todo[todo_end].out);\n \ttodo_end = (todo_end + 1) % ARRAY_SIZE(todo);\n \n+\tif (opt->repo != the_repository) {\n+\t\tp = container_of(opt->repo, struct progress_in_subrepo, subrepo);\n+\t\tp->work_counter++;\n+\t}\n+\n \tpthread_cond_signal(&cond_add);\n \tgrep_unlock();\n }\n@@ -132,6 +156,7 @@ static struct work_item *get_work(void)\n \n static void work_done(struct work_item *w)\n {\n+\tstruct progress_in_subrepo *p;\n \tint old_done;\n \n \tgrep_lock();\n@@ -156,6 +181,17 @@ static void work_done(struct work_item *w)\n \n \t\t\twrite_or_die(1, p, len);\n \t\t}\n+\n+\t\tif (w->repo != the_repository) {\n+\t\t\tp = container_of(w->repo, struct progress_in_subrepo,\n+\t\t\t\t\t subrepo);\n+\t\t\tp->work_counter--;\n+\t\t\tif (p->work_counter == 0 && p->all_work_added) {\n+\t\t\t\trepo_clear(&p->subrepo);\n+\t\t\t\tfree(p);\n+\t\t\t}\n+\t\t}\n+\n \t\tgrep_source_clear(&w->source);\n \t}\n \n@@ -179,6 +215,7 @@ static void *run(void *arg)\n \t\t\tbreak;\n \n \t\topt->output_priv = w;\n+\t\topt->repo = w->repo;\n \t\thit |= grep_source(opt, &w->source);\n \t\tgrep_source_clear_data(&w->source);\n \t\twork_done(w);\n@@ -295,12 +332,14 @@ static int grep_cmd_config(const char *var, const char *value, void *cb)\n \treturn st;\n }\n \n-static void *lock_and_read_oid_file(const struct object_id *oid, enum object_type *type, unsigned long *size)\n+static void *lock_and_read_oid_file(struct repository *r,\n+\t\t\t\t    const struct object_id *oid,\n+\t\t\t\t    enum object_type *type, unsigned long *size)\n {\n \tvoid *data;\n \n \tgrep_read_lock();\n-\tdata = read_object_file(oid, type, size);\n+\tdata = repo_read_object_file(r, oid, type, size);\n \tgrep_read_unlock();\n \treturn data;\n }\n@@ -405,7 +444,8 @@ static int grep_submodule(struct grep_opt *opt,\n \t\t\t  const struct object_id *oid,\n \t\t\t  const char *filename, const char *path, int cached)\n {\n-\tstruct repository subrepo;\n+\tstruct progress_in_subrepo *p = xcalloc(1, sizeof(*p));\n+\tstruct repository *subrepo = &p->subrepo;\n \tstruct repository *superproject = opt->repo;\n \tconst struct submodule *sub = submodule_from_path(superproject,\n \t\t\t\t\t\t\t  &null_oid, path);\n@@ -422,31 +462,21 @@ static int grep_submodule(struct grep_opt *opt,\n \n \tif (!is_submodule_active(superproject, path)) {\n \t\tgrep_read_unlock();\n+\t\tfree(p);\n \t\treturn 0;\n \t}\n \n-\tif (repo_submodule_init(&subrepo, superproject, sub)) {\n+\tif (repo_submodule_init(subrepo, superproject, sub)) {\n \t\tgrep_read_unlock();\n+\t\tfree(p);\n \t\treturn 0;\n \t}\n \n-\trepo_read_gitmodules(&subrepo);\n-\n-\t/*\n-\t * NEEDSWORK: This adds the submodule's object directory to the list of\n-\t * alternates for the single in-memory object store.  This has some bad\n-\t * consequences for memory (processed objects will never be freed) and\n-\t * performance (this increases the number of pack files git has to pay\n-\t * attention to, to the sum of the number of pack files in all the\n-\t * repositories processed so far).  This can be removed once the object\n-\t * store is no longer global and instead is a member of the repository\n-\t * object.\n-\t */\n-\tadd_to_alternates_memory(subrepo.objects->odb->path);\n+\trepo_read_gitmodules(subrepo);\n \tgrep_read_unlock();\n \n \tmemcpy(&subopt, opt, sizeof(subopt));\n-\tsubopt.repo = &subrepo;\n+\tsubopt.repo = subrepo;\n \n \tif (oid) {\n \t\tstruct object *object;\n@@ -455,10 +485,10 @@ static int grep_submodule(struct grep_opt *opt,\n \t\tunsigned long size;\n \t\tstruct strbuf base = STRBUF_INIT;\n \n-\t\tobject = parse_object_or_die(the_repository, oid, NULL);\n+\t\tobject = parse_object_or_die(subrepo, oid, NULL);\n \n \t\tgrep_read_lock();\n-\t\tdata = read_object_with_reference(&subrepo,\n+\t\tdata = read_object_with_reference(subrepo,\n \t\t\t\t\t\t  &object->oid, tree_type,\n \t\t\t\t\t\t  &size, NULL);\n \t\tgrep_read_unlock();\n@@ -478,7 +508,13 @@ static int grep_submodule(struct grep_opt *opt,\n \t\thit = grep_cache(&subopt, pathspec, cached);\n \t}\n \n-\trepo_clear(&subrepo);\n+\tgrep_lock();\n+\tp->all_work_added = 1;\n+\tif (p->work_counter == 0) {\n+\t\trepo_clear(&p->subrepo);\n+\t\tfree(p);\n+\t}\n+\tgrep_unlock();\n \treturn hit;\n }\n \n@@ -587,7 +623,7 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,\n \t\t\tvoid *data;\n \t\t\tunsigned long size;\n \n-\t\t\tdata = lock_and_read_oid_file(&entry.oid, &type, &size);\n+\t\t\tdata = lock_and_read_oid_file(repo, &entry.oid, &type, &size);\n \t\t\tif (!data)\n \t\t\t\tdie(_(\"unable to read tree (%s)\"),\n \t\t\t\t    oid_to_hex(&entry.oid));\ndiff --git a/grep.c b/grep.c\nindex cd952ef5d3..0b3f38aaae 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -10,9 +10,8 @@\n #include \"quote.h\"\n #include \"help.h\"\n \n-static int grep_source_load(struct grep_source *gs);\n-static int grep_source_is_binary(struct grep_source *gs,\n-\t\t\t\t struct index_state *istate);\n+static int grep_source_load(struct repository *r, struct grep_source *gs);\n+static int grep_source_is_binary(struct repository *r, struct grep_source *gs);\n \n static struct grep_opt grep_defaults;\n \n@@ -1714,7 +1713,7 @@ static int fill_textconv_grep(struct repository *r,\n \tsize_t size;\n \n \tif (!driver || !driver->textconv)\n-\t\treturn grep_source_load(gs);\n+\t\treturn grep_source_load(r, gs);\n \n \t/*\n \t * The textconv interface is intimately tied to diff_filespecs, so we\n@@ -1820,11 +1819,11 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle\n \tif (!textconv) {\n \t\tswitch (opt->binary) {\n \t\tcase GREP_BINARY_DEFAULT:\n-\t\t\tif (grep_source_is_binary(gs, opt->repo->index))\n+\t\t\tif (grep_source_is_binary(opt->repo, gs))\n \t\t\t\tbinary_match_only = 1;\n \t\t\tbreak;\n \t\tcase GREP_BINARY_NOMATCH:\n-\t\t\tif (grep_source_is_binary(gs, opt->repo->index))\n+\t\t\tif (grep_source_is_binary(opt->repo, gs))\n \t\t\t\treturn 0; /* Assume unmatch */\n \t\t\tbreak;\n \t\tcase GREP_BINARY_TEXT:\n@@ -2105,12 +2104,12 @@ void grep_source_clear_data(struct grep_source *gs)\n \t}\n }\n \n-static int grep_source_load_oid(struct grep_source *gs)\n+static int grep_source_load_oid(struct repository *r, struct grep_source *gs)\n {\n \tenum object_type type;\n \n \tgrep_read_lock();\n-\tgs->buf = read_object_file(gs->identifier, &type, &gs->size);\n+\tgs->buf = repo_read_object_file(r, gs->identifier, &type, &gs->size);\n \tgrep_read_unlock();\n \n \tif (!gs->buf)\n@@ -2154,7 +2153,7 @@ static int grep_source_load_file(struct grep_source *gs)\n \treturn 0;\n }\n \n-static int grep_source_load(struct grep_source *gs)\n+static int grep_source_load(struct repository *r, struct grep_source *gs)\n {\n \tif (gs->buf)\n \t\treturn 0;\n@@ -2163,7 +2162,7 @@ static int grep_source_load(struct grep_source *gs)\n \tcase GREP_SOURCE_FILE:\n \t\treturn grep_source_load_file(gs);\n \tcase GREP_SOURCE_OID:\n-\t\treturn grep_source_load_oid(gs);\n+\t\treturn grep_source_load_oid(r, gs);\n \tcase GREP_SOURCE_BUF:\n \t\treturn gs->buf ? 0 : -1;\n \t}\n@@ -2184,14 +2183,13 @@ void grep_source_load_driver(struct grep_source *gs,\n \tgrep_attr_unlock();\n }\n \n-static int grep_source_is_binary(struct grep_source *gs,\n-\t\t\t\t struct index_state *istate)\n+static int grep_source_is_binary(struct repository *r, struct grep_source *gs)\n {\n-\tgrep_source_load_driver(gs, istate);\n+\tgrep_source_load_driver(gs, r->index);\n \tif (gs->driver->binary != -1)\n \t\treturn gs->driver->binary;\n \n-\tif (!grep_source_load(gs))\n+\tif (!grep_source_load(r, gs))\n \t\treturn buffer_is_binary(gs->buf, gs->size);\n \n \treturn 0;\n-- \n2.23.0\n\n"},{"id":"382569","messageId":"xmqq36gt5qhr.fsf@gitster-ct.c.googlers.com","threadId":"51872","inReplyTo":"cover.1568771073.git.matheus.bernardino@usp.br","subject":"Re: [RFC PATCH 0/3] grep: don't add subrepos to in-memory alternates","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-09-18T19:55:12Z","receivedAt":"2019-09-18T19:55:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matheus Tavares <matheus.bernardino@usp.br> writes:\n\n> Make git-grep --recurse-submodules stop adding subrepos to the in-memory\n> alternates list and, instead, pass a reference to the subrepo struct\n> down to the threads.\n\nNice.  This is done by updating all the codepaths used by grep to\nuse the lower-level helper functions that can take a repository\ninstance and/or an object store instance that is not the one tied to\nthe top-level repository?  Quite nice.\n\n> - textconv cache is written to the_repository's object database even for\n>   submodules. Should it perhaps be written to submodules' odb instead?\n\nYou mention \"is written\", but that is what happens upon a cache\nmiss.  Before the code notices a cache miss, it must be checking if\na cached result is available.  In which odb is it done?  Writing\nthat follow the miss should happen to the same one, or the cache is\nnot very effective.\n\nBecause you would want the cache to be effective, after running \"git\ngrep --recurse-submodules\" from the top-level, when you chdir down\nto the submodule and say \"git grep\" to dig further, the answer to\nyour question is most likely \"yes\".\n\n> - Considering the following call chain: grep_source_load_driver() >\n>   userdiff_find_by_path() > git_check_attr() > collect_some_attrs() >\n>   prepare_attr_stack() > bootstrap_attr_stack():\n>\n>   * The last function tries to read the attributes from the\n>     .gitattributes and .git/info/attributes files of the_repository.\n>     However, for paths inside the submodule, shouldn't it try to read\n>     these files from the submodule?\n\nYes, I think all of those would have worked correctly if we forked a\nseparate \"git grep\" inside submodule repository, but in the rush to\n\"do everything in process\", many things like these are not done\ncorrectly.  As there is only one attribute cache IIUC, invalidating\nthe whole cache for the top-level and replacing it with the one for\na submodule, every time we cross the module boundary, would probably\nhave a negative effect on the performance, and I am not sure what\nwould happen if you run more than one threads working in different\nrepositories (i.e. top-level and submodules).\n\n>   * This function will also call: read_attr() > read_attr_from_index() >\n>     read_blob_data_from_index() which might, in turn, call\n>     read_object_file(). Shouldn't we pass the subrepo to it so that it\n>     can call repo_read_object_file()? (Again, for paths inside the\n>     submodule, read_object_file() won't be able to find the object as\n>     we won't be adding to alternates anymore.)\n\nDitto.\n\nThanks.\n"},{"id":"382580","messageId":"CAHd-oW4u+iPFMvSGNvSJxfPLE34dQQ3x5_aQ-Y4Pd99FXR1p7Q@mail.gmail.com","threadId":"51872","inReplyTo":"xmqq36gt5qhr.fsf@gitster-ct.c.googlers.com","subject":"Re: [RFC PATCH 0/3] grep: don't add subrepos to in-memory alternates","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2019-09-19T05:18:57Z","receivedAt":"2019-09-19T05:19:11Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Wed, Sep 18, 2019 at 4:55 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Matheus Tavares <matheus.bernardino@usp.br> writes:\n>\n[...]\n> > - textconv cache is written to the_repository's object database even for\n> >   submodules. Should it perhaps be written to submodules' odb instead?\n>\n> You mention \"is written\", but that is what happens upon a cache\n> miss.  Before the code notices a cache miss, it must be checking if\n> a cached result is available.  In which odb is it done?  Writing\n> that follow the miss should happen to the same one, or the cache is\n> not very effective.\n\nRight. Both writing and reading operations on the textconv cache\nalways happen in the_repository's odb.\n\n> Because you would want the cache to be effective, after running \"git\n> grep --recurse-submodules\" from the top-level, when you chdir down\n> to the submodule and say \"git grep\" to dig further, the answer to\n> your question is most likely \"yes\".\n\nOK, makes sense. To keep this series simple, I think I'll try to work\non this in a following patchset.\n\n> > - Considering the following call chain: grep_source_load_driver() >\n> >   userdiff_find_by_path() > git_check_attr() > collect_some_attrs() >\n> >   prepare_attr_stack() > bootstrap_attr_stack():\n> >\n> >   * The last function tries to read the attributes from the\n> >     .gitattributes and .git/info/attributes files of the_repository.\n> >     However, for paths inside the submodule, shouldn't it try to read\n> >     these files from the submodule?\n>\n> Yes, I think all of those would have worked correctly if we forked a\n> separate \"git grep\" inside submodule repository, but in the rush to\n> \"do everything in process\", many things like these are not done\n> correctly.  As there is only one attribute cache IIUC, invalidating\n> the whole cache for the top-level and replacing it with the one for\n> a submodule, every time we cross the module boundary, would probably\n> have a negative effect on the performance, and I am not sure what\n> would happen if you run more than one threads working in different\n> repositories (i.e. top-level and submodules).\n\nHmm, I may have gotten a little confused here. Are you talking about\nthe attributes stack (which contains .gitattributes and\ninfo/attributes)? If so, isn't this stack already rebuild for every\npath? I mean, by the previous call chain it seems to me that at least\nthese two files are reread for every path.\n\n> >   * This function will also call: read_attr() > read_attr_from_index() >\n> >     read_blob_data_from_index() which might, in turn, call\n> >     read_object_file(). Shouldn't we pass the subrepo to it so that it\n> >     can call repo_read_object_file()? (Again, for paths inside the\n> >     submodule, read_object_file() won't be able to find the object as\n> >     we won't be adding to alternates anymore.)\n\nJust as a side note, I noticed another problem with this: in a\nsubmodule with a non-checked-out .gitattributes, `git grep --textconv`\nwill successfully retrieve the file from the index. But running `git\ngrep --recurse-submodules --textconv` from the superproject won't. The\nproblem is that read_blob_data_from_index() doesn't have the\nrepository struct and thus cannot strip the subrepo_prefix from the\npath (so it won't find it in the subrepo's index). I'll try to fix\nthis in this series as well.\n"},{"id":"382666","messageId":"xmqqh857vsqz.fsf@gitster-ct.c.googlers.com","threadId":"51872","inReplyTo":"CAHd-oW4u+iPFMvSGNvSJxfPLE34dQQ3x5_aQ-Y4Pd99FXR1p7Q@mail.gmail.com","subject":"Re: [RFC PATCH 0/3] grep: don't add subrepos to in-memory alternates","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-09-20T16:26:28Z","receivedAt":"2019-09-20T16:26:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matheus Tavares Bernardino <matheus.bernardino@usp.br> writes:\n\n> Hmm, I may have gotten a little confused here. Are you talking\n> about the attributes stack (which contains .gitattributes and\n> info/attributes)?  If so, isn't this stack already rebuild for\n> every path? I mean, by the previous call chain it seems to me that\n> at least these two files are reread for every path.\n\nYes, but for the switch that happens when coming out of a normal\ndirectory and then descending into another normal directory is just\nto pop the entries from the directory hierarchy we are getting out\nof, and then pushing the entries from the new directory hierarchy.\nWe would not be discarding and rereading $GIT_DIR/info/ or the\n.gitattribute file from the top-level of the working tree.\n\nDescending into a submodule is fundamentally and completely\ndifferent.  None of the attributes defined in the superproject\nshould affect the paths in the submodule, as it is a totally\nseparate project, oblivious to the existence of enclosing the\nsuperproject. \n\nEvery time we look up attributes for a path in a project (either the\nsuperproject or a submodule) that is different from the one we did a\nlook-up the last time, we'd not just need to pop a handful and push\na handful, but need to rebuild the stack completely from scratch,\nno?\n"},{"id":"382728","messageId":"CAHd-oW5iEQarYVxEXoTG-ua2zdoybTrSjCBKtO0YT292fm0NQQ@mail.gmail.com","threadId":"51872","inReplyTo":"xmqqh857vsqz.fsf@gitster-ct.c.googlers.com","subject":"Re: [RFC PATCH 0/3] grep: don't add subrepos to in-memory alternates","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2019-09-21T20:34:35Z","receivedAt":"2019-09-21T20:34:49Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Fri, Sep 20, 2019 at 1:26 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Matheus Tavares Bernardino <matheus.bernardino@usp.br> writes:\n>\n> > Hmm, I may have gotten a little confused here. Are you talking\n> > about the attributes stack (which contains .gitattributes and\n> > info/attributes)?  If so, isn't this stack already rebuild for\n> > every path? I mean, by the previous call chain it seems to me that\n> > at least these two files are reread for every path.\n>\n> Yes, but for the switch that happens when coming out of a normal\n> directory and then descending into another normal directory is just\n> to pop the entries from the directory hierarchy we are getting out\n> of, and then pushing the entries from the new directory hierarchy.\n> We would not be discarding and rereading $GIT_DIR/info/ or the\n> .gitattribute file from the top-level of the working tree.\n\nRight, this would be the best way of doing it. However, I think this\nis not how it's currently implemented. I if correctly understood the\ncode in this call chain:\n\ngrep_source_load_driver() >  userdiff_find_by_path() >\ngit_check_attr() > collect_some_attrs() > prepare_attr_stack() >\nbootstrap_attr_stack()\n\nit seems that the whole stack is being rebuild for every path (even\nfor paths descending in the same superproject or submodule). So\n$GIT_DIR/info/ and .gitattributes are being discarded and reread every\ntime :(\n\n> Descending into a submodule is fundamentally and completely\n> different.  None of the attributes defined in the superproject\n> should affect the paths in the submodule, as it is a totally\n> separate project, oblivious to the existence of enclosing the\n> superproject.\n\nI think we currently have a bug here as the attributes from the\nsuperproject *are* affecting the paths in the submodule. Here is a\nsmall script I wrote to test this: https://gitlab.com/snippets/1896951\n\nThe cause of this problem is that boostrap_attr_stack() doesn't read\n\"<subrepo_prefix>/.gitattributes\" but just \".gitattributes\", always\ngetting the superproject's file not the suprepo's. Yet another problem\nis that when this file is not present and we need to retrieve it from\nthe index, this function calls read_attr() > read_attr_from_index() >\nread_blob_data_from_index(). The last one always reads from\nthe_repository's odb, so it won't ever find the subrepo's\n\".gitattributes\".\n\nAnd a third bug is that when reading attributes of paths inside\nsubrepo's directories, from index, we call read_attr_from_index() with\na path such as \"<subrepo_prefix>/<subdir>/.gitattributes\". However,\nthe subrepo_prefix should be stripped when looking in the subrepo's\nindex, otherwise there will be no matches.\n\nTo fix these three problems, I think we would need to pass on a struct\nrepository in these call chains. But this would require a very big\nmodification as there are many places that can lead to one of them...\nAnd there're corner cases such as index_stream_convert_blob() which\nwould need to receive a struct repository but it always writes to\nthe_repo, which would be kind of inconsistent. Do you think this would\nbe a good solution or should I try something else?\n"},{"id":"382729","messageId":"CALN-EhSnadjdgdpbuZhj5tDQe5a8uQ6CBnDgh4CZ-AG3JuU_LA@mail.gmail.com","threadId":"51872","inReplyTo":"fc5f5fe95c10ac66d1234a025122c8866965021f.1568771073.git.matheus.bernardino@usp.br","subject":"Re: [RFC PATCH 3/3] grep: don't add submodules to the alternates list","fromName":"Brandon Williams","fromEmail":"bwilliamseng@gmail.com","sentAt":"2019-09-21T21:58:26Z","receivedAt":"2019-09-21T21:58:39Z","isPatch":true,"sender":{"key":"bwilliamseng@gmail.com","avatar":null},"body":"On Tue, Sep 17, 2019 at 6:56 PM Matheus Tavares\n<matheus.bernardino@usp.br> wrote:\n>\n> When grepping with --recurse-submodules, the object directory of the\n> submodule is added to the in-memory alternates list. This makes git need\n> to watch out for more packfiles which might bring bad consequences for\n> memory and performance.\n>\n> Now that raw_object_store is a member of the struct repository, it's\n> possible to use the submodules' instances directly, without the need to\n> add its odbs to the alternates list. So let's instead pass the subrepo\n> down to the threads and replace function calls which use\n> the_repository by default for their more general counterparts. Also,\n> submodule cleanup must be refactored as calling repo_clear() at the end\n> of grep_submodules(), might free the subrepo's resources before threads\n> have finished working on it. To do that, let's keep track of the\n> workers progress and only call repo_clear() when it's safe to do so.\n\nThis is incredibly exciting. Glad to see the effort that were started years ago\nto finally come to fruition.\n"},{"id":"383076","messageId":"xmqqtv8xksrs.fsf@gitster-ct.c.googlers.com","threadId":"51872","inReplyTo":"CAHd-oW5iEQarYVxEXoTG-ua2zdoybTrSjCBKtO0YT292fm0NQQ@mail.gmail.com","subject":"Re: [RFC PATCH 0/3] grep: don't add subrepos to in-memory alternates","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-09-28T03:24:07Z","receivedAt":"2019-09-28T03:24:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matheus Tavares Bernardino <matheus.bernardino@usp.br> writes:\n\n> Right, this would be the best way of doing it. However, I think this\n> is not how it's currently implemented. I if correctly understood the\n> code in this call chain:\n>\n> grep_source_load_driver() >  userdiff_find_by_path() >\n> git_check_attr() > collect_some_attrs() > prepare_attr_stack() >\n> bootstrap_attr_stack()\n>\n> it seems that the whole stack is being rebuild for every path (even\n> for paths descending in the same superproject or submodule).\n\nbootstrap is guarded with \"if (*stack) return;\" and prepare knows to\nrewind to the common level and push down the new ones, no?\n\nAt least that is how I remember writing the first version of it.\nHave we broken the design over time, I wonder?\n\n"},{"id":"383082","messageId":"CAHd-oW6uG1fap-T4UF17bJmjoHAqWCDq9KbY+_8a3cEnnfATxg@mail.gmail.com","threadId":"51872","inReplyTo":"xmqqtv8xksrs.fsf@gitster-ct.c.googlers.com","subject":"Re: [RFC PATCH 0/3] grep: don't add subrepos to in-memory alternates","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2019-09-28T04:20:14Z","receivedAt":"2019-09-28T04:23:36Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Sat, Sep 28, 2019 at 12:24 AM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Matheus Tavares Bernardino <matheus.bernardino@usp.br> writes:\n>\n> > Right, this would be the best way of doing it. However, I think this\n> > is not how it's currently implemented. I if correctly understood the\n> > code in this call chain:\n> >\n> > grep_source_load_driver() >  userdiff_find_by_path() >\n> > git_check_attr() > collect_some_attrs() > prepare_attr_stack() >\n> > bootstrap_attr_stack()\n> >\n> > it seems that the whole stack is being rebuild for every path (even\n> > for paths descending in the same superproject or submodule).\n>\n> bootstrap is guarded with \"if (*stack) return;\" and prepare knows to\n> rewind to the common level and push down the new ones, no?\n\nRight, I've somehow missed this guard and the fact that 'check' is\nstatic at userdiff_find_by_path() so the stack is persistent. Thanks\nfor pointing this out and sorry for the noise.\n"}]}