{"thread":{"id":"62470","subject":"[RFC PATCH 0/1] maintenance: separate parallelism safe and unsafe tasks","startedAt":"2024-11-08T17:31:23Z","lastAt":"2024-11-18T06:59:01Z","messageCount":13,"participants":["Calvin Wan","Junio C Hamano","Patrick Steinhardt"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"506882","messageId":"20241108173112.1240584-1-calvinwan@google.com","threadId":"62470","inReplyTo":null,"subject":"[RFC PATCH 0/1] maintenance: separate parallelism safe and unsafe tasks","fromName":"Calvin Wan","fromEmail":"calvinwan@google.com","sentAt":"2024-11-08T17:31:11Z","receivedAt":"2024-11-08T17:31:23Z","isPatch":true,"sender":{"key":"calvinwan@google.com","avatar":"https://avatars.githubusercontent.com/u/92547554?v=4"},"body":"Unless a user changes the config options `gc.autoDetach` or\n`maintenance.autoDetach`, Git by default runs maintenance and gc tasks\nin the background. This is because maintenance and gc tasks, especially\nin large repositories, can take a long time to run. Therefore, they\n_should_ be as nonintrusive as possible to other Git commands,\nespecially porcelain ones.\n\nHowever, this is not the case as discovered earlier[1] -- certain\nmaintenance and gc tasks are not safe to run in parallel with other\ncommands. The consequences of such are that scripts with commands that\ntrigger maintenance/gc can race and crash. Users can also run into\nunexpected errors from porcelain commands that touch common files such\nas HEAD.lock, unaware that a background maintenance/gc task is the one\nholding the lock.\n\nAs Patrick points out[2], the two unsafe commands are `git reflog expire\n--all`, invoked by gc, and `git pack-refs --all --prune`, invoked by\nmaintenance. We can create two buckets for subtasks -- one for async\nsafe tasks and one for async unsafe tasks. When `[maintenance,\ngc].autoDetach` is not set or set to true, maintenance will run the\nunsafe tasks first before detaching to run the safe tasks.\n\nThis series is in RFC to see if the general direction of the patch is\ngoing in the right direction. I left a couple of WIPs in the first patch\ndocumenting what still needs to be done if the direction is palatable.\n\n[1] https://lore.kernel.org/git/CAFySSZBCKUiY5DO3fz340a0dTb0zUDNKxaTYU0LAqsBD2RMwSg@mail.gmail.com/\n[2] https://lore.kernel.org/git/ZxeilMDwq0Z3krhz@pks.im\n\nCalvin Wan (1):\n maintenance: separate parallelism safe and unsafe tasks\n\n builtin/gc.c | 173 ++++++++++++++++++++++++++++++++++++-----\n t/t7900-maintenance.sh | 24 +++---\n 2 files changed, 168 insertions(+), 29 deletions(-)\n\n-- \n2.47.0.277.g8800431eea-goog\n\n"},{"id":"506883","messageId":"20241108173112.1240584-2-calvinwan@google.com","threadId":"62470","inReplyTo":"20241108173112.1240584-1-calvinwan@google.com","subject":"[RFC PATCH 1/1] maintenance: separate parallelism safe and unsafe tasks","fromName":"Calvin Wan","fromEmail":"calvinwan@google.com","sentAt":"2024-11-08T17:31:12Z","receivedAt":"2024-11-08T17:31:29Z","isPatch":true,"sender":{"key":"calvinwan@google.com","avatar":"https://avatars.githubusercontent.com/u/92547554?v=4"},"body":"Certain maintenance tasks and subtasks within gc are unsafe to run in\nparallel with other commands because they lock up files such as\nHEAD. Therefore, tasks are marked whether they are async safe or\nnot. Async unsafe tasks are run first in the same process before running\nasync safe tasks in parallel.\n\nSince the gc task is partially safe, there are two new tasks -- an async\nsafe gc task and an async unsafe gc task. In order to properly invoke\nthis in gc, `--run-async-safe` and `--run-async-unsafe` have been added\nas options to gc. Maintenance will only run these two new tasks if it\nwas set to detach, otherwise the original gc task runs.\n\nAdditionally, if a user passes in tasks thru `--task`, we do not attempt\nto run separate async/sync tasks since the user sets the order of tasks.\n\nWIP: automatically run gc unsafe tasks when gc is invoked but not from\n maintenance\nWIP: edit test in t7900-maintainance.sh to match new functionality\nWIP: add additional documentation for new options and functionality\n\nSigned-off-by: Calvin Wan \n---\n builtin/gc.c | 173 ++++++++++++++++++++++++++++++++++++-----\n t/t7900-maintenance.sh | 24 +++---\n 2 files changed, 168 insertions(+), 29 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex d52735354c..375d304c42 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -668,6 +668,8 @@ struct repository *repo UNUSED)\n \tpid_t pid;\n \tint daemonized = 0;\n \tint keep_largest_pack = -1;\n+\tint run_async_safe = 0;\n+\tint run_async_unsafe = 0;\n \ttimestamp_t dummy;\n \tstruct child_process rerere_cmd = CHILD_PROCESS_INIT;\n \tstruct maintenance_run_opts opts = MAINTENANCE_RUN_OPTS_INIT;\n@@ -694,6 +696,10 @@ struct repository *repo UNUSED)\n \t\t\t PARSE_OPT_NOCOMPLETE),\n \t\tOPT_BOOL(0, \"keep-largest-pack\", &keep_largest_pack,\n \t\t\t N_(\"repack all other packs except the largest pack\")),\n+\t\tOPT_BOOL(0, \"run-async-safe\", &run_async_safe,\n+\t\t\t N_(\"run only background safe gc tasks, should only be invoked thru maintenance\")),\n+\t\tOPT_BOOL(0, \"run-async-unsafe\", &run_async_unsafe,\n+\t\t\t N_(\"run only background unsafe gc tasks, should only be invoked thru maintenance\")),\n \t\tOPT_END()\n \t};\n \n@@ -718,6 +724,9 @@ struct repository *repo UNUSED)\n \t\t\t builtin_gc_usage, 0);\n \tif (argc > 0)\n \t\tusage_with_options(builtin_gc_usage, builtin_gc_options);\n+\t\n+\tif (run_async_safe && run_async_unsafe)\n+\t\tdie(_(\"--run-async-safe cannot be used with --run-async-unsafe\"));\n \n \tif (prune_expire_arg != prune_expire_sentinel) {\n \t\tfree(cfg.prune_expire);\n@@ -815,7 +824,12 @@ struct repository *repo UNUSED)\n \t\tatexit(process_log_file_at_exit);\n \t}\n \n-\tgc_before_repack(&opts, &cfg);\n+\tif (run_async_unsafe) {\n+\t\tgc_before_repack(&opts, &cfg);\n+\t\tgoto out;\n+\t} else if (!run_async_safe)\n+\t\tgc_before_repack(&opts, &cfg);\n+\t\n \n \tif (!repository_format_precious_objects) {\n \t\tstruct child_process repack_cmd = CHILD_PROCESS_INIT;\n@@ -1052,6 +1066,46 @@ static int maintenance_task_prefetch(struct maintenance_run_opts *opts,\n \treturn 0;\n }\n \n+static int maintenance_task_unsafe_gc(struct maintenance_run_opts *opts,\n+\t\t\t\t struct gc_config *cfg UNUSED)\n+{\n+\tstruct child_process child = CHILD_PROCESS_INIT;\n+\n+\tchild.git_cmd = child.close_object_store = 1;\n+\tstrvec_push(&child.args, \"gc\");\n+\n+\tif (opts->auto_flag)\n+\t\tstrvec_push(&child.args, \"--auto\");\n+\tif (opts->quiet)\n+\t\tstrvec_push(&child.args, \"--quiet\");\n+\telse\n+\t\tstrvec_push(&child.args, \"--no-quiet\");\n+\tstrvec_push(&child.args, \"--no-detach\");\n+\tstrvec_push(&child.args, \"--run-async-unsafe\");\n+\n+\treturn run_command(&child);\n+}\n+\n+static int maintenance_task_safe_gc(struct maintenance_run_opts *opts,\n+\t\t\t\t struct gc_config *cfg UNUSED)\n+{\n+\tstruct child_process child = CHILD_PROCESS_INIT;\n+\n+\tchild.git_cmd = child.close_object_store = 1;\n+\tstrvec_push(&child.args, \"gc\");\n+\n+\tif (opts->auto_flag)\n+\t\tstrvec_push(&child.args, \"--auto\");\n+\tif (opts->quiet)\n+\t\tstrvec_push(&child.args, \"--quiet\");\n+\telse\n+\t\tstrvec_push(&child.args, \"--no-quiet\");\n+\tstrvec_push(&child.args, \"--no-detach\");\n+\tstrvec_push(&child.args, \"--run-async-safe\");\n+\n+\treturn run_command(&child);\n+}\n+\n static int maintenance_task_gc(struct maintenance_run_opts *opts,\n \t\t\t struct gc_config *cfg UNUSED)\n {\n@@ -1350,6 +1404,7 @@ struct maintenance_task {\n \tconst char *name;\n \tmaintenance_task_fn *fn;\n \tmaintenance_auto_fn *auto_condition;\n+\tunsigned daemonize_safe;\n \tunsigned enabled:1;\n \n \tenum schedule_priority schedule;\n@@ -1362,6 +1417,8 @@ enum maintenance_task_label {\n \tTASK_PREFETCH,\n \tTASK_LOOSE_OBJECTS,\n \tTASK_INCREMENTAL_REPACK,\n+\tTASK_UNSAFE_GC,\n+\tTASK_SAFE_GC,\n \tTASK_GC,\n \tTASK_COMMIT_GRAPH,\n \tTASK_PACK_REFS,\n@@ -1370,36 +1427,62 @@ enum maintenance_task_label {\n \tTASK__COUNT\n };\n \n+enum maintenance_task_daemonize_safe {\n+\tUNSAFE,\n+\tSAFE,\n+};\n+\n static struct maintenance_task tasks[] = {\n \t[TASK_PREFETCH] = {\n \t\t\"prefetch\",\n \t\tmaintenance_task_prefetch,\n+\t\tNULL,\n+\t\tSAFE,\n \t},\n \t[TASK_LOOSE_OBJECTS] = {\n \t\t\"loose-objects\",\n \t\tmaintenance_task_loose_objects,\n \t\tloose_object_auto_condition,\n+\t\tSAFE,\n \t},\n \t[TASK_INCREMENTAL_REPACK] = {\n \t\t\"incremental-repack\",\n \t\tmaintenance_task_incremental_repack,\n \t\tincremental_repack_auto_condition,\n+\t\tSAFE,\n+\t},\n+\t[TASK_UNSAFE_GC] = {\n+\t\t\"unsafe-gc\",\n+\t\tmaintenance_task_unsafe_gc,\n+\t\tneed_to_gc,\n+\t\tUNSAFE,\n+\t\t0,\n+\t},\n+\t[TASK_SAFE_GC] = {\n+\t\t\"safe-gc\",\n+\t\tmaintenance_task_safe_gc,\n+\t\tneed_to_gc,\n+\t\tSAFE,\n+\t\t0,\n \t},\n \t[TASK_GC] = {\n \t\t\"gc\",\n \t\tmaintenance_task_gc,\n \t\tneed_to_gc,\n+\t\tUNSAFE,\n \t\t1,\n \t},\n \t[TASK_COMMIT_GRAPH] = {\n \t\t\"commit-graph\",\n \t\tmaintenance_task_commit_graph,\n \t\tshould_write_commit_graph,\n+\t\tSAFE,\n \t},\n \t[TASK_PACK_REFS] = {\n \t\t\"pack-refs\",\n \t\tmaintenance_task_pack_refs,\n \t\tpack_refs_condition,\n+\t\tUNSAFE,\n \t},\n };\n \n@@ -1411,10 +1494,18 @@ static int compare_tasks_by_selection(const void *a_, const void *b_)\n \treturn b->selected_order - a->selected_order;\n }\n \n+static int compare_tasks_by_safeness(const void *a_, const void *b_)\n+{\n+\tconst struct maintenance_task *a = a_;\n+\tconst struct maintenance_task *b = b_;\n+\n+\treturn a->daemonize_safe - b->daemonize_safe;\n+}\n+\n static int maintenance_run_tasks(struct maintenance_run_opts *opts,\n \t\t\t\t struct gc_config *cfg)\n {\n-\tint i, found_selected = 0;\n+\tint i, j, found_selected = 0;\n \tint result = 0;\n \tstruct lock_file lk;\n \tstruct repository *r = the_repository;\n@@ -1436,6 +1527,57 @@ static int maintenance_run_tasks(struct maintenance_run_opts *opts,\n \t}\n \tfree(lock_path);\n \n+\tfor (i = 0; !found_selected && i < TASK__COUNT; i++)\n+\t\tfound_selected = tasks[i].selected_order >= 0;\n+\n+\tif (found_selected)\n+\t\tQSORT(tasks, TASK__COUNT, compare_tasks_by_selection);\n+\telse if (opts->detach > 0) {\n+\t\t/* \n+\t\t * Part of gc is unsafe to run in the background, so\n+\t\t * separate out gc into unsafe and safe tasks.\n+\t\t * \n+\t\t * Run unsafe tasks, including unsafe maintenance tasks,\n+\t\t * before daemonizing and running safe tasks.\n+\t\t */\n+\n+\t\tif (tasks[TASK_GC].enabled) {\n+\t\t\ttasks[TASK_GC].enabled = 0;\n+\t\t\ttasks[TASK_UNSAFE_GC].enabled = 1;\n+\t\t\ttasks[TASK_UNSAFE_GC].schedule = tasks[TASK_GC].schedule;\n+\t\t\ttasks[TASK_SAFE_GC].enabled = 1;\n+\t\t\ttasks[TASK_SAFE_GC].schedule = tasks[TASK_GC].schedule;\n+\t\t}\n+\n+\t\tQSORT(tasks, TASK__COUNT, compare_tasks_by_safeness);\n+\n+\t\tfor (j = 0; j < TASK__COUNT; j++) {\n+\t\t\tif (tasks[j].daemonize_safe == SAFE)\n+\t\t\t\tbreak;\n+\n+\t\t\tif (found_selected && tasks[j].selected_order < 0)\n+\t\t\t\tcontinue;\n+\n+\t\t\tif (!found_selected && !tasks[j].enabled)\n+\t\t\t\tcontinue;\n+\n+\t\t\tif (opts->auto_flag &&\n+\t\t\t(!tasks[j].auto_condition ||\n+\t\t\t!tasks[j].auto_condition(cfg)))\n+\t\t\t\tcontinue;\n+\n+\t\t\tif (opts->schedule && tasks[j].schedule < opts->schedule)\n+\t\t\t\tcontinue;\n+\n+\t\t\ttrace2_region_enter(\"maintenance\", tasks[j].name, r);\n+\t\t\tif (tasks[j].fn(opts, cfg)) {\n+\t\t\t\terror(_(\"task '%s' failed\"), tasks[j].name);\n+\t\t\t\tresult = 1;\n+\t\t\t}\n+\t\t\ttrace2_region_leave(\"maintenance\", tasks[j].name, r);\n+\t\t}\n+\t}\n+\n \t/* Failure to daemonize is ok, we'll continue in foreground. */\n \tif (opts->detach > 0) {\n \t\ttrace2_region_enter(\"maintenance\", \"detach\", the_repository);\n@@ -1443,33 +1585,28 @@ static int maintenance_run_tasks(struct maintenance_run_opts *opts,\n \t\ttrace2_region_leave(\"maintenance\", \"detach\", the_repository);\n \t}\n \n-\tfor (i = 0; !found_selected && i < TASK__COUNT; i++)\n-\t\tfound_selected = tasks[i].selected_order >= 0;\n-\n-\tif (found_selected)\n-\t\tQSORT(tasks, TASK__COUNT, compare_tasks_by_selection);\n-\n-\tfor (i = 0; i < TASK__COUNT; i++) {\n-\t\tif (found_selected && tasks[i].selected_order < 0)\n+\tfor (j = j; j < TASK__COUNT; j++) {\n+\t\tif (found_selected && tasks[j].selected_order < 0)\n \t\t\tcontinue;\n \n-\t\tif (!found_selected && !tasks[i].enabled)\n+\t\tif (!found_selected && !tasks[j].enabled)\n \t\t\tcontinue;\n \n \t\tif (opts->auto_flag &&\n-\t\t (!tasks[i].auto_condition ||\n-\t\t !tasks[i].auto_condition(cfg)))\n+\t\t (!tasks[j].auto_condition ||\n+\t\t !tasks[j].auto_condition(cfg)))\n \t\t\tcontinue;\n \n-\t\tif (opts->schedule && tasks[i].schedule < opts->schedule)\n+\t\tif (opts->schedule && tasks[j].schedule < opts->schedule)\n \t\t\tcontinue;\n \n-\t\ttrace2_region_enter(\"maintenance\", tasks[i].name, r);\n-\t\tif (tasks[i].fn(opts, cfg)) {\n-\t\t\terror(_(\"task '%s' failed\"), tasks[i].name);\n+\t\t// fprintf(stderr, \"running %i: %s\\n\",j , tasks[j].name);\n+\t\ttrace2_region_enter(\"maintenance\", tasks[j].name, r);\n+\t\tif (tasks[j].fn(opts, cfg)) {\n+\t\t\terror(_(\"task '%s' failed\"), tasks[j].name);\n \t\t\tresult = 1;\n \t\t}\n-\t\ttrace2_region_leave(\"maintenance\", tasks[i].name, r);\n+\t\ttrace2_region_leave(\"maintenance\", tasks[j].name, r);\n \t}\n \n \trollback_lock_file(&lk);\ndiff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\nindex c224c8450c..5bbd07ec30 100755\n--- a/t/t7900-maintenance.sh\n+++ b/t/t7900-maintenance.sh\n@@ -43,17 +43,19 @@ test_expect_success 'help text' '\n \ttest_grep \"usage: git maintenance\" err\n '\n \n-test_expect_success 'run [--auto|--quiet]' '\n-\tGIT_TRACE2_EVENT=\"$(pwd)/run-no-auto.txt\" \\\n-\t\tgit maintenance run 2>/dev/null &&\n-\tGIT_TRACE2_EVENT=\"$(pwd)/run-auto.txt\" \\\n-\t\tgit maintenance run --auto 2>/dev/null &&\n-\tGIT_TRACE2_EVENT=\"$(pwd)/run-no-quiet.txt\" \\\n-\t\tgit maintenance run --no-quiet 2>/dev/null &&\n-\ttest_subcommand git gc --quiet --no-detach /dev/null &&\n+# \tGIT_TRACE2_EVENT=\"$(pwd)/run-auto.txt\" \\\n+# \t\tgit maintenance run --auto 2>/dev/null &&\n+# \tGIT_TRACE2_EVENT=\"$(pwd)/run-no-quiet.txt\" \\\n+# \t\tgit maintenance run --no-quiet 2>/dev/null &&\n+# \ttest_subcommand git gc --quiet --no-detach writes:\n\n> However, this is not the case as discovered earlier[1] -- certain\n> maintenance and gc tasks are not safe to run in parallel with other\n> commands. The consequences of such are that scripts with commands that\n> trigger maintenance/gc can race and crash.\n>\n> Users can also run into\n> unexpected errors from porcelain commands that touch common files such\n> as HEAD.lock, unaware that a background maintenance/gc task is the one\n> holding the lock.\n\nThe symptom looks more like a controlled refusal of execution than a\ncrash.\n\n> As Patrick points out[2], the two unsafe commands are `git reflog expire\n> --all`, invoked by gc, and `git pack-refs --all --prune`, invoked by\n> maintenance. We can create two buckets for subtasks -- one for async\n> safe tasks and one for async unsafe tasks.\n\nI am not sure if they can be partitioned into black and white, but\nlet's see.\n\n> This series is in RFC to see if the general direction of the patch is\n> going in the right direction. I left a couple of WIPs in the first patch\n> documenting what still needs to be done if the direction is palatable.\n\n"},{"id":"506966","messageId":"ZzGtD4Jz9Wj6n0zH@pks.im","threadId":"62470","inReplyTo":"20241108173112.1240584-2-calvinwan@google.com","subject":"Re: [RFC PATCH 1/1] maintenance: separate parallelism safe and unsafe tasks","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-11-11T07:07:07Z","receivedAt":"2024-11-11T07:07:21Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Nov 08, 2024 at 05:31:12PM +0000, Calvin Wan wrote:\n> Certain maintenance tasks and subtasks within gc are unsafe to run in\n> parallel with other commands because they lock up files such as\n> HEAD.\n\nI don't think it is fair to classify this as \"unsafe\". Nothing is unsafe\nhere: we take locks to guard us against concurrent modifications.\nWhat you're having problems with is the fact that this safety mechanism\nworks as expected and keeps other processes from modifying locked the\ndata.\n\n> Therefore, tasks are marked whether they are async safe or\n> not. Async unsafe tasks are run first in the same process before running\n> async safe tasks in parallel.\n> \n> Since the gc task is partially safe, there are two new tasks -- an async\n> safe gc task and an async unsafe gc task. In order to properly invoke\n> this in gc, `--run-async-safe` and `--run-async-unsafe` have been added\n> as options to gc. Maintenance will only run these two new tasks if it\n> was set to detach, otherwise the original gc task runs.\n> \n> Additionally, if a user passes in tasks thru `--task`, we do not attempt\n> to run separate async/sync tasks since the user sets the order of tasks.\n> \n> WIP: automatically run gc unsafe tasks when gc is invoked but not from\n> maintenance\n> WIP: edit test in t7900-maintainance.sh to match new functionality\n> WIP: add additional documentation for new options and functionality\n> \n> Signed-off-by: Calvin Wan \n> ---\n> builtin/gc.c | 173 ++++++++++++++++++++++++++++++++++++-----\n> t/t7900-maintenance.sh | 24 +++---\n> 2 files changed, 168 insertions(+), 29 deletions(-)\n> \n> diff --git a/builtin/gc.c b/builtin/gc.c\n> index d52735354c..375d304c42 100644\n> --- a/builtin/gc.c\n> +++ b/builtin/gc.c\n\nIt might make sense to split out the git-gc(1) changes into a\npreparatory commit with its own set of tests.\n\n> @@ -815,7 +824,12 @@ struct repository *repo UNUSED)\n> \t\tatexit(process_log_file_at_exit);\n> \t}\n> \n> -\tgc_before_repack(&opts, &cfg);\n> +\tif (run_async_unsafe) {\n> +\t\tgc_before_repack(&opts, &cfg);\n> +\t\tgoto out;\n> +\t} else if (!run_async_safe)\n> +\t\tgc_before_repack(&opts, &cfg);\n> +\t\n> \n\nStyle: there should be curly braces around the `else if` here.\n\n> \tif (!repository_format_precious_objects) {\n> \t\tstruct child_process repack_cmd = CHILD_PROCESS_INIT;\n> @@ -1052,6 +1066,46 @@ static int maintenance_task_prefetch(struct maintenance_run_opts *opts,\n> \treturn 0;\n> }\n> \n> +static int maintenance_task_unsafe_gc(struct maintenance_run_opts *opts,\n> +\t\t\t\t struct gc_config *cfg UNUSED)\n> +{\n> +\tstruct child_process child = CHILD_PROCESS_INIT;\n> +\n> +\tchild.git_cmd = child.close_object_store = 1;\n> +\tstrvec_push(&child.args, \"gc\");\n> +\n> +\tif (opts->auto_flag)\n> +\t\tstrvec_push(&child.args, \"--auto\");\n> +\tif (opts->quiet)\n> +\t\tstrvec_push(&child.args, \"--quiet\");\n> +\telse\n> +\t\tstrvec_push(&child.args, \"--no-quiet\");\n> +\tstrvec_push(&child.args, \"--no-detach\");\n> +\tstrvec_push(&child.args, \"--run-async-unsafe\");\n> +\n> +\treturn run_command(&child);\n> +}\n> +\n> +static int maintenance_task_safe_gc(struct maintenance_run_opts *opts,\n> +\t\t\t\t struct gc_config *cfg UNUSED)\n> +{\n> +\tstruct child_process child = CHILD_PROCESS_INIT;\n> +\n> +\tchild.git_cmd = child.close_object_store = 1;\n> +\tstrvec_push(&child.args, \"gc\");\n> +\n> +\tif (opts->auto_flag)\n> +\t\tstrvec_push(&child.args, \"--auto\");\n> +\tif (opts->quiet)\n> +\t\tstrvec_push(&child.args, \"--quiet\");\n> +\telse\n> +\t\tstrvec_push(&child.args, \"--no-quiet\");\n> +\tstrvec_push(&child.args, \"--no-detach\");\n> +\tstrvec_push(&child.args, \"--run-async-safe\");\n> +\n> +\treturn run_command(&child);\n> +}\n\nThese two functions and `maintenance_task_gc()` all look exactly the\nsame. We should deduplicate them.\n\n> static int maintenance_task_gc(struct maintenance_run_opts *opts,\n> \t\t\t struct gc_config *cfg UNUSED)\n> {\n> @@ -1350,6 +1404,7 @@ struct maintenance_task {\n> \tconst char *name;\n> \tmaintenance_task_fn *fn;\n> \tmaintenance_auto_fn *auto_condition;\n> +\tunsigned daemonize_safe;\n\nWe can use the enum here to give readers a better hint what this\nvariable is about.\n\n> \tunsigned enabled:1;\n> \n> \tenum schedule_priority schedule;\n> @@ -1362,6 +1417,8 @@ enum maintenance_task_label {\n> \tTASK_PREFETCH,\n> \tTASK_LOOSE_OBJECTS,\n> \tTASK_INCREMENTAL_REPACK,\n> +\tTASK_UNSAFE_GC,\n> +\tTASK_SAFE_GC,\n> \tTASK_GC,\n> \tTASK_COMMIT_GRAPH,\n> \tTASK_PACK_REFS,\n> @@ -1370,36 +1427,62 @@ enum maintenance_task_label {\n> \tTASK__COUNT\n> };\n> \n> +enum maintenance_task_daemonize_safe {\n> +\tUNSAFE,\n> +\tSAFE,\n> +};\n\nThese names can conflict quite fast. Do we maybe want to rename them to\ne.g. `MAINTENANCE_TASK_DAEMONIZE_(SAFE|UNSAFE)`?\n\n> static struct maintenance_task tasks[] = {\n> \t[TASK_PREFETCH] = {\n> \t\t\"prefetch\",\n> \t\tmaintenance_task_prefetch,\n> +\t\tNULL,\n> +\t\tSAFE,\n> \t},\n\nIt might make sense to prepare these to take designated field\ninitializers in a preparatory commit.\n\n> \t[TASK_LOOSE_OBJECTS] = {\n> \t\t\"loose-objects\",\n> \t\tmaintenance_task_loose_objects,\n> \t\tloose_object_auto_condition,\n> +\t\tSAFE,\n> \t},\n> \t[TASK_INCREMENTAL_REPACK] = {\n> \t\t\"incremental-repack\",\n> \t\tmaintenance_task_incremental_repack,\n> \t\tincremental_repack_auto_condition,\n> +\t\tSAFE,\n> +\t},\n> +\t[TASK_UNSAFE_GC] = {\n> +\t\t\"unsafe-gc\",\n> +\t\tmaintenance_task_unsafe_gc,\n> +\t\tneed_to_gc,\n> +\t\tUNSAFE,\n> +\t\t0,\n> +\t},\n> +\t[TASK_SAFE_GC] = {\n> +\t\t\"safe-gc\",\n> +\t\tmaintenance_task_safe_gc,\n> +\t\tneed_to_gc,\n> +\t\tSAFE,\n> +\t\t0,\n> \t},\n\nHm. I wonder whether we really want to expose additional tasks to\naddress the issue, which feels like we're leaking implementation details\nto our users. Would it maybe be preferable to instead introduce a new\noptional callback function for every task that handles the pre-detach\nlogic?\n\nI wonder whether we also have to adapt the \"pack-refs\" task to be\nsynchronous instead of asynchronous?\n\n> @@ -1436,6 +1527,57 @@ static int maintenance_run_tasks(struct maintenance_run_opts *opts,\n> \t}\n> \tfree(lock_path);\n> \n> +\tfor (i = 0; !found_selected && i < TASK__COUNT; i++)\n> +\t\tfound_selected = tasks[i].selected_order >= 0;\n> +\n> +\tif (found_selected)\n> +\t\tQSORT(tasks, TASK__COUNT, compare_tasks_by_selection);\n> +\telse if (opts->detach > 0) {\n> +\t\t/* \n> +\t\t * Part of gc is unsafe to run in the background, so\n> +\t\t * separate out gc into unsafe and safe tasks.\n> +\t\t * \n> +\t\t * Run unsafe tasks, including unsafe maintenance tasks,\n> +\t\t * before daemonizing and running safe tasks.\n> +\t\t */\n> +\n> +\t\tif (tasks[TASK_GC].enabled) {\n> +\t\t\ttasks[TASK_GC].enabled = 0;\n> +\t\t\ttasks[TASK_UNSAFE_GC].enabled = 1;\n> +\t\t\ttasks[TASK_UNSAFE_GC].schedule = tasks[TASK_GC].schedule;\n> +\t\t\ttasks[TASK_SAFE_GC].enabled = 1;\n> +\t\t\ttasks[TASK_SAFE_GC].schedule = tasks[TASK_GC].schedule;\n> +\t\t}\n\nIf we did the above, then we could also get rid of the complexity here.\n\nThanks!\n\nPatrick\n"},{"id":"506967","messageId":"xmqq1pzich44.fsf@gitster.g","threadId":"62470","inReplyTo":"20241108173112.1240584-2-calvinwan@google.com","subject":"Re: [RFC PATCH 1/1] maintenance: separate parallelism safe and unsafe tasks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-11T08:24:59Z","receivedAt":"2024-11-11T08:25:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Calvin Wan writes:\n\n> +\tfor (j = j; j < TASK__COUNT; j++) {\n\nThis seems to break the build.\nHere is what I got from my compiler.\n\nbuiltin/gc.c:1588:9: error: explicitly assigning value of variable of type 'int' to itself [-Werror,-Wself-assign]\n for (j = j; j < TASK__COUNT; j++) {\n ~ ^ ~\nbuiltin/gc.c:1535:11: error: variable 'j' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]\n else if (opts->detach > 0) {\n ^~~~~~~~~~~~~~~~\nbuiltin/gc.c:1588:11: note: uninitialized use occurs here\n for (j = j; j < TASK__COUNT; j++) {\n ^\nbuiltin/gc.c:1535:7: note: remove the 'if' if its condition is always true\n else if (opts->detach > 0) {\n ^~~~~~~~~~~~~~~~~~~~~~\nbuiltin/gc.c:1533:6: error: variable 'j' is used uninitialized whenever 'if' condition is true [-Werror,-Wsometimes-uninitialized]\n if (found_selected)\n ^~~~~~~~~~~~~~\nbuiltin/gc.c:1588:11: note: uninitialized use occurs here\n for (j = j; j < TASK__COUNT; j++) {\n ^\nbuiltin/gc.c:1533:2: note: remove the 'if' if its condition is always false\n if (found_selected)\n ^~~~~~~~~~~~~~~~~~~\nbuiltin/gc.c:1508:10: note: initialize the variable 'j' to silence this warning\n int i, j, found_selected = 0;\n ^\n = 0\n\n"},{"id":"506979","messageId":"xmqqv7wub08c.fsf@gitster.g","threadId":"62470","inReplyTo":"20241108173112.1240584-2-calvinwan@google.com","subject":"Re: [RFC PATCH 1/1] maintenance: separate parallelism safe and unsafe tasks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-11T09:14:59Z","receivedAt":"2024-11-11T09:15:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Calvin Wan writes:\n\n> Since the gc task is partially safe, there are two new tasks -- an async\n> safe gc task and an async unsafe gc task. In order to properly invoke\n> this in gc, `--run-async-safe` and `--run-async-unsafe` have been added\n> as options to gc. Maintenance will only run these two new tasks if it\n> was set to detach, otherwise the original gc task runs.\n\nWould it essentially boil down to ensure that only one \"maintenance\"\nis running at a time, and when it is running the \"unsafe\" part,\nsomehow the end-user MUST be made aware of that fact and told to\nrefrain from touching the repository?\n\n> Additionally, if a user passes in tasks thru `--task`, we do not attempt\n> to run separate async/sync tasks since the user sets the order of tasks.\n\nIn other words, the rope is long enough that the user can do\nwhatever they want, regardless of what we think the order should be.\n\nWhich probably makes sense.\n\n> diff --git a/builtin/gc.c b/builtin/gc.c\n> index d52735354c..375d304c42 100644\n> --- a/builtin/gc.c\n> +++ b/builtin/gc.c\n> @@ -668,6 +668,8 @@ struct repository *repo UNUSED)\n> \tpid_t pid;\n> \tint daemonized = 0;\n> \tint keep_largest_pack = -1;\n> +\tint run_async_safe = 0;\n> +\tint run_async_unsafe = 0;\n> \ttimestamp_t dummy;\n> \tstruct child_process rerere_cmd = CHILD_PROCESS_INIT;\n> \tstruct maintenance_run_opts opts = MAINTENANCE_RUN_OPTS_INIT;\n> @@ -694,6 +696,10 @@ struct repository *repo UNUSED)\n> \t\t\t PARSE_OPT_NOCOMPLETE),\n> \t\tOPT_BOOL(0, \"keep-largest-pack\", &keep_largest_pack,\n> \t\t\t N_(\"repack all other packs except the largest pack\")),\n> +\t\tOPT_BOOL(0, \"run-async-safe\", &run_async_safe,\n> +\t\t\t N_(\"run only background safe gc tasks, should only be invoked thru maintenance\")),\n> +\t\tOPT_BOOL(0, \"run-async-unsafe\", &run_async_unsafe,\n> +\t\t\t N_(\"run only background unsafe gc tasks, should only be invoked thru maintenance\")),\n> \t\tOPT_END()\n> \t};\n> \n> @@ -718,6 +724,9 @@ struct repository *repo UNUSED)\n> \t\t\t builtin_gc_usage, 0);\n> \tif (argc > 0)\n> \t\tusage_with_options(builtin_gc_usage, builtin_gc_options);\n> +\t\n> +\tif (run_async_safe && run_async_unsafe)\n> +\t\tdie(_(\"--run-async-safe cannot be used with --run-async-unsafe\"));\n\nSo if the caller wants to eventually run both, it has to spawn this\nprogram twice, the first time with one option and the second time\nwith the other option? Somehow it feels a bit unsatisfying. If the\ncaller says \"I want to run both classes\", and if your safe/unsafe\nclassification system knows which task belongs to which class and\nthe classification system that unsafe ones should be run before safe\nones (or whatever), wouldn't it be easier to use for the caller to\nbe able to say \"run both\", and let your classification system take\ncare of the ordering?\n"},{"id":"507065","messageId":"CAFySSZCzxfqpMWH5ORv8fYb7f5WU3Fc2N99fW33wD9JOcYVrVA@mail.gmail.com","threadId":"62470","inReplyTo":"ZzGtD4Jz9Wj6n0zH@pks.im","subject":"Re: [RFC PATCH 1/1] maintenance: separate parallelism safe and unsafe tasks","fromName":"Calvin Wan","fromEmail":"calvinwan@google.com","sentAt":"2024-11-11T18:06:10Z","receivedAt":"2024-11-11T18:06:23Z","isPatch":true,"sender":{"key":"calvinwan@google.com","avatar":"https://avatars.githubusercontent.com/u/92547554?v=4"},"body":"On Sun, Nov 10, 2024 at 11:07 PM Patrick Steinhardt wrote:\n>\n> On Fri, Nov 08, 2024 at 05:31:12PM +0000, Calvin Wan wrote:\n> > Certain maintenance tasks and subtasks within gc are unsafe to run in\n> > parallel with other commands because they lock up files such as\n> > HEAD.\n>\n> I don't think it is fair to classify this as \"unsafe\". Nothing is unsafe\n> here: we take locks to guard us against concurrent modifications.\n> What you're having problems with is the fact that this safety mechanism\n> works as expected and keeps other processes from modifying locked the\n> data.\n>\n> > Therefore, tasks are marked whether they are async safe or\n> > not. Async unsafe tasks are run first in the same process before running\n> > async safe tasks in parallel.\n> >\n> > Since the gc task is partially safe, there are two new tasks -- an async\n> > safe gc task and an async unsafe gc task. In order to properly invoke\n> > this in gc, `--run-async-safe` and `--run-async-unsafe` have been added\n> > as options to gc. Maintenance will only run these two new tasks if it\n> > was set to detach, otherwise the original gc task runs.\n> >\n> > Additionally, if a user passes in tasks thru `--task`, we do not attempt\n> > to run separate async/sync tasks since the user sets the order of tasks.\n> >\n> > WIP: automatically run gc unsafe tasks when gc is invoked but not from\n> > maintenance\n> > WIP: edit test in t7900-maintainance.sh to match new functionality\n> > WIP: add additional documentation for new options and functionality\n> >\n> > Signed-off-by: Calvin Wan \n> > ---\n> > builtin/gc.c | 173 ++++++++++++++++++++++++++++++++++++-----\n> > t/t7900-maintenance.sh | 24 +++---\n> > 2 files changed, 168 insertions(+), 29 deletions(-)\n> >\n> > diff --git a/builtin/gc.c b/builtin/gc.c\n> > index d52735354c..375d304c42 100644\n> > --- a/builtin/gc.c\n> > +++ b/builtin/gc.c\n>\n> It might make sense to split out the git-gc(1) changes into a\n> preparatory commit with its own set of tests.\n>\n> > @@ -815,7 +824,12 @@ struct repository *repo UNUSED)\n> > atexit(process_log_file_at_exit);\n> > }\n> >\n> > - gc_before_repack(&opts, &cfg);\n> > + if (run_async_unsafe) {\n> > + gc_before_repack(&opts, &cfg);\n> > + goto out;\n> > + } else if (!run_async_safe)\n> > + gc_before_repack(&opts, &cfg);\n> > +\n> >\n>\n> Style: there should be curly braces around the `else if` here.\n>\n> > if (!repository_format_precious_objects) {\n> > struct child_process repack_cmd = CHILD_PROCESS_INIT;\n> > @@ -1052,6 +1066,46 @@ static int maintenance_task_prefetch(struct maintenance_run_opts *opts,\n> > return 0;\n> > }\n> >\n> > +static int maintenance_task_unsafe_gc(struct maintenance_run_opts *opts,\n> > + struct gc_config *cfg UNUSED)\n> > +{\n> > + struct child_process child = CHILD_PROCESS_INIT;\n> > +\n> > + child.git_cmd = child.close_object_store = 1;\n> > + strvec_push(&child.args, \"gc\");\n> > +\n> > + if (opts->auto_flag)\n> > + strvec_push(&child.args, \"--auto\");\n> > + if (opts->quiet)\n> > + strvec_push(&child.args, \"--quiet\");\n> > + else\n> > + strvec_push(&child.args, \"--no-quiet\");\n> > + strvec_push(&child.args, \"--no-detach\");\n> > + strvec_push(&child.args, \"--run-async-unsafe\");\n> > +\n> > + return run_command(&child);\n> > +}\n> > +\n> > +static int maintenance_task_safe_gc(struct maintenance_run_opts *opts,\n> > + struct gc_config *cfg UNUSED)\n> > +{\n> > + struct child_process child = CHILD_PROCESS_INIT;\n> > +\n> > + child.git_cmd = child.close_object_store = 1;\n> > + strvec_push(&child.args, \"gc\");\n> > +\n> > + if (opts->auto_flag)\n> > + strvec_push(&child.args, \"--auto\");\n> > + if (opts->quiet)\n> > + strvec_push(&child.args, \"--quiet\");\n> > + else\n> > + strvec_push(&child.args, \"--no-quiet\");\n> > + strvec_push(&child.args, \"--no-detach\");\n> > + strvec_push(&child.args, \"--run-async-safe\");\n> > +\n> > + return run_command(&child);\n> > +}\n>\n> These two functions and `maintenance_task_gc()` all look exactly the\n> same. We should deduplicate them.\n>\n> > static int maintenance_task_gc(struct maintenance_run_opts *opts,\n> > struct gc_config *cfg UNUSED)\n> > {\n> > @@ -1350,6 +1404,7 @@ struct maintenance_task {\n> > const char *name;\n> > maintenance_task_fn *fn;\n> > maintenance_auto_fn *auto_condition;\n> > + unsigned daemonize_safe;\n>\n> We can use the enum here to give readers a better hint what this\n> variable is about.\n>\n> > unsigned enabled:1;\n> >\n> > enum schedule_priority schedule;\n> > @@ -1362,6 +1417,8 @@ enum maintenance_task_label {\n> > TASK_PREFETCH,\n> > TASK_LOOSE_OBJECTS,\n> > TASK_INCREMENTAL_REPACK,\n> > + TASK_UNSAFE_GC,\n> > + TASK_SAFE_GC,\n> > TASK_GC,\n> > TASK_COMMIT_GRAPH,\n> > TASK_PACK_REFS,\n> > @@ -1370,36 +1427,62 @@ enum maintenance_task_label {\n> > TASK__COUNT\n> > };\n> >\n> > +enum maintenance_task_daemonize_safe {\n> > + UNSAFE,\n> > + SAFE,\n> > +};\n>\n> These names can conflict quite fast. Do we maybe want to rename them to\n> e.g. `MAINTENANCE_TASK_DAEMONIZE_(SAFE|UNSAFE)`?\n>\n> > static struct maintenance_task tasks[] = {\n> > [TASK_PREFETCH] = {\n> > \"prefetch\",\n> > maintenance_task_prefetch,\n> > + NULL,\n> > + SAFE,\n> > },\n>\n> It might make sense to prepare these to take designated field\n> initializers in a preparatory commit.\n\nThanks for all the stylistic feedback. I agree much of this can be\ncleaned up to be simpler, but I sent this as an RFC to gather feedback\non whether this patch directionally made sense. Will clean everything\nup in the v1.\n\n>\n> > [TASK_LOOSE_OBJECTS] = {\n> > \"loose-objects\",\n> > maintenance_task_loose_objects,\n> > loose_object_auto_condition,\n> > + SAFE,\n> > },\n> > [TASK_INCREMENTAL_REPACK] = {\n> > \"incremental-repack\",\n> > maintenance_task_incremental_repack,\n> > incremental_repack_auto_condition,\n> > + SAFE,\n> > + },\n> > + [TASK_UNSAFE_GC] = {\n> > + \"unsafe-gc\",\n> > + maintenance_task_unsafe_gc,\n> > + need_to_gc,\n> > + UNSAFE,\n> > + 0,\n> > + },\n> > + [TASK_SAFE_GC] = {\n> > + \"safe-gc\",\n> > + maintenance_task_safe_gc,\n> > + need_to_gc,\n> > + SAFE,\n> > + 0,\n> > },\n>\n> Hm. I wonder whether we really want to expose additional tasks to\n> address the issue, which feels like we're leaking implementation details\n> to our users. Would it maybe be preferable to instead introduce a new\n> optional callback function for every task that handles the pre-detach\n> logic?\n\nThis does sound like a good idea. However, would there be any issue\nwith running all pre-detach logic before running post-detach logic?\nI'm thinking if pre-detach logic from a different function could\naffect post-detach logic from another. If not, I do agree this would\nbe the best solution going forward.\n\n> I wonder whether we also have to adapt the \"pack-refs\" task to be\n> synchronous instead of asynchronous?\n"},{"id":"507066","messageId":"CAFySSZC_rqGSXLZBQ78zTQ4Mt0+8Cs0OwmfwzKNvyiKoOQRX+g@mail.gmail.com","threadId":"62470","inReplyTo":"xmqqv7wub08c.fsf@gitster.g","subject":"Re: [RFC PATCH 1/1] maintenance: separate parallelism safe and unsafe tasks","fromName":"Calvin Wan","fromEmail":"calvinwan@google.com","sentAt":"2024-11-11T18:12:58Z","receivedAt":"2024-11-11T18:13:11Z","isPatch":true,"sender":{"key":"calvinwan@google.com","avatar":"https://avatars.githubusercontent.com/u/92547554?v=4"},"body":"On Mon, Nov 11, 2024 at 1:15 AM Junio C Hamano wrote:\n>\n> So if the caller wants to eventually run both, it has to spawn this\n> program twice, the first time with one option and the second time\n> with the other option? Somehow it feels a bit unsatisfying. If the\n> caller says \"I want to run both classes\", and if your safe/unsafe\n> classification system knows which task belongs to which class and\n> the classification system that unsafe ones should be run before safe\n> ones (or whatever), wouldn't it be easier to use for the caller to\n> be able to say \"run both\", and let your classification system take\n> care of the ordering?\n\nYes this felt unsatisfactory to me as well for now. The issue is that,\nwhen invoked from maintenance, whether gc is detached or not is\ndependent on maintenance being detached or not. That is why in the\ncommit message of the first patch, I left a \"WIP: automatically run gc\nunsafe tasks when gc is invoked but not from maintenance\". When I turn\nthis into v1, however, I do intend on making this satisfactory.\n"},{"id":"507070","messageId":"CAFySSZCirPpRgJC_zkfNoZZB105V=tu2R4FG43aqND=oDu2xtQ@mail.gmail.com","threadId":"62470","inReplyTo":"xmqqzfm6fk6z.fsf@gitster.g","subject":"Re: [RFC PATCH 0/1] maintenance: separate parallelism safe and unsafe tasks","fromName":"Calvin Wan","fromEmail":"calvinwan@google.com","sentAt":"2024-11-11T18:39:53Z","receivedAt":"2024-11-11T18:40:06Z","isPatch":true,"sender":{"key":"calvinwan@google.com","avatar":"https://avatars.githubusercontent.com/u/92547554?v=4"},"body":"On Sun, Nov 10, 2024 at 8:50 PM Junio C Hamano wrote:\n>\n> Calvin Wan writes:\n>\n> > However, this is not the case as discovered earlier[1] -- certain\n> > maintenance and gc tasks are not safe to run in parallel with other\n> > commands. The consequences of such are that scripts with commands that\n> > trigger maintenance/gc can race and crash.\n> >\n> > Users can also run into\n> > unexpected errors from porcelain commands that touch common files such\n> > as HEAD.lock, unaware that a background maintenance/gc task is the one\n> > holding the lock.\n>\n> The symptom looks more like a controlled refusal of execution than a\n> crash.\n\nCorrect, I also do think some responsibility should be on the scripts\nto be able to both handle the \"controlled refusal of execution\" as\nwell as checking to see if those lock files exist first to avoid such\nerrors.\n"},{"id":"507107","messageId":"ZzL1jy3plVeld_3m@pks.im","threadId":"62470","inReplyTo":"CAFySSZCzxfqpMWH5ORv8fYb7f5WU3Fc2N99fW33wD9JOcYVrVA@mail.gmail.com","subject":"Re: [RFC PATCH 1/1] maintenance: separate parallelism safe and unsafe tasks","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-11-12T06:28:59Z","receivedAt":"2024-11-12T06:29:12Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Nov 11, 2024 at 10:06:10AM -0800, Calvin Wan wrote:\n> On Sun, Nov 10, 2024 at 11:07 PM Patrick Steinhardt wrote:\n> > > [TASK_LOOSE_OBJECTS] = {\n> > > \"loose-objects\",\n> > > maintenance_task_loose_objects,\n> > > loose_object_auto_condition,\n> > > + SAFE,\n> > > },\n> > > [TASK_INCREMENTAL_REPACK] = {\n> > > \"incremental-repack\",\n> > > maintenance_task_incremental_repack,\n> > > incremental_repack_auto_condition,\n> > > + SAFE,\n> > > + },\n> > > + [TASK_UNSAFE_GC] = {\n> > > + \"unsafe-gc\",\n> > > + maintenance_task_unsafe_gc,\n> > > + need_to_gc,\n> > > + UNSAFE,\n> > > + 0,\n> > > + },\n> > > + [TASK_SAFE_GC] = {\n> > > + \"safe-gc\",\n> > > + maintenance_task_safe_gc,\n> > > + need_to_gc,\n> > > + SAFE,\n> > > + 0,\n> > > },\n> >\n> > Hm. I wonder whether we really want to expose additional tasks to\n> > address the issue, which feels like we're leaking implementation details\n> > to our users. Would it maybe be preferable to instead introduce a new\n> > optional callback function for every task that handles the pre-detach\n> > logic?\n> \n> This does sound like a good idea. However, would there be any issue\n> with running all pre-detach logic before running post-detach logic?\n> I'm thinking if pre-detach logic from a different function could\n> affect post-detach logic from another. If not, I do agree this would\n> be the best solution going forward.\n\nSure, in theory these can interact with each other. But is that any\ndifferent when you represent this with tasks instead? The conflict would\nstill exist there. It's also not any different to how things work right\nnow: the \"gc\" task will impact the \"repack\" task, so configuring them\nboth at the same time does not really make much sense.\n\nPatrick\n"},{"id":"507376","messageId":"CAFySSZBioOrfk5O7oni3LRLWasFo6DsuyW7icDDVkiUxq4fNOQ@mail.gmail.com","threadId":"62470","inReplyTo":"ZzL1jy3plVeld_3m@pks.im","subject":"Re: [RFC PATCH 1/1] maintenance: separate parallelism safe and unsafe tasks","fromName":"Calvin Wan","fromEmail":"calvinwan@google.com","sentAt":"2024-11-15T20:13:24Z","receivedAt":"2024-11-15T20:13:37Z","isPatch":true,"sender":{"key":"calvinwan@google.com","avatar":"https://avatars.githubusercontent.com/u/92547554?v=4"},"body":"On Mon, Nov 11, 2024 at 10:29 PM Patrick Steinhardt wrote:\n>\n> On Mon, Nov 11, 2024 at 10:06:10AM -0800, Calvin Wan wrote:\n> > On Sun, Nov 10, 2024 at 11:07 PM Patrick Steinhardt wrote:\n> > >\n> > > Hm. I wonder whether we really want to expose additional tasks to\n> > > address the issue, which feels like we're leaking implementation details\n> > > to our users. Would it maybe be preferable to instead introduce a new\n> > > optional callback function for every task that handles the pre-detach\n> > > logic?\n> >\n> > This does sound like a good idea. However, would there be any issue\n> > with running all pre-detach logic before running post-detach logic?\n> > I'm thinking if pre-detach logic from a different function could\n> > affect post-detach logic from another. If not, I do agree this would\n> > be the best solution going forward.\n>\n> Sure, in theory these can interact with each other. But is that any\n> different when you represent this with tasks instead? The conflict would\n> still exist there. It's also not any different to how things work right\n> now: the \"gc\" task will impact the \"repack\" task, so configuring them\n> both at the same time does not really make much sense.\n\nNo you are correct that this is no different than how these tasks are\ncurrently run. However, I have just received some numbers that the\nrepack, when gc'ing in Android, is the longest operation so even if we\nwere able to run repack first in the foreground, ultimately it\nwouldn't save a significant amount of time compared to running gc\nentirely in the foreground. I think for now it makes sense to hold off\non rerolling this series (at least in the form of auto\nbackgrounding/foregrounding tasks) since the purported benefits\ncurrently aren't worth the churn. Thanks again for the comments on\nthis series\n"},{"id":"507456","messageId":"xmqqr079jphr.fsf@gitster.g","threadId":"62470","inReplyTo":"CAFySSZBioOrfk5O7oni3LRLWasFo6DsuyW7icDDVkiUxq4fNOQ@mail.gmail.com","subject":"Re: [RFC PATCH 1/1] maintenance: separate parallelism safe and unsafe tasks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-18T01:32:32Z","receivedAt":"2024-11-18T01:32:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Calvin Wan writes:\n\n> .... I think for now it makes sense to hold off\n> on rerolling this series (at least in the form of auto\n> backgrounding/foregrounding tasks) since the purported benefits\n> currently aren't worth the churn. Thanks again for the comments on\n> this series\n\nSo, we'll place this topic on hold, or even eject from the tree to\nbe revisited later?\n\nThanks.\n"},{"id":"507459","messageId":"Zzrlou9WbylWD1R9@pks.im","threadId":"62470","inReplyTo":"CAFySSZBioOrfk5O7oni3LRLWasFo6DsuyW7icDDVkiUxq4fNOQ@mail.gmail.com","subject":"Re: [RFC PATCH 1/1] maintenance: separate parallelism safe and unsafe tasks","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-11-18T06:58:48Z","receivedAt":"2024-11-18T06:59:01Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Nov 15, 2024 at 12:13:24PM -0800, Calvin Wan wrote:\n> On Mon, Nov 11, 2024 at 10:29 PM Patrick Steinhardt wrote:\n> >\n> > On Mon, Nov 11, 2024 at 10:06:10AM -0800, Calvin Wan wrote:\n> > > On Sun, Nov 10, 2024 at 11:07 PM Patrick Steinhardt wrote:\n> > > >\n> > > > Hm. I wonder whether we really want to expose additional tasks to\n> > > > address the issue, which feels like we're leaking implementation details\n> > > > to our users. Would it maybe be preferable to instead introduce a new\n> > > > optional callback function for every task that handles the pre-detach\n> > > > logic?\n> > >\n> > > This does sound like a good idea. However, would there be any issue\n> > > with running all pre-detach logic before running post-detach logic?\n> > > I'm thinking if pre-detach logic from a different function could\n> > > affect post-detach logic from another. If not, I do agree this would\n> > > be the best solution going forward.\n> >\n> > Sure, in theory these can interact with each other. But is that any\n> > different when you represent this with tasks instead? The conflict would\n> > still exist there. It's also not any different to how things work right\n> > now: the \"gc\" task will impact the \"repack\" task, so configuring them\n> > both at the same time does not really make much sense.\n> \n> No you are correct that this is no different than how these tasks are\n> currently run. However, I have just received some numbers that the\n> repack, when gc'ing in Android, is the longest operation so even if we\n> were able to run repack first in the foreground, ultimately it\n> wouldn't save a significant amount of time compared to running gc\n> entirely in the foreground. I think for now it makes sense to hold off\n> on rerolling this series (at least in the form of auto\n> backgrounding/foregrounding tasks) since the purported benefits\n> currently aren't worth the churn. Thanks again for the comments on\n> this series\n\nWait, I think I'm missing something. Why should the repack run in the\nforeground? Based on your report the only thing that'd have to run in\nthe foreground are tasks that lock refs, so `git pack-refs` and `git\nreflog expire`.\n\nPatrick\n"}]}