From 7eca84e6011f8b3c3068d7dd99a21f2e4ca04445 Mon Sep 17 00:00:00 2001 From: Jeff Hostetler Date: Fri, 9 Apr 2021 16:43:24 -0400 Subject: [PATCH 01/34] odb--daemon: stub in new ODB over IPC daemon Signed-off-by: Jeff Hostetler --- .gitignore | 1 + Makefile | 1 + builtin.h | 1 + builtin/odb--daemon.c | 37 +++++++++++++++++++++++++++++++++++++ git.c | 1 + 5 files changed, 41 insertions(+) create mode 100644 builtin/odb--daemon.c diff --git a/.gitignore b/.gitignore index beccf34abe9ee0..3e2e14a510175e 100644 --- a/.gitignore +++ b/.gitignore @@ -112,6 +112,7 @@ /git-mv /git-name-rev /git-notes +/git-odb--daemon /git-p4 /git-pack-redundant /git-pack-objects diff --git a/Makefile b/Makefile index 8cf06d8b367500..17832cfbf0703f 100644 --- a/Makefile +++ b/Makefile @@ -1119,6 +1119,7 @@ BUILTIN_OBJS += builtin/multi-pack-index.o BUILTIN_OBJS += builtin/mv.o BUILTIN_OBJS += builtin/name-rev.o BUILTIN_OBJS += builtin/notes.o +BUILTIN_OBJS += builtin/odb--daemon.o BUILTIN_OBJS += builtin/pack-objects.o BUILTIN_OBJS += builtin/pack-redundant.o BUILTIN_OBJS += builtin/pack-refs.o diff --git a/builtin.h b/builtin.h index 7554476f90a4bc..0e3080a3abcc5a 100644 --- a/builtin.h +++ b/builtin.h @@ -188,6 +188,7 @@ int cmd_multi_pack_index(int argc, const char **argv, const char *prefix); int cmd_mv(int argc, const char **argv, const char *prefix); int cmd_name_rev(int argc, const char **argv, const char *prefix); int cmd_notes(int argc, const char **argv, const char *prefix); +int cmd_odb__daemon(int argc, const char **argv, const char *prefix); int cmd_pack_objects(int argc, const char **argv, const char *prefix); int cmd_pack_redundant(int argc, const char **argv, const char *prefix); int cmd_patch_id(int argc, const char **argv, const char *prefix); diff --git a/builtin/odb--daemon.c b/builtin/odb--daemon.c new file mode 100644 index 00000000000000..92827a298adeb3 --- /dev/null +++ b/builtin/odb--daemon.c @@ -0,0 +1,37 @@ +#include "builtin.h" +#include "config.h" +#include "parse-options.h" +#include "simple-ipc.h" + +static const char * const my_usage[] = { + "git odb--daemon TBD...", + NULL +}; + +static struct option my_options[] = { + OPT_END() +}; + +#ifndef SUPPORTS_SIMPLE_IPC +int cmd_odb__daemon(int argc, const char **argv, const char *prefix) +{ + if (argc == 2 && !strcmp(argv[1], "-h")) + usage_with_options(my_usage, my_options); + + die(_("odb--daemon not supported on this platform")); +} +#else + + +int cmd_odb__daemon(int argc, const char **argv, const char *prefix) +{ + if (argc == 2 && !strcmp(argv[1], "-h")) + usage_with_options(my_usage, my_options); + + git_config(git_default_config, NULL); + + argc = parse_options(argc, argv, prefix, my_options, my_usage, 0); + + die(_("Unhandled command mode")); +} +#endif /* SUPPORTS_SIMPLE_IPC */ diff --git a/git.c b/git.c index 239deb9823fcd8..7c6356e7d5d8c3 100644 --- a/git.c +++ b/git.c @@ -556,6 +556,7 @@ static struct cmd_struct commands[] = { { "mv", cmd_mv, RUN_SETUP | NEED_WORK_TREE }, { "name-rev", cmd_name_rev, RUN_SETUP }, { "notes", cmd_notes, RUN_SETUP }, + { "odb--daemon", cmd_odb__daemon, RUN_SETUP }, { "pack-objects", cmd_pack_objects, RUN_SETUP }, { "pack-redundant", cmd_pack_redundant, RUN_SETUP | NO_PARSEOPT }, { "pack-refs", cmd_pack_refs, RUN_SETUP }, From b8aec5f4f5adc6f29ff81a16511816be3e238813 Mon Sep 17 00:00:00 2001 From: Jeff Hostetler Date: Fri, 9 Apr 2021 16:57:41 -0400 Subject: [PATCH 02/34] odb--daemon: stub in --run --ipc-threads Signed-off-by: Jeff Hostetler --- builtin/odb--daemon.c | 35 ++++++++++++++++++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-) diff --git a/builtin/odb--daemon.c b/builtin/odb--daemon.c index 92827a298adeb3..537cf4c244f5be 100644 --- a/builtin/odb--daemon.c +++ b/builtin/odb--daemon.c @@ -2,13 +2,33 @@ #include "config.h" #include "parse-options.h" #include "simple-ipc.h" +#include "thread-utils.h" + +enum my_mode { + MODE_UNDEFINED = 0, + MODE_RUN, +}; static const char * const my_usage[] = { + "git odb--daemon --run []", "git odb--daemon TBD...", NULL }; +struct my_args +{ + enum my_mode mode; + int nr_ipc_threads; +}; + +static struct my_args my_args; + static struct option my_options[] = { + OPT_CMDMODE('r', "run", &my_args.mode, + N_("run the ODB daemon in the foreground"), MODE_RUN), + + OPT_GROUP(N_("Daemon options")), + OPT_INTEGER(0, "ipc-threads", &my_args.nr_ipc_threads, N_("use ipc threads")), OPT_END() }; @@ -22,6 +42,10 @@ int cmd_odb__daemon(int argc, const char **argv, const char *prefix) } #else +int try_run_daemon(void) +{ + return 0; +} int cmd_odb__daemon(int argc, const char **argv, const char *prefix) { @@ -32,6 +56,15 @@ int cmd_odb__daemon(int argc, const char **argv, const char *prefix) argc = parse_options(argc, argv, prefix, my_options, my_usage, 0); - die(_("Unhandled command mode")); + if (my_args.nr_ipc_threads < 1) + my_args.nr_ipc_threads = online_cpus(); + + switch (my_args.mode) { + case MODE_RUN: + return !!try_run_daemon(); + + default: + die(_("Unhandled command mode")); + } } #endif /* SUPPORTS_SIMPLE_IPC */ From 6a809203bea1757d02db4a4932277e92eea0f1f4 Mon Sep 17 00:00:00 2001 From: Jeff Hostetler Date: Fri, 9 Apr 2021 17:32:46 -0400 Subject: [PATCH 03/34] odb-over-ipc: stub in client routines Signed-off-by: Jeff Hostetler --- odb-over-ipc.c | 34 ++++++++++++++++++++++++++++++++++ odb-over-ipc.h | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 84 insertions(+) create mode 100644 odb-over-ipc.c create mode 100644 odb-over-ipc.h diff --git a/odb-over-ipc.c b/odb-over-ipc.c new file mode 100644 index 00000000000000..5196e6fa0fcd50 --- /dev/null +++ b/odb-over-ipc.c @@ -0,0 +1,34 @@ +#include "cache.h" +#include "simple-ipc.h" +#include "odb-over-ipc.h" + +int odb_over_ipc__is_supported(void) +{ +#ifdef SUPPORTS_SIMPLE_IPC + return 1; +#else + return 0; +#endif +} + +#ifdef SUPPORTS_SIMPLE_IPC +/* + * We claim "/odb-over-ipc" as the name of the Unix Domain Socket + * that we will use on Unix. And something based on this unique string + * in the Named Pipe File System on Windows. So we don't need a command + * line argument for this. + */ +GIT_PATH_FUNC(odb_over_ipc__get_path, "odb-over-ipc"); + +enum ipc_active_state odb_over_ipc__get_state(void) +{ + return ipc_get_active_state(odb_over_ipc__get_path()); +} + +int odb_over_ipc__command(const char *command, struct strbuf *answer) +{ + // TODO + return -1; +} + +#endif /* SUPPORTS_SIMPLE_IPC */ diff --git a/odb-over-ipc.h b/odb-over-ipc.h new file mode 100644 index 00000000000000..12c7eb7b7f8e98 --- /dev/null +++ b/odb-over-ipc.h @@ -0,0 +1,50 @@ +#ifndef ODB_OVER_IPC_H +#define ODB_OVER_IPC_H + +/* + * Returns true if Simple IPC is supported on this platform. This symbol + * must always be visible and outside of the ifdef. + */ +int odb_over_ipc__is_supported(void); + +#include "simple-ipc.h" + +#ifdef SUPPORTS_SIMPLE_IPC + +/* + * Returns the pathname to the IPC named pipe or Unix domain socket + * where a `git odb--daemon` process will listen. + * + * TODO Technically, this is a per repo value, since it needs to + * TODO look at the ODB (both the `/objects` directory and + * TODO ODB alternates). So we should share a daemon between multiple + * TODO worktrees. Verify this. + */ +const char *odb_over_ipc__get_path(void); + +/* + * Try to determine whether there is a `git odb--daemon` process + * listening on the official IPC named pipe / socket for the + * current repo. + */ +enum ipc_active_state odb_over_ipc__get_state(void); + +/* + * Connect to an existing `git odb--daemon` process, send a command, + * and receive a response. If no daemon is running, this DOES NOT try + * to start one. + * + * Commands include: + * [] ask for an object + * + * TODO If we can trust the code that creates/deletes packfiles, we + * TODO might consider adding a command here to let that process tell + * TODO the daemon to update the list of cached packfiles. + * + * TODO For simplicit during prototyping I am NOT going to + * TODO auto-start one. Revisit this later. + */ +int odb_over_ipc__command(const char *command, struct strbuf *answer); + +#endif /* SUPPORTS_SIMPLE_IPC */ +#endif /* ODB_OVER_IPC_H */ From 1ecf40c382620c528387cfebd44349fb58dd17b9 Mon Sep 17 00:00:00 2001 From: Jeff Hostetler Date: Fri, 9 Apr 2021 18:14:40 -0400 Subject: [PATCH 04/34] odb--daemon: stub in --run framework Signed-off-by: Jeff Hostetler --- Makefile | 1 + builtin/odb--daemon.c | 93 ++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 93 insertions(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 17832cfbf0703f..dfe285c008ffda 100644 --- a/Makefile +++ b/Makefile @@ -936,6 +936,7 @@ LIB_OBJS += notes.o LIB_OBJS += object-file.o LIB_OBJS += object-name.o LIB_OBJS += object.o +LIB_OBJS += odb-over-ipc.o LIB_OBJS += oid-array.o LIB_OBJS += oidmap.o LIB_OBJS += oidset.o diff --git a/builtin/odb--daemon.c b/builtin/odb--daemon.c index 537cf4c244f5be..aa6f6baa1183aa 100644 --- a/builtin/odb--daemon.c +++ b/builtin/odb--daemon.c @@ -3,6 +3,7 @@ #include "parse-options.h" #include "simple-ipc.h" #include "thread-utils.h" +#include "odb-over-ipc.h" enum my_mode { MODE_UNDEFINED = 0, @@ -42,11 +43,101 @@ int cmd_odb__daemon(int argc, const char **argv, const char *prefix) } #else -int try_run_daemon(void) +/* + * Technically, we don't need to probe for an existing daemon process + * running on our named pipe / socket, since we could just try to + * create it and let it fail if the pipe/socket is busy. However, + * probing first allows us to give a better error message (especially + * in the case where we disassociate from the terminal and fork into + * the background). + */ +static int is_daemon_listening(void) { + return odb_over_ipc__get_state() == IPC_STATE__LISTENING; +} + +struct my_odb_ipc_state +{ +}; + +static struct my_odb_ipc_state *my_state; + +static ipc_server_application_cb odb_cb; + +static int odb_cb(void *data, const char *command, + ipc_server_reply_cb *reply_cb, + struct ipc_server_reply_data *reply_data) +{ + struct my_odb_ipc_state *state = data; + + assert(state == my_state); + + // TODO respond to request from client. + return 0; } +/* + * Use the simple SYNC interface, start the IPC thread pool, and + * block the calling thread until it shuts down. + */ +static int do_ipc(void) +{ + int ret; + const char *path = odb_over_ipc__get_path(); + + struct ipc_server_opts ipc_opts = { + .nr_threads = my_args.nr_ipc_threads, + }; + + ret = ipc_server_run(path, &ipc_opts, odb_cb, my_state); + + if (ret == -2) /* maybe we lost a startup race */ + error(_("IPC socket/pipe already in use: '%s'"), path); + + else if (ret == -1) + error(_("could not start IPC thread pool on: '%s'"), path); + + return ret; +} + +/* + * Actually run the daemon. + */ +static int do_run_daemon(void) +{ + int ret; + + // TODO Create mutexes and locks + // + // TODO Load up the packfiles + // + // TODO Consider starting a thread to watch for new/deleted packfiles + // TODO and update the in-memory database. + + ret = do_ipc(); + + // TODO As a precaution, send stop events to our other threads and + // TODO JOIN them. + // + // TODO destroy mutexes and locks. + // + // TODO destroy in-memory databases. + + return ret; +} + +/* + * Try to run the odb--daemon in the foreground of the current process. + */ +static int try_run_daemon(void) +{ + if (is_daemon_listening()) + die("odb--daemon is already running"); + + return do_run_daemon(); +} + int cmd_odb__daemon(int argc, const char **argv, const char *prefix) { if (argc == 2 && !strcmp(argv[1], "-h")) From 8f8749ec65e56051b6753d3765994b2c51487181 Mon Sep 17 00:00:00 2001 From: Jeff Hostetler Date: Mon, 12 Apr 2021 09:22:26 -0400 Subject: [PATCH 05/34] odb--daemon: use async version of IPC Signed-off-by: Jeff Hostetler --- builtin/odb--daemon.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/builtin/odb--daemon.c b/builtin/odb--daemon.c index aa6f6baa1183aa..01d408ce49e78f 100644 --- a/builtin/odb--daemon.c +++ b/builtin/odb--daemon.c @@ -58,9 +58,10 @@ static int is_daemon_listening(void) struct my_odb_ipc_state { + struct ipc_server_data *ipc_state; }; -static struct my_odb_ipc_state *my_state; +static struct my_odb_ipc_state my_state; static ipc_server_application_cb odb_cb; @@ -70,18 +71,14 @@ static int odb_cb(void *data, const char *command, { struct my_odb_ipc_state *state = data; - assert(state == my_state); + assert(state == &my_state); // TODO respond to request from client. return 0; } -/* - * Use the simple SYNC interface, start the IPC thread pool, and - * block the calling thread until it shuts down. - */ -static int do_ipc(void) +static int launch_ipc_thread_pool(void) { int ret; const char *path = odb_over_ipc__get_path(); @@ -90,7 +87,8 @@ static int do_ipc(void) .nr_threads = my_args.nr_ipc_threads, }; - ret = ipc_server_run(path, &ipc_opts, odb_cb, my_state); + ret = ipc_server_run_async(&my_state.ipc_state, + path, &ipc_opts, odb_cb, &my_state); if (ret == -2) /* maybe we lost a startup race */ error(_("IPC socket/pipe already in use: '%s'"), path); @@ -115,7 +113,9 @@ static int do_run_daemon(void) // TODO Consider starting a thread to watch for new/deleted packfiles // TODO and update the in-memory database. - ret = do_ipc(); + ret = launch_ipc_thread_pool(); + if (!ret) + ret = ipc_server_await(my_state.ipc_state); // TODO As a precaution, send stop events to our other threads and // TODO JOIN them. From 680113bdb2fdab1a73d0788ca55f3b913287396b Mon Sep 17 00:00:00 2001 From: Jeff Hostetler Date: Mon, 12 Apr 2021 09:38:50 -0400 Subject: [PATCH 06/34] odb-over-ipc: add --stop command Signed-off-by: Jeff Hostetler --- builtin/odb--daemon.c | 69 +++++++++++++++++++++++++++++++++++++++---- odb-over-ipc.c | 30 +++++++++++++++++-- 2 files changed, 91 insertions(+), 8 deletions(-) diff --git a/builtin/odb--daemon.c b/builtin/odb--daemon.c index 01d408ce49e78f..8a307aea371285 100644 --- a/builtin/odb--daemon.c +++ b/builtin/odb--daemon.c @@ -7,11 +7,13 @@ enum my_mode { MODE_UNDEFINED = 0, - MODE_RUN, + MODE_RUN, /* run daemon in current process */ + MODE_STOP, /* stop an existing daemon */ }; static const char * const my_usage[] = { - "git odb--daemon --run []", + "git odb--daemon --run []", + "git odb--daemon --stop []", "git odb--daemon TBD...", NULL }; @@ -25,8 +27,10 @@ struct my_args static struct my_args my_args; static struct option my_options[] = { - OPT_CMDMODE('r', "run", &my_args.mode, + OPT_CMDMODE(0, "run", &my_args.mode, N_("run the ODB daemon in the foreground"), MODE_RUN), + OPT_CMDMODE(0, "stop", &my_args.mode, + N_("stop an existing ODB daemon"), MODE_STOP), OPT_GROUP(N_("Daemon options")), OPT_INTEGER(0, "ipc-threads", &my_args.nr_ipc_threads, N_("use ipc threads")), @@ -63,9 +67,9 @@ struct my_odb_ipc_state static struct my_odb_ipc_state my_state; -static ipc_server_application_cb odb_cb; +static ipc_server_application_cb odb_ipc_cb; -static int odb_cb(void *data, const char *command, +static int odb_ipc_cb(void *data, const char *command, ipc_server_reply_cb *reply_cb, struct ipc_server_reply_data *reply_data) { @@ -73,6 +77,27 @@ static int odb_cb(void *data, const char *command, assert(state == &my_state); + /* + * We expect `command` to be of the form: + * + * := quit NUL + * | TBC... + */ + + if (!strcmp(command, "quit")) { + /* + * A client has requested (over the pipe/socket) that this + * daemon shutdown. + * + * Tell the IPC thread pool to shutdown (which completes the + * `await` in the main thread and causes us to shutdown). + * + * In this callback we are NOT in control of the life of this + * thread, so we cannot directly shutdown here. + */ + return SIMPLE_IPC_QUIT; + } + // TODO respond to request from client. return 0; @@ -88,7 +113,7 @@ static int launch_ipc_thread_pool(void) }; ret = ipc_server_run_async(&my_state.ipc_state, - path, &ipc_opts, odb_cb, &my_state); + path, &ipc_opts, odb_ipc_cb, &my_state); if (ret == -2) /* maybe we lost a startup race */ error(_("IPC socket/pipe already in use: '%s'"), path); @@ -138,6 +163,35 @@ static int try_run_daemon(void) return do_run_daemon(); } +/* + * Acting as a CLIENT. + * + * Send a "quit" command to an existing `git odb--daemon` process + * (if running) and wait for it to shutdown. + * + * The wait here is important for helping the test suite be stable. + */ +static int client_send_stop(void) +{ + struct strbuf answer = STRBUF_INIT; + int ret; + + ret = odb_over_ipc__command("quit", &answer); + + /* The quit command does not return any response data. */ + strbuf_release(&answer); + + if (ret) + return ret; + + trace2_region_enter("odb_client", "polling-for-daemon-exit", NULL); + while (odb_over_ipc__get_state() == IPC_STATE__LISTENING) + sleep_millisec(50); + trace2_region_leave("odb_client", "polling-for-daemon-exit", NULL); + + return 0; +} + int cmd_odb__daemon(int argc, const char **argv, const char *prefix) { if (argc == 2 && !strcmp(argv[1], "-h")) @@ -154,6 +208,9 @@ int cmd_odb__daemon(int argc, const char **argv, const char *prefix) case MODE_RUN: return !!try_run_daemon(); + case MODE_STOP: + return !!client_send_stop(); + default: die(_("Unhandled command mode")); } diff --git a/odb-over-ipc.c b/odb-over-ipc.c index 5196e6fa0fcd50..6b6bac89150724 100644 --- a/odb-over-ipc.c +++ b/odb-over-ipc.c @@ -27,8 +27,34 @@ enum ipc_active_state odb_over_ipc__get_state(void) int odb_over_ipc__command(const char *command, struct strbuf *answer) { - // TODO - return -1; + struct ipc_client_connection *connection = NULL; + struct ipc_client_connect_options options + = IPC_CLIENT_CONNECT_OPTIONS_INIT; + int ret; + enum ipc_active_state state; + + strbuf_reset(answer); + + options.wait_if_busy = 1; + options.wait_if_not_found = 0; + + state = ipc_client_try_connect(odb_over_ipc__get_path(), &options, + &connection); + if (state != IPC_STATE__LISTENING) { + die("odb--daemon is not running"); + return -1; + } + + ret = ipc_client_send_command_to_connection(connection, command, answer); + ipc_client_close_connection(connection); + + if (ret == -1) { + die("could not send '%s' command to odb--daemon", + command); + return -1; + } + + return 0; } #endif /* SUPPORTS_SIMPLE_IPC */ From 08af616c965cfa519d1c5b606f16a19ee0436a8f Mon Sep 17 00:00:00 2001 From: Jeff Hostetler Date: Mon, 12 Apr 2021 13:55:03 -0400 Subject: [PATCH 07/34] odb-over-ipc: checkpoint. functional daemon side. Signed-off-by: Jeff Hostetler --- builtin/odb--daemon.c | 136 ++++++++++++++++++++++++++++++++++++++---- object-file.c | 10 ++++ odb-over-ipc.c | 49 ++++++++++++++- odb-over-ipc.h | 22 ++++++- 4 files changed, 200 insertions(+), 17 deletions(-) diff --git a/builtin/odb--daemon.c b/builtin/odb--daemon.c index 8a307aea371285..9e87bab1218953 100644 --- a/builtin/odb--daemon.c +++ b/builtin/odb--daemon.c @@ -1,7 +1,9 @@ #include "builtin.h" #include "config.h" +#include "object-store.h" #include "parse-options.h" #include "simple-ipc.h" +#include "strbuf.h" #include "thread-utils.h" #include "odb-over-ipc.h" @@ -67,23 +69,121 @@ struct my_odb_ipc_state static struct my_odb_ipc_state my_state; +static int odb_ipc_cb__get_oid(struct my_odb_ipc_state *state, + const char *command, + ipc_server_reply_cb *reply_cb, + struct ipc_server_reply_data *reply_data) +{ + struct strbuf **lines = strbuf_split_str(command, '\n', 0); + struct strbuf response = STRBUF_INIT; + struct object_id oid; + const char *sz; + uintmax_t umax_flags = 0; + int k; + + oidclr(&oid); + + for (k = 0; lines[k]; k++) { + strbuf_trim_trailing_newline(lines[k]); + + if (skip_prefix(lines[k]->buf, "oid ", &sz)) { + if (get_oid_hex(sz, &oid)) + goto fail; + continue; + } + + if (skip_prefix(lines[k]->buf, "flags ", &sz)) { + umax_flags = strtoumax(sz, NULL, 10); + continue; + } + + BUG("unexpected line '%s' in OID request", lines[k]->buf); + } + + // TODO Insert ODB lookup from in-memory database here. + // TODO For now, just do the regular lookup. + + { + enum object_type var_type = OBJ_BAD; + unsigned long var_size = 0; + off_t var_disk_size = 0; + struct object_id var_delta_base_oid; + struct strbuf var_type_name = STRBUF_INIT; + void *var_content = NULL; + struct object_info var_oi = OBJECT_INFO_INIT; + int ret; + + oidclr(&var_delta_base_oid); + + var_oi.typep = &var_type; + var_oi.sizep = &var_size; + var_oi.disk_sizep = &var_disk_size; + var_oi.delta_base_oid = &var_delta_base_oid; + var_oi.type_name = &var_type_name; + var_oi.contentp = &var_content; + + ret = oid_object_info_extended(the_repository, &oid, &var_oi, + (unsigned)umax_flags); + if (ret) + goto fail; + + strbuf_reset(&response); + strbuf_addf(&response, "oid %s\n", oid_to_hex(&oid)); + strbuf_addf(&response, "type %d\n", var_type); + strbuf_addf(&response, "size %"PRIuMAX"\n", (uintmax_t)var_size); + strbuf_addf(&response, "disk %"PRIuMAX"\n", (uintmax_t)var_disk_size); + // TODO only include delta base oid if non zero. + strbuf_addf(&response, "delta %s\n", oid_to_hex(&var_delta_base_oid)); + strbuf_addf(&response, "name %s\n", var_type_name.buf); + + // TODO we do not care about oi.whence nor oi.u.packed + + strbuf_addstr(&response, "content\n"); /* must be last */ + + reply_cb(reply_data, response.buf, response.len); + reply_cb(reply_data, var_content, var_size); + + strbuf_release(&var_type_name); + free(var_content); + } + + goto done; + +fail: + /* + * Send the client an error response to force it to do + * the work itself. + */ + strbuf_reset(&response); + strbuf_addstr(&response, "error"); + reply_cb(reply_data, response.buf, response.len + 1); + goto done; + +done: + strbuf_list_free(lines); + strbuf_release(&response); + return 0; +} + +/* + * This callback handles IPC requests from clients. We run on an + * arbitrary thread. + * + * We expect `command` to be of the form: + * + * := quit NUL + * | TBC... + */ static ipc_server_application_cb odb_ipc_cb; static int odb_ipc_cb(void *data, const char *command, - ipc_server_reply_cb *reply_cb, - struct ipc_server_reply_data *reply_data) + ipc_server_reply_cb *reply_cb, + struct ipc_server_reply_data *reply_data) { struct my_odb_ipc_state *state = data; assert(state == &my_state); - /* - * We expect `command` to be of the form: - * - * := quit NUL - * | TBC... - */ - if (!strcmp(command, "quit")) { /* * A client has requested (over the pipe/socket) that this @@ -98,7 +198,17 @@ static int odb_ipc_cb(void *data, const char *command, return SIMPLE_IPC_QUIT; } - // TODO respond to request from client. + if (!strncmp(command, "oid", 3)) { + /* + * A client has requested that we lookup an object from the + * ODB and send it to them. + */ + return odb_ipc_cb__get_oid(state, command, reply_cb, reply_data); + } + + // TODO respond to other requests from client. + // + // TODO decide how to return an error for unknown commands. return 0; } @@ -131,6 +241,8 @@ static int do_run_daemon(void) { int ret; + odb_over_ipc__set_is_daemon(); + // TODO Create mutexes and locks // // TODO Load up the packfiles @@ -181,8 +293,10 @@ static int client_send_stop(void) /* The quit command does not return any response data. */ strbuf_release(&answer); - if (ret) + if (ret) { + die("could not send stop command to odb--daemon"); return ret; + } trace2_region_enter("odb_client", "polling-for-daemon-exit", NULL); while (odb_over_ipc__get_state() == IPC_STATE__LISTENING) diff --git a/object-file.c b/object-file.c index 624af408cdcd2a..1140111013d211 100644 --- a/object-file.c +++ b/object-file.c @@ -32,6 +32,7 @@ #include "packfile.h" #include "object-store.h" #include "promisor-remote.h" +#include "odb-over-ipc.h" /* The maximum size for an object header. */ #define MAX_HEADER_LEN 32 @@ -1566,9 +1567,18 @@ int oid_object_info_extended(struct repository *r, const struct object_id *oid, struct object_info *oi, unsigned flags) { int ret; + + if (!odb_over_ipc__get_oid(r, oid, oi, flags)) + return 0; + + trace2_region_enter("oid", "object", r); + obj_read_lock(); ret = do_oid_object_info_extended(r, oid, oi, flags); obj_read_unlock(); + + trace2_region_leave("oid", "object", r); + return ret; } diff --git a/odb-over-ipc.c b/odb-over-ipc.c index 6b6bac89150724..0972e9b571d0bd 100644 --- a/odb-over-ipc.c +++ b/odb-over-ipc.c @@ -20,6 +20,13 @@ int odb_over_ipc__is_supported(void) */ GIT_PATH_FUNC(odb_over_ipc__get_path, "odb-over-ipc"); +static int is_daemon = 0; + +void odb_over_ipc__set_is_daemon(void) +{ + is_daemon = 1; +} + enum ipc_active_state odb_over_ipc__get_state(void) { return ipc_get_active_state(odb_over_ipc__get_path()); @@ -41,7 +48,7 @@ int odb_over_ipc__command(const char *command, struct strbuf *answer) state = ipc_client_try_connect(odb_over_ipc__get_path(), &options, &connection); if (state != IPC_STATE__LISTENING) { - die("odb--daemon is not running"); + // error("odb--daemon is not running"); return -1; } @@ -49,12 +56,48 @@ int odb_over_ipc__command(const char *command, struct strbuf *answer) ipc_client_close_connection(connection); if (ret == -1) { - die("could not send '%s' command to odb--daemon", - command); + error("could not send '%s' command to odb--daemon", command); return -1; } return 0; } +int odb_over_ipc__get_oid(struct repository *r, const struct object_id *oid, + struct object_info *oi, unsigned flags) +{ + struct strbuf cmd = STRBUF_INIT; + struct strbuf answer = STRBUF_INIT; + int ret; + + if (is_daemon) + return -1; + + if (r != the_repository) // TODO not dealing with this + return -1; + + /* + * If we are going to the trouble to ask the daemon for information on + * the object, always get all of the optional fields. That is, don't + * worry with which fields within `oi` are populated on the request side. + */ + strbuf_addf(&cmd, "oid %s\n", oid_to_hex(oid)); + strbuf_addf(&cmd, "flags %"PRIuMAX"\n", (uintmax_t)flags); + + ret = odb_over_ipc__command(cmd.buf, &answer); + + strbuf_release(&cmd); + if (ret) + return ret; + + // TODO We expect either valid or error response.... + + trace2_printf("get_oid: '%s'", answer.buf); + + // TODO actually use the result... + + strbuf_release(&answer); + return -1; +} + #endif /* SUPPORTS_SIMPLE_IPC */ diff --git a/odb-over-ipc.h b/odb-over-ipc.h index 12c7eb7b7f8e98..4cf4f88416cd14 100644 --- a/odb-over-ipc.h +++ b/odb-over-ipc.h @@ -34,9 +34,6 @@ enum ipc_active_state odb_over_ipc__get_state(void); * and receive a response. If no daemon is running, this DOES NOT try * to start one. * - * Commands include: - * [] ask for an object - * * TODO If we can trust the code that creates/deletes packfiles, we * TODO might consider adding a command here to let that process tell * TODO the daemon to update the list of cached packfiles. @@ -46,5 +43,24 @@ enum ipc_active_state odb_over_ipc__get_state(void); */ int odb_over_ipc__command(const char *command, struct strbuf *answer); +/* + * Connect to an existing `git odb--daemon` process and ask it for + * an object. This is intended to be inserted into the client + * near `oid_object_info_extended()`. + * + * Returns non-zero when the caller should use the traditional + * method. + */ +struct object_info; + +int odb_over_ipc__get_oid(struct repository *r, const struct object_id *oid, + struct object_info *oi, unsigned flags); + +/* + * Insurance to protect the daemon from calling ODB code and accidentally + * falling into the client-side code and trying to connect to itself. + */ +void odb_over_ipc__set_is_daemon(void); + #endif /* SUPPORTS_SIMPLE_IPC */ #endif /* ODB_OVER_IPC_H */ From e0be696d21a860d3cd3a0c927c04cba660d5fac5 Mon Sep 17 00:00:00 2001 From: Jeff Hostetler Date: Mon, 12 Apr 2021 17:19:26 -0400 Subject: [PATCH 08/34] odb--daemon: handle get-oid response on client side Signed-off-by: Jeff Hostetler --- builtin/odb--daemon.c | 31 ++++++++----- odb-over-ipc.c | 101 ++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 118 insertions(+), 14 deletions(-) diff --git a/builtin/odb--daemon.c b/builtin/odb--daemon.c index 9e87bab1218953..6ea2dc8598247b 100644 --- a/builtin/odb--daemon.c +++ b/builtin/odb--daemon.c @@ -81,6 +81,8 @@ static int odb_ipc_cb__get_oid(struct my_odb_ipc_state *state, uintmax_t umax_flags = 0; int k; + trace2_printf("oid--daemon: received:\n%s", command); + oidclr(&oid); for (k = 0; lines[k]; k++) { @@ -108,7 +110,6 @@ static int odb_ipc_cb__get_oid(struct my_odb_ipc_state *state, unsigned long var_size = 0; off_t var_disk_size = 0; struct object_id var_delta_base_oid; - struct strbuf var_type_name = STRBUF_INIT; void *var_content = NULL; struct object_info var_oi = OBJECT_INFO_INIT; int ret; @@ -119,7 +120,8 @@ static int odb_ipc_cb__get_oid(struct my_odb_ipc_state *state, var_oi.sizep = &var_size; var_oi.disk_sizep = &var_disk_size; var_oi.delta_base_oid = &var_delta_base_oid; - var_oi.type_name = &var_type_name; + // TODO have the client send another field to indicate + // TODO whether it wants the contents or not. var_oi.contentp = &var_content; ret = oid_object_info_extended(the_repository, &oid, &var_oi, @@ -132,18 +134,26 @@ static int odb_ipc_cb__get_oid(struct my_odb_ipc_state *state, strbuf_addf(&response, "type %d\n", var_type); strbuf_addf(&response, "size %"PRIuMAX"\n", (uintmax_t)var_size); strbuf_addf(&response, "disk %"PRIuMAX"\n", (uintmax_t)var_disk_size); - // TODO only include delta base oid if non zero. - strbuf_addf(&response, "delta %s\n", oid_to_hex(&var_delta_base_oid)); - strbuf_addf(&response, "name %s\n", var_type_name.buf); + if (!is_null_oid(&var_delta_base_oid)) + strbuf_addf(&response, "delta %s\n", oid_to_hex(&var_delta_base_oid)); + // TODO should we create a new fake whence value that we report to + // TODO the client -- something like "ipc" to indicate that the + // TODO client got it from us and therefore doesn't have the in-memory + // TODO cache nor any of the packfile data loaded. The answer to this + // TODO also affects the oi.u.packed fields. + strbuf_addf(&response, "whence %d\n", var_oi.whence); - // TODO we do not care about oi.whence nor oi.u.packed + // TODO decide if we care about oi.u.packed - strbuf_addstr(&response, "content\n"); /* must be last */ + trace2_printf("oid--daemon: sending:\n%s", response.buf); - reply_cb(reply_data, response.buf, response.len); + /* + * Add one to the length of the headers to include the NUL and + * then immediately send the object contents. + */ + reply_cb(reply_data, response.buf, response.len + 1); reply_cb(reply_data, var_content, var_size); - strbuf_release(&var_type_name); free(var_content); } @@ -160,7 +170,8 @@ static int odb_ipc_cb__get_oid(struct my_odb_ipc_state *state, goto done; done: - strbuf_list_free(lines); + if (lines) + strbuf_list_free(lines); strbuf_release(&response); return 0; } diff --git a/odb-over-ipc.c b/odb-over-ipc.c index 0972e9b571d0bd..58912e27afaeb0 100644 --- a/odb-over-ipc.c +++ b/odb-over-ipc.c @@ -1,4 +1,6 @@ #include "cache.h" +#include "object.h" +#include "object-store.h" #include "simple-ipc.h" #include "odb-over-ipc.h" @@ -68,6 +70,13 @@ int odb_over_ipc__get_oid(struct repository *r, const struct object_id *oid, { struct strbuf cmd = STRBUF_INIT; struct strbuf answer = STRBUF_INIT; + struct strbuf headers = STRBUF_INIT; + struct strbuf **lines = NULL; + const char *sz; + const char *ch_nul; + const char *content; + ssize_t content_len; + int k; int ret; if (is_daemon) @@ -83,6 +92,7 @@ int odb_over_ipc__get_oid(struct repository *r, const struct object_id *oid, */ strbuf_addf(&cmd, "oid %s\n", oid_to_hex(oid)); strbuf_addf(&cmd, "flags %"PRIuMAX"\n", (uintmax_t)flags); + // TODO send another row to indicate whether we want the content buffer. ret = odb_over_ipc__command(cmd.buf, &answer); @@ -90,14 +100,97 @@ int odb_over_ipc__get_oid(struct repository *r, const struct object_id *oid, if (ret) return ret; - // TODO We expect either valid or error response.... + if (!strncmp(answer.buf, "error", 5)) { + trace2_printf("odb-over-ipc: failed for '%s'", oid_to_hex(oid)); + return -1; + } + + if (!oi) { + /* + * The caller doesn't care about the object itself; + * just whether it exists?? + */ + goto done; + } - trace2_printf("get_oid: '%s'", answer.buf); + /* Find the divider between the headers and the content. */ + ch_nul = strchr(answer.buf, '\0'); + content = ch_nul + 1; + content_len = &answer.buf[answer.len] - content; + + /* + * Extract the portion before the divider into another string so that + * we can split / parse it by lines. + */ + strbuf_add(&headers, answer.buf, (ch_nul - answer.buf)); + + lines = strbuf_split_str(headers.buf, '\n', 0); + strbuf_release(&headers); + + for (k = 0; lines[k]; k++) { + strbuf_trim_trailing_newline(lines[k]); + + if (skip_prefix(lines[k]->buf, "oid ", &sz)) { + assert(!strcmp(sz, oid_to_hex(oid))); + continue; + } + + if (skip_prefix(lines[k]->buf, "type ", &sz)) { + enum object_type t = strtol(sz, NULL, 10); + if (oi->typep) + *(oi->typep) = t; + if (oi->type_name) + strbuf_addstr(oi->type_name, type_name(t)); + continue; + } + + if (skip_prefix(lines[k]->buf, "size ", &sz)) { + ssize_t size = strtoumax(sz, NULL, 10); + assert(size == content_len); + + if (oi->sizep) + *(oi->sizep) = size; + continue; + } + + if (skip_prefix(lines[k]->buf, "disk ", &sz)) { + if (oi->disk_sizep) + *(oi->disk_sizep) = strtoumax(sz, NULL, 10); + continue; + } + + // TODO do we really care about the delta-base ?? + if (skip_prefix(lines[k]->buf, "delta ", &sz)) { + if (oi->delta_base_oid) { + oidclr(oi->delta_base_oid); + if (get_oid_hex(sz, oi->delta_base_oid)) { + error("could not parse delta base in odb-over-ipc response"); + ret = -1; + goto done; + } + } + continue; + } + + if (skip_prefix(lines[k]->buf, "whence ", &sz)) { + oi->whence = strtol(sz, NULL, 10); + continue; + } + + // TODO The server does not send the contents of oi.u.packed. + // TODO Do we care? + + BUG("unexpected line '%s' in OID response", lines[k]->buf); + } - // TODO actually use the result... + if (oi->contentp) + *oi->contentp = xmemdupz(content, content_len); +done: + if (lines) + strbuf_list_free(lines); strbuf_release(&answer); - return -1; + return ret; } #endif /* SUPPORTS_SIMPLE_IPC */ From 49069e6d32d67d664c2a958743c9fe002ad6cecd Mon Sep 17 00:00:00 2001 From: Jeff Hostetler Date: Tue, 13 Apr 2021 13:05:29 -0400 Subject: [PATCH 09/34] odb-over-ipc: fix for static analysis Signed-off-by: Jeff Hostetler --- odb-over-ipc.h | 1 + 1 file changed, 1 insertion(+) diff --git a/odb-over-ipc.h b/odb-over-ipc.h index 4cf4f88416cd14..f08ae1c32fda08 100644 --- a/odb-over-ipc.h +++ b/odb-over-ipc.h @@ -52,6 +52,7 @@ int odb_over_ipc__command(const char *command, struct strbuf *answer); * method. */ struct object_info; +struct repository; int odb_over_ipc__get_oid(struct repository *r, const struct object_id *oid, struct object_info *oi, unsigned flags); From df65f8919804cf49af9fb706eafbeefb8c775532 Mon Sep 17 00:00:00 2001 From: Jeff Hostetler Date: Tue, 13 Apr 2021 13:05:58 -0400 Subject: [PATCH 10/34] odb-over-ipc: don't use oid_to_hex() Test t3206 failed on various CI runs because of the unsafe `oid_to_hex()` function. Signed-off-by: Jeff Hostetler --- odb-over-ipc.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/odb-over-ipc.c b/odb-over-ipc.c index 58912e27afaeb0..3f4f156241c007 100644 --- a/odb-over-ipc.c +++ b/odb-over-ipc.c @@ -68,6 +68,7 @@ int odb_over_ipc__command(const char *command, struct strbuf *answer) int odb_over_ipc__get_oid(struct repository *r, const struct object_id *oid, struct object_info *oi, unsigned flags) { + char hex_buf[GIT_MAX_HEXSZ + 1]; struct strbuf cmd = STRBUF_INIT; struct strbuf answer = STRBUF_INIT; struct strbuf headers = STRBUF_INIT; @@ -90,7 +91,7 @@ int odb_over_ipc__get_oid(struct repository *r, const struct object_id *oid, * the object, always get all of the optional fields. That is, don't * worry with which fields within `oi` are populated on the request side. */ - strbuf_addf(&cmd, "oid %s\n", oid_to_hex(oid)); + strbuf_addf(&cmd, "oid %s\n", oid_to_hex_r(hex_buf, oid)); strbuf_addf(&cmd, "flags %"PRIuMAX"\n", (uintmax_t)flags); // TODO send another row to indicate whether we want the content buffer. From b18ac402184a54f50e27e6b039386b904f72ce70 Mon Sep 17 00:00:00 2001 From: Jeff Hostetler Date: Tue, 13 Apr 2021 14:18:58 -0400 Subject: [PATCH 11/34] ipc-unix-socket: refactor worker thread Refactor worker thread and replace break/continue statements with explicit goto labels. This will simplify things as we add a keepalive-like feature. Signed-off-by: Jeff Hostetler --- compat/simple-ipc/ipc-unix-socket.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/compat/simple-ipc/ipc-unix-socket.c b/compat/simple-ipc/ipc-unix-socket.c index 49d045010481e0..22f470a895af42 100644 --- a/compat/simple-ipc/ipc-unix-socket.c +++ b/compat/simple-ipc/ipc-unix-socket.c @@ -548,14 +548,19 @@ static void *worker_thread_proc(void *_worker_thread_data) thread_block_sigpipe(&old_set); +top: for (;;) { fd = worker_thread__wait_for_connection(worker_thread_data); - if (fd == -1) - break; /* in shutdown */ + if (fd == -1) { + /* shutdown already requested */ + goto shutdown; + } io = worker_thread__wait_for_io_start(worker_thread_data, fd); - if (io == -1) - continue; /* client hung up without sending anything */ + if (io == -1) { + /* client hung up without sending anything */ + goto top; + } ret = worker_thread__do_io(worker_thread_data, fd); @@ -576,10 +581,11 @@ static void *worker_thread_proc(void *_worker_thread_data) * responding to their current clients. */ ipc_server_stop_async(server_data); - break; + goto shutdown; } } +shutdown: trace2_thread_exit(); return NULL; } From 4c1e2564bab1bb4f682f02b9ae2258c12a6b930e Mon Sep 17 00:00:00 2001 From: Jeff Hostetler Date: Tue, 13 Apr 2021 13:58:09 -0400 Subject: [PATCH 12/34] ipc-unix-socket: refactor worker_thread__wait_for_io_start Simplify the cleanup code in `worker_thread__wait_for_io_start()` and move the `close(fd)` to the caller. This will help us add a keepalive like feature in a later commit. Signed-off-by: Jeff Hostetler --- compat/simple-ipc/ipc-unix-socket.c | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/compat/simple-ipc/ipc-unix-socket.c b/compat/simple-ipc/ipc-unix-socket.c index 22f470a895af42..ff3d86128fb185 100644 --- a/compat/simple-ipc/ipc-unix-socket.c +++ b/compat/simple-ipc/ipc-unix-socket.c @@ -405,7 +405,7 @@ static int do_io_reply_callback(struct ipc_server_reply_data *reply_data, * data), our use of the pkt-line read routines will spew an error * message. * - * Return -1 if the client hung up. + * Return -1 if the client hung up (or we are in a shutdown). * Return 0 if data (possibly incomplete) is ready. */ static int worker_thread__wait_for_io_start( @@ -424,7 +424,7 @@ static int worker_thread__wait_for_io_start( if (result < 0) { if (errno == EINTR) continue; - goto cleanup; + return -1; } if (result == 0) { @@ -441,22 +441,18 @@ static int worker_thread__wait_for_io_start( * client has not started talking yet, just drop it. */ if (in_shutdown) - goto cleanup; + return -1; continue; } if (pollfd[0].revents & POLLHUP) - goto cleanup; + return -1; if (pollfd[0].revents & POLLIN) return 0; - goto cleanup; + return -1; /* should not happen */ } - -cleanup: - close(fd); - return -1; } /* @@ -559,6 +555,7 @@ static void *worker_thread_proc(void *_worker_thread_data) io = worker_thread__wait_for_io_start(worker_thread_data, fd); if (io == -1) { /* client hung up without sending anything */ + close(fd); goto top; } From 98dd9a09425f92c8c6d902ed6a81abc1bd0f6f15 Mon Sep 17 00:00:00 2001 From: Jeff Hostetler Date: Tue, 13 Apr 2021 14:04:55 -0400 Subject: [PATCH 13/34] ipc-unix-socket: refactor worker_thread__do_io Refactor `worker_thread__do_io()` to let the caller close the fd. This will help us add a keepalive like feature in a later commit. Signed-off-by: Jeff Hostetler --- compat/simple-ipc/ipc-unix-socket.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compat/simple-ipc/ipc-unix-socket.c b/compat/simple-ipc/ipc-unix-socket.c index ff3d86128fb185..7f7424e7a8bb87 100644 --- a/compat/simple-ipc/ipc-unix-socket.c +++ b/compat/simple-ipc/ipc-unix-socket.c @@ -493,7 +493,6 @@ static int worker_thread__do_io( } strbuf_release(&buf); - close(reply_data.fd); return ret; } @@ -560,6 +559,7 @@ static void *worker_thread_proc(void *_worker_thread_data) } ret = worker_thread__do_io(worker_thread_data, fd); + close(fd); if (ret == SIMPLE_IPC_QUIT) { trace2_data_string("ipc-worker", NULL, "queue_stop_async", From 2bbe2a7b9b890f4c2cca20377089b673e529d863 Mon Sep 17 00:00:00 2001 From: Jeff Hostetler Date: Tue, 13 Apr 2021 14:33:55 -0400 Subject: [PATCH 14/34] ipc-unix-socket: refactor SIMPLE_IPC_QUIT handling Refactor `worker_thread_proc()` and move our `SIMPLE_IPC_QUIT` handling outside of the loop. This will help us add a keepalive like feature in a later commit. Signed-off-by: Jeff Hostetler --- compat/simple-ipc/ipc-unix-socket.c | 40 +++++++++++++++-------------- 1 file changed, 21 insertions(+), 19 deletions(-) diff --git a/compat/simple-ipc/ipc-unix-socket.c b/compat/simple-ipc/ipc-unix-socket.c index 7f7424e7a8bb87..0a5c47c408bc2e 100644 --- a/compat/simple-ipc/ipc-unix-socket.c +++ b/compat/simple-ipc/ipc-unix-socket.c @@ -561,27 +561,29 @@ static void *worker_thread_proc(void *_worker_thread_data) ret = worker_thread__do_io(worker_thread_data, fd); close(fd); - if (ret == SIMPLE_IPC_QUIT) { - trace2_data_string("ipc-worker", NULL, "queue_stop_async", - "application_quit"); - /* - * The application layer is telling the ipc-server - * layer to shutdown. - * - * We DO NOT have a response to send to the client. - * - * Queue an async stop (to stop the other threads) and - * allow this worker thread to exit now (no sense waiting - * for the thread-pool shutdown signal). - * - * Other non-idle worker threads are allowed to finish - * responding to their current clients. - */ - ipc_server_stop_async(server_data); - goto shutdown; - } + if (ret == SIMPLE_IPC_QUIT) + goto quit; } +quit: + trace2_data_string("ipc-worker", NULL, "queue_stop_async", + "application_quit"); + /* + * The application layer is telling the ipc-server + * layer to shutdown. + * + * We DO NOT have a response to send to the client. + * + * Queue an async stop (to stop the other threads) and + * allow this worker thread to exit now (no sense waiting + * for the thread-pool shutdown signal). + * + * Other non-idle worker threads are allowed to finish + * responding to their current clients. + */ + ipc_server_stop_async(server_data); + goto shutdown; + shutdown: trace2_thread_exit(); return NULL; From a3a203a4b5becc04570a814061b2d690b90ab332 Mon Sep 17 00:00:00 2001 From: Jeff Hostetler Date: Tue, 13 Apr 2021 15:33:41 -0400 Subject: [PATCH 15/34] ipc-unix-socket: add keepalive support Teach `ipc-unix-socket` to keep a connection alive after the first request/response and accept a series of them. To maintain sync, each side must send a flush at the end of their message. This detail is hidden within the IPC layer. This extends the "Simple IPC" protocol in the spirit of keepalive. Signed-off-by: Jeff Hostetler --- compat/simple-ipc/ipc-unix-socket.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/compat/simple-ipc/ipc-unix-socket.c b/compat/simple-ipc/ipc-unix-socket.c index 0a5c47c408bc2e..36148cf11fc1db 100644 --- a/compat/simple-ipc/ipc-unix-socket.c +++ b/compat/simple-ipc/ipc-unix-socket.c @@ -551,18 +551,19 @@ static void *worker_thread_proc(void *_worker_thread_data) goto shutdown; } - io = worker_thread__wait_for_io_start(worker_thread_data, fd); - if (io == -1) { - /* client hung up without sending anything */ - close(fd); - goto top; - } + for (;;) { + io = worker_thread__wait_for_io_start(worker_thread_data, fd); + if (io == -1) { + /* client hung up without sending anything */ + close(fd); + goto top; + } - ret = worker_thread__do_io(worker_thread_data, fd); - close(fd); + ret = worker_thread__do_io(worker_thread_data, fd); - if (ret == SIMPLE_IPC_QUIT) - goto quit; + if (ret == SIMPLE_IPC_QUIT) + goto quit; + } } quit: From b13c45d9571e9723aee76b2d73fe1fed1a8ae127 Mon Sep 17 00:00:00 2001 From: Jeff Hostetler Date: Tue, 13 Apr 2021 16:11:15 -0400 Subject: [PATCH 16/34] test-simple-ipc: add unit test for keepalive over simple ipc Signed-off-by: Jeff Hostetler --- t/helper/test-simple-ipc.c | 49 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/t/helper/test-simple-ipc.c b/t/helper/test-simple-ipc.c index 42040ef81b1e84..c309322c824582 100644 --- a/t/helper/test-simple-ipc.c +++ b/t/helper/test-simple-ipc.c @@ -676,6 +676,48 @@ static int client__multiple(void) return (sum_join_errors + sum_thread_errors) ? 1 : 0; } +/* + * Send a series of commands over a single connection keepalive-style. + */ +static int client__keepalive(void) +{ + struct ipc_client_connection *connection = NULL; + struct ipc_client_connect_options options + = IPC_CLIENT_CONNECT_OPTIONS_INIT; + struct strbuf answer = STRBUF_INIT; + int ret = 0; + int k; + enum ipc_active_state state; + + strbuf_reset(&answer); + + options.wait_if_busy = 1; + options.wait_if_not_found = 0; + options.uds_disallow_chdir = 0; + + state = ipc_client_try_connect(cl_args.path, &options, &connection); + if (state != IPC_STATE__LISTENING) + return error("failed to connect to '%s'", cl_args.path); + + for (k = 0; k < 10; k++) { + ret = ipc_client_send_command_to_connection(connection, "ping", &answer); + if (ret) { + error("failed to send ping[%d]", k); + goto cleanup; + } + if (answer.len) { + printf("%s\n", answer.buf); + fflush(stdout); + } + } + +cleanup: + ipc_client_close_connection(connection); + strbuf_release(&answer); + + return ret; +} + int cmd__simple_ipc(int argc, const char **argv) { const char * const simple_ipc_usage[] = { @@ -686,6 +728,7 @@ int cmd__simple_ipc(int argc, const char **argv) N_("test-helper simple-ipc send [] []"), N_("test-helper simple-ipc sendbytes [] [] []"), N_("test-helper simple-ipc multiple [] [] [] []"), + N_("test-helper simple-ipc keepalive []"), NULL }; @@ -782,6 +825,12 @@ int cmd__simple_ipc(int argc, const char **argv) return !!client__multiple(); } + if (!strcmp(cl_args.subcommand, "keepalive")) { + if (client__probe_server()) + return 1; + return !!client__keepalive(); + } + die("Unhandled subcommand: '%s'", cl_args.subcommand); } #endif From 2fd2c66efab34d7aaac3434d87052e84b87e07dc Mon Sep 17 00:00:00 2001 From: Jeff Hostetler Date: Tue, 13 Apr 2021 16:27:07 -0400 Subject: [PATCH 17/34] ipc-win32: add keepalive support Teach `ipc-win32` to keep a connection alive after the first request/response and accept a series of them. Signed-off-by: Jeff Hostetler --- compat/simple-ipc/ipc-win32.c | 37 +++++++++++++++++++++-------------- 1 file changed, 22 insertions(+), 15 deletions(-) diff --git a/compat/simple-ipc/ipc-win32.c b/compat/simple-ipc/ipc-win32.c index 8f89c02037e36c..3d8432d6bd6850 100644 --- a/compat/simple-ipc/ipc-win32.c +++ b/compat/simple-ipc/ipc-win32.c @@ -448,23 +448,30 @@ static int do_io(struct ipc_server_thread_data *server_thread_data) return error(_("could not create fd from pipe for '%s'"), server_thread_data->server_data->buf_path.buf); - ret = read_packetized_to_strbuf( - reply_data.fd, &buf, - PACKET_READ_GENTLE_ON_EOF | PACKET_READ_GENTLE_ON_READ_ERROR); - if (ret >= 0) { - ret = server_thread_data->server_data->application_cb( - server_thread_data->server_data->application_data, - buf.buf, do_io_reply_callback, &reply_data); + for (;;) { + strbuf_reset(&buf); + ret = read_packetized_to_strbuf( + reply_data.fd, &buf, + PACKET_READ_GENTLE_ON_EOF | PACKET_READ_GENTLE_ON_READ_ERROR); + if (ret >= 0) { + ret = server_thread_data->server_data->application_cb( + server_thread_data->server_data->application_data, + buf.buf, do_io_reply_callback, &reply_data); - packet_flush_gently(reply_data.fd); + packet_flush_gently(reply_data.fd); - FlushFileBuffers((HANDLE)_get_osfhandle((reply_data.fd))); - } - else { - /* - * The client probably disconnected/shutdown before it - * could send a well-formed message. Ignore it. - */ + FlushFileBuffers((HANDLE)_get_osfhandle((reply_data.fd))); + + if (ret == SIMPLE_IPC_QUIT) + break; + } + else { + /* + * The client probably disconnected/shutdown before it + * could send a well-formed message. Ignore it. + */ + break; + } } strbuf_release(&buf); From 58461d374427a065573f33d9cd5646f6bd96bedb Mon Sep 17 00:00:00 2001 From: Jeff Hostetler Date: Wed, 14 Apr 2021 10:28:44 -0400 Subject: [PATCH 18/34] t0052: add keepalive unit test Signed-off-by: Jeff Hostetler --- t/t0052-simple-ipc.sh | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/t/t0052-simple-ipc.sh b/t/t0052-simple-ipc.sh index ff98be31a51b36..9737e897fc5400 100755 --- a/t/t0052-simple-ipc.sh +++ b/t/t0052-simple-ipc.sh @@ -113,6 +113,15 @@ test_expect_success 'stress test threads' ' test_cmp expect_a actual_a ' +# Test keepalive feature. Have client open a single connection +# and exchange a series of requests/responses over it. +# +test_expect_success 'keepalive' ' + test-tool simple-ipc keepalive >actual && + grep "pong" actual >actual_pong && + test_line_count = 10 actual_pong +' + test_expect_success 'stop-daemon works' ' test-tool simple-ipc stop-daemon && test_must_fail test-tool simple-ipc is-active && From 092c418ff58dd1271b2326a24cdc74c1ee7ad4c5 Mon Sep 17 00:00:00 2001 From: Jeff Hostetler Date: Wed, 14 Apr 2021 12:56:13 -0400 Subject: [PATCH 19/34] odb-over-ipc: make use of keepalive-style connection Signed-off-by: Jeff Hostetler --- odb-over-ipc.c | 69 ++++++++++++++++++++++++++++++++++++++++---------- odb-over-ipc.h | 7 +++++ 2 files changed, 62 insertions(+), 14 deletions(-) diff --git a/odb-over-ipc.c b/odb-over-ipc.c index 3f4f156241c007..8b790598736b91 100644 --- a/odb-over-ipc.c +++ b/odb-over-ipc.c @@ -34,31 +34,72 @@ enum ipc_active_state odb_over_ipc__get_state(void) return ipc_get_active_state(odb_over_ipc__get_path()); } +// TODO This is a hackathon project, so I'm not going to worry about +// TODO ensuring that full threading works right now. That is, I'm +// TODO NOT going to give each thread its own connection to the server +// TODO and I'm NOT going to install locking to let concurrent threads +// TODO properly share a single connection. +// TODO +// TODO We already know that the ODB has limited thread-safety, so I'm +// TODO going to rely on our callers to already behave themselves. +// +static struct ipc_client_connection *my_ipc_connection; +static int my_ipc_available = -1; + +// TOOD We need someone to call this to close our connection after we +// TODO have finished with the ODB. Yes, it will be implicitly closed +// TODO when the foreground Git client process exits, but we are +// TODO holding a connection and thread in the `git odb--daemon` open +// TODO and should try to release it quickly. +// +void odb_over_ipc__shutdown_keepalive_connection(void) +{ + if (my_ipc_connection) { + ipc_client_close_connection(my_ipc_connection); + my_ipc_connection = NULL; + } + + /* + * Assume that we shutdown a fully functioning connection and + * could reconnect again if desired. Our caller can reset this + * assumption, for example when it gets an error. + */ + my_ipc_available = 1; +} + int odb_over_ipc__command(const char *command, struct strbuf *answer) { - struct ipc_client_connection *connection = NULL; - struct ipc_client_connect_options options - = IPC_CLIENT_CONNECT_OPTIONS_INIT; int ret; - enum ipc_active_state state; - strbuf_reset(answer); + if (my_ipc_available == -1) { + enum ipc_active_state state; + struct ipc_client_connect_options options + = IPC_CLIENT_CONNECT_OPTIONS_INIT; - options.wait_if_busy = 1; - options.wait_if_not_found = 0; + options.wait_if_busy = 1; + options.wait_if_not_found = 0; - state = ipc_client_try_connect(odb_over_ipc__get_path(), &options, - &connection); - if (state != IPC_STATE__LISTENING) { - // error("odb--daemon is not running"); - return -1; + state = ipc_client_try_connect(odb_over_ipc__get_path(), &options, + &my_ipc_connection); + if (state != IPC_STATE__LISTENING) { + // error("odb--daemon is not running"); + my_ipc_available = 0; + return -1; + } + + my_ipc_available = 1; } + if (!my_ipc_available) + return -1; + + strbuf_reset(answer); - ret = ipc_client_send_command_to_connection(connection, command, answer); - ipc_client_close_connection(connection); + ret = ipc_client_send_command_to_connection(my_ipc_connection, command, answer); if (ret == -1) { error("could not send '%s' command to odb--daemon", command); + odb_over_ipc__shutdown_keepalive_connection(); + my_ipc_available = 0; return -1; } diff --git a/odb-over-ipc.h b/odb-over-ipc.h index f08ae1c32fda08..9bfe11c2bc0272 100644 --- a/odb-over-ipc.h +++ b/odb-over-ipc.h @@ -57,6 +57,13 @@ struct repository; int odb_over_ipc__get_oid(struct repository *r, const struct object_id *oid, struct object_info *oi, unsigned flags); +/* + * Explicitly shutdown IPC connection to the `git odb--daemon` process. + * The connection is implicitly created upon the first request and we + * use a keep-alive model to re-use it for subsequent requests. + */ +void odb_over_ipc__shutdown_keepalive_connection(void); + /* * Insurance to protect the daemon from calling ODB code and accidentally * falling into the client-side code and trying to connect to itself. From 47e9683c0f3827b3de5926588cada002caf0ea19 Mon Sep 17 00:00:00 2001 From: Jeff Hostetler Date: Wed, 14 Apr 2021 15:38:19 -0400 Subject: [PATCH 20/34] odb--daemon: add trace2 regions around object fetching Signed-off-by: Jeff Hostetler --- builtin/odb--daemon.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/builtin/odb--daemon.c b/builtin/odb--daemon.c index 6ea2dc8598247b..3bfd5f21cfc295 100644 --- a/builtin/odb--daemon.c +++ b/builtin/odb--daemon.c @@ -81,7 +81,7 @@ static int odb_ipc_cb__get_oid(struct my_odb_ipc_state *state, uintmax_t umax_flags = 0; int k; - trace2_printf("oid--daemon: received:\n%s", command); + // trace2_printf("oid--daemon: received:\n%s", command); oidclr(&oid); @@ -145,7 +145,7 @@ static int odb_ipc_cb__get_oid(struct my_odb_ipc_state *state, // TODO decide if we care about oi.u.packed - trace2_printf("oid--daemon: sending:\n%s", response.buf); + // trace2_printf("oid--daemon: sending:\n%s", response.buf); /* * Add one to the length of the headers to include the NUL and @@ -192,6 +192,7 @@ static int odb_ipc_cb(void *data, const char *command, struct ipc_server_reply_data *reply_data) { struct my_odb_ipc_state *state = data; + int ret; assert(state == &my_state); @@ -214,7 +215,11 @@ static int odb_ipc_cb(void *data, const char *command, * A client has requested that we lookup an object from the * ODB and send it to them. */ - return odb_ipc_cb__get_oid(state, command, reply_cb, reply_data); + trace2_region_enter("odb-daemon", "get-oid", NULL); + ret = odb_ipc_cb__get_oid(state, command, reply_cb, reply_data); + trace2_region_leave("odb-daemon", "get-oid", NULL); + + return ret; } // TODO respond to other requests from client. From 59f44905ad583b76d9caa1375c6ae5e2fe5bc1c1 Mon Sep 17 00:00:00 2001 From: Jeff Hostetler Date: Thu, 15 Apr 2021 10:39:09 -0400 Subject: [PATCH 21/34] odb-over-ipc: only send object content if requested Signed-off-by: Jeff Hostetler --- builtin/odb--daemon.c | 16 +++++++++++----- odb-over-ipc.c | 7 ++++--- 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/builtin/odb--daemon.c b/builtin/odb--daemon.c index 3bfd5f21cfc295..f5bb8ec7b8dde7 100644 --- a/builtin/odb--daemon.c +++ b/builtin/odb--daemon.c @@ -79,6 +79,7 @@ static int odb_ipc_cb__get_oid(struct my_odb_ipc_state *state, struct object_id oid; const char *sz; uintmax_t umax_flags = 0; + int want_content = 0; int k; // trace2_printf("oid--daemon: received:\n%s", command); @@ -99,6 +100,11 @@ static int odb_ipc_cb__get_oid(struct my_odb_ipc_state *state, continue; } + if (skip_prefix(lines[k]->buf, "content ", &sz)) { + want_content = (*sz == 't'); + continue; + } + BUG("unexpected line '%s' in OID request", lines[k]->buf); } @@ -120,9 +126,8 @@ static int odb_ipc_cb__get_oid(struct my_odb_ipc_state *state, var_oi.sizep = &var_size; var_oi.disk_sizep = &var_disk_size; var_oi.delta_base_oid = &var_delta_base_oid; - // TODO have the client send another field to indicate - // TODO whether it wants the contents or not. - var_oi.contentp = &var_content; + if (want_content) + var_oi.contentp = &var_content; ret = oid_object_info_extended(the_repository, &oid, &var_oi, (unsigned)umax_flags); @@ -149,10 +154,11 @@ static int odb_ipc_cb__get_oid(struct my_odb_ipc_state *state, /* * Add one to the length of the headers to include the NUL and - * then immediately send the object contents. + * then (if requested) immediately send the object contents. */ reply_cb(reply_data, response.buf, response.len + 1); - reply_cb(reply_data, var_content, var_size); + if (want_content) + reply_cb(reply_data, var_content, var_size); free(var_content); } diff --git a/odb-over-ipc.c b/odb-over-ipc.c index 8b790598736b91..c686ab8bce9f8a 100644 --- a/odb-over-ipc.c +++ b/odb-over-ipc.c @@ -134,7 +134,7 @@ int odb_over_ipc__get_oid(struct repository *r, const struct object_id *oid, */ strbuf_addf(&cmd, "oid %s\n", oid_to_hex_r(hex_buf, oid)); strbuf_addf(&cmd, "flags %"PRIuMAX"\n", (uintmax_t)flags); - // TODO send another row to indicate whether we want the content buffer. + strbuf_addf(&cmd, "content %c\n", (oi && oi->contentp ? 't' : 'f')); ret = odb_over_ipc__command(cmd.buf, &answer); @@ -158,6 +158,7 @@ int odb_over_ipc__get_oid(struct repository *r, const struct object_id *oid, /* Find the divider between the headers and the content. */ ch_nul = strchr(answer.buf, '\0'); content = ch_nul + 1; + /* The content_len is only defined if we asked for content. */ content_len = &answer.buf[answer.len] - content; /* @@ -188,10 +189,10 @@ int odb_over_ipc__get_oid(struct repository *r, const struct object_id *oid, if (skip_prefix(lines[k]->buf, "size ", &sz)) { ssize_t size = strtoumax(sz, NULL, 10); - assert(size == content_len); - if (oi->sizep) *(oi->sizep) = size; + if (oi->contentp && size != content_len) + BUG("observed content length does not match size"); continue; } From e7c187ec9293c36f1109bb79b1a7581af854a60f Mon Sep 17 00:00:00 2001 From: Jeff Hostetler Date: Fri, 16 Apr 2021 08:33:53 -0400 Subject: [PATCH 22/34] simple-ipc: preparations for supporting binary messages. Add `command_len` argument to the Simple IPC API. In my original Simple IPC API, I assumed that the request would always be a null-terminated string of text characters. The command arg was just a `const char *`. I found a caller that would like to pass a binary command to the daemon, so I want to ammend the Simple IPC API to take `const char *command, size_t command_len` and pass that to the daemon. (Really, the first arg should just be a `void *` or `const unsigned byte *` to make that clear, but I'm not going to bother right now.) Note, the response side has always been a `struct strbuf` which includes the buffer and length, so we already support returning a binary answer. (Yes, it feels a little weird returning a binary buffer in a `strbuf`, but it works.) Signed-off-by: Jeff Hostetler --- builtin/fsmonitor--daemon.c | 16 ++++++++++++++- builtin/odb--daemon.c | 16 +++++++++++---- compat/simple-ipc/ipc-unix-socket.c | 14 ++++++++----- fsmonitor-ipc.c | 7 +++++-- odb-over-ipc.c | 9 ++++++--- odb-over-ipc.h | 3 ++- simple-ipc.h | 7 +++++-- t/helper/test-simple-ipc.c | 31 +++++++++++++++++++++++------ 8 files changed, 79 insertions(+), 24 deletions(-) diff --git a/builtin/fsmonitor--daemon.c b/builtin/fsmonitor--daemon.c index d6b59a98ceddd5..d9b93af01dec6b 100644 --- a/builtin/fsmonitor--daemon.c +++ b/builtin/fsmonitor--daemon.c @@ -996,10 +996,24 @@ static int do_handle_client(struct fsmonitor_daemon_state *state, static ipc_server_application_cb handle_client; -static int handle_client(void *data, const char *command, +static int handle_client(void *data, + const char *command, size_t command_len, ipc_server_reply_cb *reply, struct ipc_server_reply_data *reply_data) { + // TODO I did not take time to ensure that `command_len` is + // TODO large enough to do all of the strcmp() and starts_with() + // TODO calculations when I converted the IPC API to take + // TODO `command, command_len` rather than just `command`. + // TODO So some cleanup is needed here. + // TODO + // TODO But we know that FSMonitor always uses regular text + // TODO in both the request and response (no binary messages + // TODO here), so the cleanup is more of the `assert` form. + // TODO + // TODO For example, I should pass `command, command_len` into + // TODO `do_handle_client()` for completeness. + struct fsmonitor_daemon_state *state = data; int result; diff --git a/builtin/odb--daemon.c b/builtin/odb--daemon.c index f5bb8ec7b8dde7..bbe3579c6368d0 100644 --- a/builtin/odb--daemon.c +++ b/builtin/odb--daemon.c @@ -70,7 +70,7 @@ struct my_odb_ipc_state static struct my_odb_ipc_state my_state; static int odb_ipc_cb__get_oid(struct my_odb_ipc_state *state, - const char *command, + const char *command, size_t command_len, ipc_server_reply_cb *reply_cb, struct ipc_server_reply_data *reply_data) { @@ -193,10 +193,17 @@ static int odb_ipc_cb__get_oid(struct my_odb_ipc_state *state, */ static ipc_server_application_cb odb_ipc_cb; -static int odb_ipc_cb(void *data, const char *command, +static int odb_ipc_cb(void *data, + const char *command, size_t command_len, ipc_server_reply_cb *reply_cb, struct ipc_server_reply_data *reply_data) { + // TODO I did not take time to ensure that `command_len` is + // TODO large enough to do all of the strcmp() and starts_with() + // TODO calculations when I converted the IPC API to take + // TODO `command, command_len` rather than just `command`. + // TODO So some cleanup is needed here. + struct my_odb_ipc_state *state = data; int ret; @@ -222,7 +229,8 @@ static int odb_ipc_cb(void *data, const char *command, * ODB and send it to them. */ trace2_region_enter("odb-daemon", "get-oid", NULL); - ret = odb_ipc_cb__get_oid(state, command, reply_cb, reply_data); + ret = odb_ipc_cb__get_oid(state, command, command_len, + reply_cb, reply_data); trace2_region_leave("odb-daemon", "get-oid", NULL); return ret; @@ -310,7 +318,7 @@ static int client_send_stop(void) struct strbuf answer = STRBUF_INIT; int ret; - ret = odb_over_ipc__command("quit", &answer); + ret = odb_over_ipc__command("quit", 4, &answer); /* The quit command does not return any response data. */ strbuf_release(&answer); diff --git a/compat/simple-ipc/ipc-unix-socket.c b/compat/simple-ipc/ipc-unix-socket.c index 36148cf11fc1db..37d47fd992222e 100644 --- a/compat/simple-ipc/ipc-unix-socket.c +++ b/compat/simple-ipc/ipc-unix-socket.c @@ -150,7 +150,8 @@ void ipc_client_close_connection(struct ipc_client_connection *connection) int ipc_client_send_command_to_connection( struct ipc_client_connection *connection, - const char *message, struct strbuf *answer) + const char *message, size_t message_len, + struct strbuf *answer) { int ret = 0; @@ -158,7 +159,7 @@ int ipc_client_send_command_to_connection( trace2_region_enter("ipc-client", "send-command", NULL); - if (write_packetized_from_buf_no_flush(message, strlen(message), + if (write_packetized_from_buf_no_flush(message, message_len, connection->fd) < 0 || packet_flush_gently(connection->fd) < 0) { ret = error(_("could not send IPC command")); @@ -179,7 +180,8 @@ int ipc_client_send_command_to_connection( int ipc_client_send_command(const char *path, const struct ipc_client_connect_options *options, - const char *message, struct strbuf *answer) + const char *message, size_t message_len, + struct strbuf *answer) { int ret = -1; enum ipc_active_state state; @@ -190,7 +192,9 @@ int ipc_client_send_command(const char *path, if (state != IPC_STATE__LISTENING) return ret; - ret = ipc_client_send_command_to_connection(connection, message, answer); + ret = ipc_client_send_command_to_connection(connection, + message, message_len, + answer); ipc_client_close_connection(connection); @@ -481,7 +485,7 @@ static int worker_thread__do_io( if (ret >= 0) { ret = worker_thread_data->server_data->application_cb( worker_thread_data->server_data->application_data, - buf.buf, do_io_reply_callback, &reply_data); + buf.buf, buf.len, do_io_reply_callback, &reply_data); packet_flush_gently(reply_data.fd); } diff --git a/fsmonitor-ipc.c b/fsmonitor-ipc.c index b0dc334ff02d43..0b6107bc890769 100644 --- a/fsmonitor-ipc.c +++ b/fsmonitor-ipc.c @@ -63,7 +63,8 @@ int fsmonitor_ipc__send_query(const char *since_token, switch (state) { case IPC_STATE__LISTENING: ret = ipc_client_send_command_to_connection( - connection, since_token, answer); + connection, since_token, strlen(since_token), + answer); ipc_client_close_connection(connection); trace2_data_intmax("fsm_client", NULL, @@ -138,7 +139,9 @@ int fsmonitor_ipc__send_command(const char *command, return -1; } - ret = ipc_client_send_command_to_connection(connection, command, answer); + ret = ipc_client_send_command_to_connection(connection, + command, strlen(command), + answer); ipc_client_close_connection(connection); if (ret == -1) { diff --git a/odb-over-ipc.c b/odb-over-ipc.c index c686ab8bce9f8a..50c778979f8371 100644 --- a/odb-over-ipc.c +++ b/odb-over-ipc.c @@ -67,7 +67,8 @@ void odb_over_ipc__shutdown_keepalive_connection(void) my_ipc_available = 1; } -int odb_over_ipc__command(const char *command, struct strbuf *answer) +int odb_over_ipc__command(const char *command, size_t command_len, + struct strbuf *answer) { int ret; @@ -94,7 +95,9 @@ int odb_over_ipc__command(const char *command, struct strbuf *answer) strbuf_reset(answer); - ret = ipc_client_send_command_to_connection(my_ipc_connection, command, answer); + ret = ipc_client_send_command_to_connection(my_ipc_connection, + command, command_len, + answer); if (ret == -1) { error("could not send '%s' command to odb--daemon", command); @@ -136,7 +139,7 @@ int odb_over_ipc__get_oid(struct repository *r, const struct object_id *oid, strbuf_addf(&cmd, "flags %"PRIuMAX"\n", (uintmax_t)flags); strbuf_addf(&cmd, "content %c\n", (oi && oi->contentp ? 't' : 'f')); - ret = odb_over_ipc__command(cmd.buf, &answer); + ret = odb_over_ipc__command(cmd.buf, cmd.len, &answer); strbuf_release(&cmd); if (ret) diff --git a/odb-over-ipc.h b/odb-over-ipc.h index 9bfe11c2bc0272..a47d9eb5862559 100644 --- a/odb-over-ipc.h +++ b/odb-over-ipc.h @@ -41,7 +41,8 @@ enum ipc_active_state odb_over_ipc__get_state(void); * TODO For simplicit during prototyping I am NOT going to * TODO auto-start one. Revisit this later. */ -int odb_over_ipc__command(const char *command, struct strbuf *answer); +int odb_over_ipc__command(const char *command, size_t command_len, + struct strbuf *answer); /* * Connect to an existing `git odb--daemon` process and ask it for diff --git a/simple-ipc.h b/simple-ipc.h index dc3606e30bd619..c4d5225b41c2a4 100644 --- a/simple-ipc.h +++ b/simple-ipc.h @@ -111,7 +111,8 @@ void ipc_client_close_connection(struct ipc_client_connection *connection); */ int ipc_client_send_command_to_connection( struct ipc_client_connection *connection, - const char *message, struct strbuf *answer); + const char *message, size_t message_len, + struct strbuf *answer); /* * Used by the client to synchronously connect and send and receive a @@ -123,7 +124,8 @@ int ipc_client_send_command_to_connection( */ int ipc_client_send_command(const char *path, const struct ipc_client_connect_options *options, - const char *message, struct strbuf *answer); + const char *message, size_t message_len, + struct strbuf *answer); /* * Simple IPC Server Side API. @@ -148,6 +150,7 @@ typedef int (ipc_server_reply_cb)(struct ipc_server_reply_data *, */ typedef int (ipc_server_application_cb)(void *application_data, const char *request, + size_t request_len, ipc_server_reply_cb *reply_cb, struct ipc_server_reply_data *reply_data); diff --git a/t/helper/test-simple-ipc.c b/t/helper/test-simple-ipc.c index c309322c824582..fb1638e179d5e4 100644 --- a/t/helper/test-simple-ipc.c +++ b/t/helper/test-simple-ipc.c @@ -112,7 +112,7 @@ static int app__slow_command(ipc_server_reply_cb *reply_cb, /* * The client sent a command followed by a (possibly very) large buffer. */ -static int app__sendbytes_command(const char *received, +static int app__sendbytes_command(const char *received, size_t received_len, ipc_server_reply_cb *reply_cb, struct ipc_server_reply_data *reply_data) { @@ -123,6 +123,12 @@ static int app__sendbytes_command(const char *received, int errs = 0; int ret; + // TODO I did not take time to ensure that `received_len` is + // TODO large enough to do all of the skip_prefix() + // TODO calculations when I converted the IPC API to take + // TODO `received, received_len` rather than just `received`. + // TODO So some cleanup is needed here. + if (skip_prefix(received, "sendbytes ", &p)) len_ballast = strlen(p); @@ -160,10 +166,16 @@ static ipc_server_application_cb test_app_cb; * by this application. */ static int test_app_cb(void *application_data, - const char *command, + const char *command, size_t command_len, ipc_server_reply_cb *reply_cb, struct ipc_server_reply_data *reply_data) { + // TODO I did not take time to ensure that `command_len` is + // TODO large enough to do all of the strcmp() and starts_with() + // TODO calculations when I converted the IPC API to take + // TODO `command, command_len` rather than just `command`. + // TODO So some cleanup is needed here. + /* * Verify that we received the application-data that we passed * when we started the ipc-server. (We have several layers of @@ -208,7 +220,8 @@ static int test_app_cb(void *application_data, return app__slow_command(reply_cb, reply_data); if (starts_with(command, "sendbytes ")) - return app__sendbytes_command(command, reply_cb, reply_data); + return app__sendbytes_command(command, command_len, + reply_cb, reply_data); return app__unhandled_command(command, reply_cb, reply_data); } @@ -488,7 +501,9 @@ static int client__send_ipc(void) options.wait_if_busy = 1; options.wait_if_not_found = 0; - if (!ipc_client_send_command(cl_args.path, &options, command, &buf)) { + if (!ipc_client_send_command(cl_args.path, &options, + command, strlen(command), + &buf)) { if (buf.len) { printf("%s\n", buf.buf); fflush(stdout); @@ -556,7 +571,9 @@ static int do_sendbytes(int bytecount, char byte, const char *path, strbuf_addstr(&buf_send, "sendbytes "); strbuf_addchars(&buf_send, byte, bytecount); - if (!ipc_client_send_command(path, options, buf_send.buf, &buf_resp)) { + if (!ipc_client_send_command(path, options, + buf_send.buf, buf_send.len, + &buf_resp)) { strbuf_rtrim(&buf_resp); printf("sent:%c%08d %s\n", byte, bytecount, buf_resp.buf); fflush(stdout); @@ -700,7 +717,9 @@ static int client__keepalive(void) return error("failed to connect to '%s'", cl_args.path); for (k = 0; k < 10; k++) { - ret = ipc_client_send_command_to_connection(connection, "ping", &answer); + ret = ipc_client_send_command_to_connection(connection, + "ping", 4, + &answer); if (ret) { error("failed to send ping[%d]", k); goto cleanup; From 149a5ab215aa98933a0ee13dc20e1c421f66600a Mon Sep 17 00:00:00 2001 From: Jeff Hostetler Date: Fri, 16 Apr 2021 09:05:06 -0400 Subject: [PATCH 23/34] odb-over-ipc: convert request to binary Signed-off-by: Jeff Hostetler --- builtin/odb--daemon.c | 48 +++++++++---------------------------------- odb-over-ipc.c | 21 ++++++++----------- odb-over-ipc.h | 13 ++++++++++++ 3 files changed, 31 insertions(+), 51 deletions(-) diff --git a/builtin/odb--daemon.c b/builtin/odb--daemon.c index bbe3579c6368d0..b744b4d52f4bf6 100644 --- a/builtin/odb--daemon.c +++ b/builtin/odb--daemon.c @@ -74,39 +74,13 @@ static int odb_ipc_cb__get_oid(struct my_odb_ipc_state *state, ipc_server_reply_cb *reply_cb, struct ipc_server_reply_data *reply_data) { - struct strbuf **lines = strbuf_split_str(command, '\n', 0); + struct odb_over_ipc__get_oid__request *req; struct strbuf response = STRBUF_INIT; - struct object_id oid; - const char *sz; - uintmax_t umax_flags = 0; - int want_content = 0; - int k; - // trace2_printf("oid--daemon: received:\n%s", command); + if (command_len != sizeof(*req)) + BUG("incorrect size for binary data"); - oidclr(&oid); - - for (k = 0; lines[k]; k++) { - strbuf_trim_trailing_newline(lines[k]); - - if (skip_prefix(lines[k]->buf, "oid ", &sz)) { - if (get_oid_hex(sz, &oid)) - goto fail; - continue; - } - - if (skip_prefix(lines[k]->buf, "flags ", &sz)) { - umax_flags = strtoumax(sz, NULL, 10); - continue; - } - - if (skip_prefix(lines[k]->buf, "content ", &sz)) { - want_content = (*sz == 't'); - continue; - } - - BUG("unexpected line '%s' in OID request", lines[k]->buf); - } + req = (struct odb_over_ipc__get_oid__request *)command; // TODO Insert ODB lookup from in-memory database here. // TODO For now, just do the regular lookup. @@ -126,16 +100,16 @@ static int odb_ipc_cb__get_oid(struct my_odb_ipc_state *state, var_oi.sizep = &var_size; var_oi.disk_sizep = &var_disk_size; var_oi.delta_base_oid = &var_delta_base_oid; - if (want_content) + if (req->want_content) var_oi.contentp = &var_content; - ret = oid_object_info_extended(the_repository, &oid, &var_oi, - (unsigned)umax_flags); + ret = oid_object_info_extended(the_repository, &req->oid, &var_oi, + req->flags); if (ret) goto fail; strbuf_reset(&response); - strbuf_addf(&response, "oid %s\n", oid_to_hex(&oid)); + strbuf_addf(&response, "oid %s\n", oid_to_hex(&req->oid)); strbuf_addf(&response, "type %d\n", var_type); strbuf_addf(&response, "size %"PRIuMAX"\n", (uintmax_t)var_size); strbuf_addf(&response, "disk %"PRIuMAX"\n", (uintmax_t)var_disk_size); @@ -157,7 +131,7 @@ static int odb_ipc_cb__get_oid(struct my_odb_ipc_state *state, * then (if requested) immediately send the object contents. */ reply_cb(reply_data, response.buf, response.len + 1); - if (want_content) + if (req->want_content) reply_cb(reply_data, var_content, var_size); free(var_content); @@ -176,8 +150,6 @@ static int odb_ipc_cb__get_oid(struct my_odb_ipc_state *state, goto done; done: - if (lines) - strbuf_list_free(lines); strbuf_release(&response); return 0; } @@ -223,7 +195,7 @@ static int odb_ipc_cb(void *data, return SIMPLE_IPC_QUIT; } - if (!strncmp(command, "oid", 3)) { + if (!strcmp(command, "oid")) { /* * A client has requested that we lookup an object from the * ODB and send it to them. diff --git a/odb-over-ipc.c b/odb-over-ipc.c index 50c778979f8371..a9bbd8e57e5908 100644 --- a/odb-over-ipc.c +++ b/odb-over-ipc.c @@ -112,8 +112,8 @@ int odb_over_ipc__command(const char *command, size_t command_len, int odb_over_ipc__get_oid(struct repository *r, const struct object_id *oid, struct object_info *oi, unsigned flags) { - char hex_buf[GIT_MAX_HEXSZ + 1]; - struct strbuf cmd = STRBUF_INIT; + struct odb_over_ipc__get_oid__request req; + struct strbuf answer = STRBUF_INIT; struct strbuf headers = STRBUF_INIT; struct strbuf **lines = NULL; @@ -130,18 +130,13 @@ int odb_over_ipc__get_oid(struct repository *r, const struct object_id *oid, if (r != the_repository) // TODO not dealing with this return -1; - /* - * If we are going to the trouble to ask the daemon for information on - * the object, always get all of the optional fields. That is, don't - * worry with which fields within `oi` are populated on the request side. - */ - strbuf_addf(&cmd, "oid %s\n", oid_to_hex_r(hex_buf, oid)); - strbuf_addf(&cmd, "flags %"PRIuMAX"\n", (uintmax_t)flags); - strbuf_addf(&cmd, "content %c\n", (oi && oi->contentp ? 't' : 'f')); - - ret = odb_over_ipc__command(cmd.buf, cmd.len, &answer); + memset(&req, 0, sizeof(req)); + memcpy(req.key.key, "oid", 4); + oidcpy(&req.oid, oid); + req.flags = flags; + req.want_content = (oi && oi->contentp); - strbuf_release(&cmd); + ret = odb_over_ipc__command((const char *)&req, sizeof(req), &answer); if (ret) return ret; diff --git a/odb-over-ipc.h b/odb-over-ipc.h index a47d9eb5862559..268da2847eef9b 100644 --- a/odb-over-ipc.h +++ b/odb-over-ipc.h @@ -44,6 +44,19 @@ enum ipc_active_state odb_over_ipc__get_state(void); int odb_over_ipc__command(const char *command, size_t command_len, struct strbuf *answer); +struct odb_over_ipc__key +{ + char key[16]; +}; + +struct odb_over_ipc__get_oid__request +{ + struct odb_over_ipc__key key; + struct object_id oid; + unsigned flags; + unsigned want_content:1; +}; + /* * Connect to an existing `git odb--daemon` process and ask it for * an object. This is intended to be inserted into the client From b16b5fcf51be7202002f37e351754d3ce92cb523 Mon Sep 17 00:00:00 2001 From: Jeff Hostetler Date: Fri, 16 Apr 2021 09:44:36 -0400 Subject: [PATCH 24/34] odb-over-ipc: add binary response to get-oid Signed-off-by: Jeff Hostetler --- builtin/odb--daemon.c | 49 +++++++------------- odb-over-ipc.c | 102 +++++++++++------------------------------- odb-over-ipc.h | 12 +++++ 3 files changed, 54 insertions(+), 109 deletions(-) diff --git a/builtin/odb--daemon.c b/builtin/odb--daemon.c index b744b4d52f4bf6..fe2207dd8332ce 100644 --- a/builtin/odb--daemon.c +++ b/builtin/odb--daemon.c @@ -75,7 +75,7 @@ static int odb_ipc_cb__get_oid(struct my_odb_ipc_state *state, struct ipc_server_reply_data *reply_data) { struct odb_over_ipc__get_oid__request *req; - struct strbuf response = STRBUF_INIT; + struct odb_over_ipc__get_oid__response resp; if (command_len != sizeof(*req)) BUG("incorrect size for binary data"); @@ -86,20 +86,20 @@ static int odb_ipc_cb__get_oid(struct my_odb_ipc_state *state, // TODO For now, just do the regular lookup. { - enum object_type var_type = OBJ_BAD; - unsigned long var_size = 0; - off_t var_disk_size = 0; - struct object_id var_delta_base_oid; void *var_content = NULL; struct object_info var_oi = OBJECT_INFO_INIT; int ret; - oidclr(&var_delta_base_oid); + memset(&resp, 0, sizeof(resp)); + memcpy(resp.key.key, "oid", 4); + oidcpy(&resp.oid, &req->oid); + oidclr(&resp.delta_base_oid); - var_oi.typep = &var_type; - var_oi.sizep = &var_size; - var_oi.disk_sizep = &var_disk_size; - var_oi.delta_base_oid = &var_delta_base_oid; + var_oi.typep = &resp.type; + var_oi.sizep = &resp.size; + var_oi.disk_sizep = &resp.disk_size; + var_oi.delta_base_oid = &resp.delta_base_oid; + /* the client can compute `type_name` from `type`. */ if (req->want_content) var_oi.contentp = &var_content; @@ -108,49 +108,30 @@ static int odb_ipc_cb__get_oid(struct my_odb_ipc_state *state, if (ret) goto fail; - strbuf_reset(&response); - strbuf_addf(&response, "oid %s\n", oid_to_hex(&req->oid)); - strbuf_addf(&response, "type %d\n", var_type); - strbuf_addf(&response, "size %"PRIuMAX"\n", (uintmax_t)var_size); - strbuf_addf(&response, "disk %"PRIuMAX"\n", (uintmax_t)var_disk_size); - if (!is_null_oid(&var_delta_base_oid)) - strbuf_addf(&response, "delta %s\n", oid_to_hex(&var_delta_base_oid)); // TODO should we create a new fake whence value that we report to // TODO the client -- something like "ipc" to indicate that the // TODO client got it from us and therefore doesn't have the in-memory // TODO cache nor any of the packfile data loaded. The answer to this // TODO also affects the oi.u.packed fields. - strbuf_addf(&response, "whence %d\n", var_oi.whence); + resp.whence = var_oi.whence; // TODO decide if we care about oi.u.packed - // trace2_printf("oid--daemon: sending:\n%s", response.buf); - - /* - * Add one to the length of the headers to include the NUL and - * then (if requested) immediately send the object contents. - */ - reply_cb(reply_data, response.buf, response.len + 1); + reply_cb(reply_data, (const char *)&resp, sizeof(resp)); if (req->want_content) - reply_cb(reply_data, var_content, var_size); + reply_cb(reply_data, var_content, resp.size); free(var_content); } - goto done; + return 0; fail: /* * Send the client an error response to force it to do * the work itself. */ - strbuf_reset(&response); - strbuf_addstr(&response, "error"); - reply_cb(reply_data, response.buf, response.len + 1); - goto done; - -done: - strbuf_release(&response); + reply_cb(reply_data, "error", 6); return 0; } diff --git a/odb-over-ipc.c b/odb-over-ipc.c index a9bbd8e57e5908..b00794545bc32f 100644 --- a/odb-over-ipc.c +++ b/odb-over-ipc.c @@ -113,15 +113,11 @@ int odb_over_ipc__get_oid(struct repository *r, const struct object_id *oid, struct object_info *oi, unsigned flags) { struct odb_over_ipc__get_oid__request req; + struct odb_over_ipc__get_oid__response *resp; struct strbuf answer = STRBUF_INIT; - struct strbuf headers = STRBUF_INIT; - struct strbuf **lines = NULL; - const char *sz; - const char *ch_nul; const char *content; ssize_t content_len; - int k; int ret; if (is_daemon) @@ -153,83 +149,39 @@ int odb_over_ipc__get_oid(struct repository *r, const struct object_id *oid, goto done; } - /* Find the divider between the headers and the content. */ - ch_nul = strchr(answer.buf, '\0'); - content = ch_nul + 1; - /* The content_len is only defined if we asked for content. */ - content_len = &answer.buf[answer.len] - content; + if (answer.len < sizeof(*resp)) + BUG("incorrect size for binary data"); + resp = (struct odb_over_ipc__get_oid__response *)answer.buf; - /* - * Extract the portion before the divider into another string so that - * we can split / parse it by lines. - */ - strbuf_add(&headers, answer.buf, (ch_nul - answer.buf)); - - lines = strbuf_split_str(headers.buf, '\n', 0); - strbuf_release(&headers); - - for (k = 0; lines[k]; k++) { - strbuf_trim_trailing_newline(lines[k]); - - if (skip_prefix(lines[k]->buf, "oid ", &sz)) { - assert(!strcmp(sz, oid_to_hex(oid))); - continue; - } - - if (skip_prefix(lines[k]->buf, "type ", &sz)) { - enum object_type t = strtol(sz, NULL, 10); - if (oi->typep) - *(oi->typep) = t; - if (oi->type_name) - strbuf_addstr(oi->type_name, type_name(t)); - continue; - } - - if (skip_prefix(lines[k]->buf, "size ", &sz)) { - ssize_t size = strtoumax(sz, NULL, 10); - if (oi->sizep) - *(oi->sizep) = size; - if (oi->contentp && size != content_len) - BUG("observed content length does not match size"); - continue; - } + content = answer.buf + sizeof(*resp); + content_len = answer.len - sizeof(*resp); - if (skip_prefix(lines[k]->buf, "disk ", &sz)) { - if (oi->disk_sizep) - *(oi->disk_sizep) = strtoumax(sz, NULL, 10); - continue; - } - - // TODO do we really care about the delta-base ?? - if (skip_prefix(lines[k]->buf, "delta ", &sz)) { - if (oi->delta_base_oid) { - oidclr(oi->delta_base_oid); - if (get_oid_hex(sz, oi->delta_base_oid)) { - error("could not parse delta base in odb-over-ipc response"); - ret = -1; - goto done; - } - } - continue; - } - - if (skip_prefix(lines[k]->buf, "whence ", &sz)) { - oi->whence = strtol(sz, NULL, 10); - continue; - } - - // TODO The server does not send the contents of oi.u.packed. - // TODO Do we care? - - BUG("unexpected line '%s' in OID response", lines[k]->buf); + if (oidcmp(&resp->oid, oid)) { + // TODO Think about the _LOOKUP_REPLACE code here + BUG("received different OID"); } - if (oi->contentp) + if (oi->typep) + *(oi->typep) = resp->type; + if (oi->type_name) + strbuf_addstr(oi->type_name, type_name(resp->type)); + if (oi->sizep) + *(oi->sizep) = resp->size; + if (oi->disk_sizep) + *(oi->disk_sizep) = resp->disk_size; + if (oi->delta_base_oid) + oidcpy(oi->delta_base_oid, &resp->delta_base_oid); + oi->whence = resp->whence; + if (oi->contentp) { + if (content_len != resp->size) + BUG("observed content length does not match size"); *oi->contentp = xmemdupz(content, content_len); + } + + // TODO The server does not send the contents of oi.u.packed. + // TODO Do we care? done: - if (lines) - strbuf_list_free(lines); strbuf_release(&answer); return ret; } diff --git a/odb-over-ipc.h b/odb-over-ipc.h index 268da2847eef9b..68b26085f5c562 100644 --- a/odb-over-ipc.h +++ b/odb-over-ipc.h @@ -57,6 +57,18 @@ struct odb_over_ipc__get_oid__request unsigned want_content:1; }; +struct odb_over_ipc__get_oid__response +{ + struct odb_over_ipc__key key; + struct object_id oid; + struct object_id delta_base_oid; + off_t disk_size; + unsigned long size; + unsigned whence; /* see struct object_info */ + enum object_type type; + unsigned content_follows:1; +}; + /* * Connect to an existing `git odb--daemon` process and ask it for * an object. This is intended to be inserted into the client From 90a4379e6297b84876b4f2f37ffae5ce40987918 Mon Sep 17 00:00:00 2001 From: Jeff Hostetler Date: Fri, 16 Apr 2021 13:01:08 -0400 Subject: [PATCH 25/34] simple-ipc: fix Windows version for binary requests Signed-off-by: Jeff Hostetler --- compat/simple-ipc/ipc-win32.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/compat/simple-ipc/ipc-win32.c b/compat/simple-ipc/ipc-win32.c index 3d8432d6bd6850..a4d88861689633 100644 --- a/compat/simple-ipc/ipc-win32.c +++ b/compat/simple-ipc/ipc-win32.c @@ -204,7 +204,8 @@ void ipc_client_close_connection(struct ipc_client_connection *connection) int ipc_client_send_command_to_connection( struct ipc_client_connection *connection, - const char *message, struct strbuf *answer) + const char *message, size_t message_len, + struct strbuf *answer) { int ret = 0; @@ -212,7 +213,7 @@ int ipc_client_send_command_to_connection( trace2_region_enter("ipc-client", "send-command", NULL); - if (write_packetized_from_buf_no_flush(message, strlen(message), + if (write_packetized_from_buf_no_flush(message, message_len, connection->fd) < 0 || packet_flush_gently(connection->fd) < 0) { ret = error(_("could not send IPC command")); @@ -235,7 +236,8 @@ int ipc_client_send_command_to_connection( int ipc_client_send_command(const char *path, const struct ipc_client_connect_options *options, - const char *message, struct strbuf *response) + const char *message, size_t message_len, + struct strbuf *response) { int ret = -1; enum ipc_active_state state; @@ -246,7 +248,9 @@ int ipc_client_send_command(const char *path, if (state != IPC_STATE__LISTENING) return ret; - ret = ipc_client_send_command_to_connection(connection, message, response); + ret = ipc_client_send_command_to_connection(connection, + message, message_len, + response); ipc_client_close_connection(connection); @@ -456,7 +460,7 @@ static int do_io(struct ipc_server_thread_data *server_thread_data) if (ret >= 0) { ret = server_thread_data->server_data->application_cb( server_thread_data->server_data->application_data, - buf.buf, do_io_reply_callback, &reply_data); + buf.buf, buf.len, do_io_reply_callback, &reply_data); packet_flush_gently(reply_data.fd); From 2727739d5ad16c0d138df532047d42021cf32863 Mon Sep 17 00:00:00 2001 From: Jeff Hostetler Date: Fri, 16 Apr 2021 13:35:00 -0400 Subject: [PATCH 26/34] odb-over-ipc: set initial size of receive buffer to avoid reallocs Signed-off-by: Jeff Hostetler --- odb-over-ipc.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/odb-over-ipc.c b/odb-over-ipc.c index b00794545bc32f..9ede4cf0148722 100644 --- a/odb-over-ipc.c +++ b/odb-over-ipc.c @@ -109,6 +109,17 @@ int odb_over_ipc__command(const char *command, size_t command_len, return 0; } +/* + * When we request an object from the daemon over IPC, the response + * contains both the response-header and the content of the object in + * one buffer. We want to pre-alloc the strbuf-buffer big enough to + * avoid multiple realloc's when we are receiving large blobs. + * + * IPC uses pkt-line and will handle the chunking and reassembly, so + * we are not limited to LARGE_PACKET_DATA_MAX buffers. + */ +#define LARGE_ANSWER (64 * 1024) + int odb_over_ipc__get_oid(struct repository *r, const struct object_id *oid, struct object_info *oi, unsigned flags) { @@ -132,6 +143,8 @@ int odb_over_ipc__get_oid(struct repository *r, const struct object_id *oid, req.flags = flags; req.want_content = (oi && oi->contentp); + strbuf_init(&answer, LARGE_ANSWER); + ret = odb_over_ipc__command((const char *)&req, sizeof(req), &answer); if (ret) return ret; From f90cecb571fedccd1eb94a070d97f3da8e0f6330 Mon Sep 17 00:00:00 2001 From: Jeff Hostetler Date: Fri, 16 Apr 2021 14:29:07 -0400 Subject: [PATCH 27/34] odb--daemon: add oidmap cache Signed-off-by: Jeff Hostetler --- builtin/odb--daemon.c | 62 ++++++++++++++++++++++++++++--------------- odb-over-ipc.h | 1 - 2 files changed, 40 insertions(+), 23 deletions(-) diff --git a/builtin/odb--daemon.c b/builtin/odb--daemon.c index fe2207dd8332ce..615f9daca8b736 100644 --- a/builtin/odb--daemon.c +++ b/builtin/odb--daemon.c @@ -1,6 +1,7 @@ #include "builtin.h" #include "config.h" #include "object-store.h" +#include "oidmap.h" #include "parse-options.h" #include "simple-ipc.h" #include "strbuf.h" @@ -69,39 +70,53 @@ struct my_odb_ipc_state static struct my_odb_ipc_state my_state; +struct my_oidmap_entry +{ + struct oidmap_entry entry; + struct odb_over_ipc__get_oid__response resp; + char *content; +}; + +static struct oidmap my_oidmap = OIDMAP_INIT; + + static int odb_ipc_cb__get_oid(struct my_odb_ipc_state *state, const char *command, size_t command_len, ipc_server_reply_cb *reply_cb, struct ipc_server_reply_data *reply_data) { struct odb_over_ipc__get_oid__request *req; - struct odb_over_ipc__get_oid__response resp; + struct my_oidmap_entry *e; if (command_len != sizeof(*req)) BUG("incorrect size for binary data"); req = (struct odb_over_ipc__get_oid__request *)command; - // TODO Insert ODB lookup from in-memory database here. - // TODO For now, just do the regular lookup. - - { - void *var_content = NULL; + e = oidmap_get(&my_oidmap, &req->oid); +// { +// char hexbuf[200]; +// trace2_printf("get-oid: %s %s", oid_to_hex_r(hexbuf, &req->oid), +// (e ? "found" : "not-found")); +// } + if (!e) { struct object_info var_oi = OBJECT_INFO_INIT; int ret; - memset(&resp, 0, sizeof(resp)); - memcpy(resp.key.key, "oid", 4); - oidcpy(&resp.oid, &req->oid); - oidclr(&resp.delta_base_oid); + e = xcalloc(1, sizeof(*e)); + + memcpy(e->resp.key.key, "oid", 4); + oidcpy(&e->resp.oid, &req->oid); + oidclr(&e->resp.delta_base_oid); - var_oi.typep = &resp.type; - var_oi.sizep = &resp.size; - var_oi.disk_sizep = &resp.disk_size; - var_oi.delta_base_oid = &resp.delta_base_oid; + var_oi.typep = &e->resp.type; + var_oi.sizep = &e->resp.size; + var_oi.disk_sizep = &e->resp.disk_size; + var_oi.delta_base_oid = &e->resp.delta_base_oid; /* the client can compute `type_name` from `type`. */ - if (req->want_content) - var_oi.contentp = &var_content; + + /* always fetch the content so that our oidmap cache is complete. */ + var_oi.contentp = &e->content; ret = oid_object_info_extended(the_repository, &req->oid, &var_oi, req->flags); @@ -113,17 +128,18 @@ static int odb_ipc_cb__get_oid(struct my_odb_ipc_state *state, // TODO client got it from us and therefore doesn't have the in-memory // TODO cache nor any of the packfile data loaded. The answer to this // TODO also affects the oi.u.packed fields. - resp.whence = var_oi.whence; + e->resp.whence = var_oi.whence; // TODO decide if we care about oi.u.packed - reply_cb(reply_data, (const char *)&resp, sizeof(resp)); - if (req->want_content) - reply_cb(reply_data, var_content, resp.size); - - free(var_content); + oidcpy(&e->entry.oid, &req->oid); + oidmap_put(&my_oidmap, e); } + reply_cb(reply_data, (const char *)&e->resp, sizeof(e->resp)); + if (req->want_content) + reply_cb(reply_data, e->content, e->resp.size); + return 0; fail: @@ -226,6 +242,8 @@ static int do_run_daemon(void) odb_over_ipc__set_is_daemon(); + oidmap_init(&my_oidmap, 1024 * 1024); + // TODO Create mutexes and locks // // TODO Load up the packfiles diff --git a/odb-over-ipc.h b/odb-over-ipc.h index 68b26085f5c562..98524dd1c5015a 100644 --- a/odb-over-ipc.h +++ b/odb-over-ipc.h @@ -66,7 +66,6 @@ struct odb_over_ipc__get_oid__response unsigned long size; unsigned whence; /* see struct object_info */ enum object_type type; - unsigned content_follows:1; }; /* From 41da0282095ed7c5fdbd61e63c18da8d11333759 Mon Sep 17 00:00:00 2001 From: Jeff Hostetler Date: Fri, 16 Apr 2021 15:04:44 -0400 Subject: [PATCH 28/34] fixup cast in oidmap code Signed-off-by: Jeff Hostetler --- builtin/odb--daemon.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/odb--daemon.c b/builtin/odb--daemon.c index 615f9daca8b736..88af7499fd09e0 100644 --- a/builtin/odb--daemon.c +++ b/builtin/odb--daemon.c @@ -116,7 +116,7 @@ static int odb_ipc_cb__get_oid(struct my_odb_ipc_state *state, /* the client can compute `type_name` from `type`. */ /* always fetch the content so that our oidmap cache is complete. */ - var_oi.contentp = &e->content; + var_oi.contentp = (void **)&e->content; ret = oid_object_info_extended(the_repository, &req->oid, &var_oi, req->flags); From e5ea314395541daaf54d550ae2247ae639812ed0 Mon Sep 17 00:00:00 2001 From: Jeff Hostetler Date: Sun, 18 Apr 2021 10:36:28 -0400 Subject: [PATCH 29/34] odb-over-ipc: add core.useOdbOverIpc config setting Signed-off-by: Jeff Hostetler --- cache.h | 2 ++ config.c | 5 +++++ environment.c | 2 ++ odb-over-ipc.c | 3 +++ 4 files changed, 12 insertions(+) diff --git a/cache.h b/cache.h index bbf4e601c4beec..858aa2f2dacce3 100644 --- a/cache.h +++ b/cache.h @@ -965,6 +965,8 @@ extern const char *core_fsmonitor; extern int core_apply_sparse_checkout; extern int core_sparse_checkout_cone; +extern int core_use_odb_over_ipc; + /* * Include broken refs in all ref iterations, which will * generally choke dangerous operations rather than letting diff --git a/config.c b/config.c index d47d70dc9c643e..4d263e26279643 100644 --- a/config.c +++ b/config.c @@ -1572,6 +1572,11 @@ static int git_default_core_config(const char *var, const char *value, void *cb) return 0; } + if (!strcmp(var, "core.useodboveripc")) { + core_use_odb_over_ipc = git_config_bool(var, value); + return 0; + } + /* Add other config variables here and to Documentation/config.txt. */ return platform_core_config(var, value, cb); } diff --git a/environment.c b/environment.c index 2f27008424a074..c41eea12f2170b 100644 --- a/environment.c +++ b/environment.c @@ -110,6 +110,8 @@ static char *git_namespace; static char *super_prefix; +int core_use_odb_over_ipc; + /* * Repository-local GIT_* environment variables; see cache.h for details. */ diff --git a/odb-over-ipc.c b/odb-over-ipc.c index 9ede4cf0148722..f5347029425f36 100644 --- a/odb-over-ipc.c +++ b/odb-over-ipc.c @@ -134,6 +134,9 @@ int odb_over_ipc__get_oid(struct repository *r, const struct object_id *oid, if (is_daemon) return -1; + if (!core_use_odb_over_ipc) + return -1; + if (r != the_repository) // TODO not dealing with this return -1; From c67771c89c5dff676a5a84c4f1f052667a8315f4 Mon Sep 17 00:00:00 2001 From: Jeff Hostetler Date: Mon, 19 Apr 2021 02:32:03 -0400 Subject: [PATCH 30/34] odb--daemon: only fetch content from disk if/when requested Signed-off-by: Jeff Hostetler --- builtin/odb--daemon.c | 68 ++++++++++++++++++++++++++++--------------- 1 file changed, 44 insertions(+), 24 deletions(-) diff --git a/builtin/odb--daemon.c b/builtin/odb--daemon.c index 88af7499fd09e0..7e540388600887 100644 --- a/builtin/odb--daemon.c +++ b/builtin/odb--daemon.c @@ -79,6 +79,37 @@ struct my_oidmap_entry static struct oidmap my_oidmap = OIDMAP_INIT; +static int do_oid_lookup(struct my_oidmap_entry *e, + struct odb_over_ipc__get_oid__request *req) +{ + struct object_info var_oi = OBJECT_INFO_INIT; + int ret; + + var_oi.typep = &e->resp.type; + var_oi.sizep = &e->resp.size; + var_oi.disk_sizep = &e->resp.disk_size; + var_oi.delta_base_oid = &e->resp.delta_base_oid; + /* the client can compute `type_name` from `type`. */ + + if (req->want_content) + var_oi.contentp = (void **)&e->content; + + ret = oid_object_info_extended(the_repository, &req->oid, &var_oi, + req->flags); + if (ret) + return -1; + + // TODO should we create a new fake whence value that we report to + // TODO the client -- something like "ipc" to indicate that the + // TODO client got it from us and therefore doesn't have the in-memory + // TODO cache nor any of the packfile data loaded. The answer to this + // TODO also affects the oi.u.packed fields. + e->resp.whence = var_oi.whence; + + // TODO decide if we care about oi.u.packed + + return 0; +} static int odb_ipc_cb__get_oid(struct my_odb_ipc_state *state, const char *command, size_t command_len, @@ -99,39 +130,28 @@ static int odb_ipc_cb__get_oid(struct my_odb_ipc_state *state, // trace2_printf("get-oid: %s %s", oid_to_hex_r(hexbuf, &req->oid), // (e ? "found" : "not-found")); // } - if (!e) { - struct object_info var_oi = OBJECT_INFO_INIT; - int ret; + if (e && req->want_content && !e->content) { + /* + * We have a cached entry from a previous request where + * the client did not want the content, but this client + * does want the content. So repeat the lookup and ammend + * our cache entry. + */ + if (do_oid_lookup(e, req)) + goto fail; + } + + if (!e) { e = xcalloc(1, sizeof(*e)); memcpy(e->resp.key.key, "oid", 4); oidcpy(&e->resp.oid, &req->oid); oidclr(&e->resp.delta_base_oid); - var_oi.typep = &e->resp.type; - var_oi.sizep = &e->resp.size; - var_oi.disk_sizep = &e->resp.disk_size; - var_oi.delta_base_oid = &e->resp.delta_base_oid; - /* the client can compute `type_name` from `type`. */ - - /* always fetch the content so that our oidmap cache is complete. */ - var_oi.contentp = (void **)&e->content; - - ret = oid_object_info_extended(the_repository, &req->oid, &var_oi, - req->flags); - if (ret) + if (do_oid_lookup(e, req)) goto fail; - // TODO should we create a new fake whence value that we report to - // TODO the client -- something like "ipc" to indicate that the - // TODO client got it from us and therefore doesn't have the in-memory - // TODO cache nor any of the packfile data loaded. The answer to this - // TODO also affects the oi.u.packed fields. - e->resp.whence = var_oi.whence; - - // TODO decide if we care about oi.u.packed - oidcpy(&e->entry.oid, &req->oid); oidmap_put(&my_oidmap, e); } From 3d0def341be9d6945324da09e546a1b7530f760a Mon Sep 17 00:00:00 2001 From: Jeff Hostetler Date: Mon, 19 Apr 2021 04:00:06 -0400 Subject: [PATCH 31/34] p7100: add perf test for odb--daemon Signed-off-by: Jeff Hostetler --- t/perf/p7100-odb-daemon.sh | 104 +++++++++++++++++++++++++++++++++++++ 1 file changed, 104 insertions(+) create mode 100755 t/perf/p7100-odb-daemon.sh diff --git a/t/perf/p7100-odb-daemon.sh b/t/perf/p7100-odb-daemon.sh new file mode 100755 index 00000000000000..80abc4dfb7b53f --- /dev/null +++ b/t/perf/p7100-odb-daemon.sh @@ -0,0 +1,104 @@ +#!/bin/sh + +test_description="Test git odb--daemon" + +. ./perf-lib.sh + +test_perf_large_repo +test_checkout_worktree + +test_odb_daemon_suite() { + test_perf "status ($ENABLE)" ' + git -c core.useodboveripc=$ENABLE status >/dev/null + ' + test_perf "status ($ENABLE)" ' + git -c core.useodboveripc=$ENABLE status >/dev/null + ' + test_perf "status ($ENABLE)" ' + git -c core.useodboveripc=$ENABLE status >/dev/null + ' + test_perf "status ($ENABLE)" ' + git -c core.useodboveripc=$ENABLE status >/dev/null + ' + + test_perf "diff ($ENABLE)" ' + git -c core.useodboveripc=$ENABLE diff HEAD~1 >/dev/null + ' + test_perf "diff ($ENABLE)" ' + git -c core.useodboveripc=$ENABLE diff HEAD~1 >/dev/null + ' + test_perf "diff ($ENABLE)" ' + git -c core.useodboveripc=$ENABLE diff HEAD~1 >/dev/null + ' + test_perf "diff ($ENABLE)" ' + git -c core.useodboveripc=$ENABLE diff HEAD~1 >/dev/null + ' + + test_perf "log -200 ($ENABLE)" ' + git -c core.useodboveripc=$ENABLE log -200 >/dev/null + ' + test_perf "log -200 ($ENABLE)" ' + git -c core.useodboveripc=$ENABLE log -200 >/dev/null + ' + test_perf "log -200 ($ENABLE)" ' + git -c core.useodboveripc=$ENABLE log -200 >/dev/null + ' + test_perf "log -200 ($ENABLE)" ' + git -c core.useodboveripc=$ENABLE log -200 >/dev/null + ' + + test_perf "rev-list HEAD^{tree} ($ENABLE)" ' + git -c core.useodboveripc=$ENABLE rev-list --objects HEAD^{tree} >/dev/null + ' + test_perf "rev-list HEAD^{tree} ($ENABLE)" ' + git -c core.useodboveripc=$ENABLE rev-list --objects HEAD^{tree} >/dev/null + ' + test_perf "rev-list HEAD^{tree} ($ENABLE)" ' + git -c core.useodboveripc=$ENABLE rev-list --objects HEAD^{tree} >/dev/null + ' + test_perf "rev-list HEAD^{tree} ($ENABLE)" ' + git -c core.useodboveripc=$ENABLE rev-list --objects HEAD^{tree} >/dev/null + ' + + test_perf "rev-list HEAD ($ENABLE)" ' + git -c core.useodboveripc=$ENABLE rev-list --objects HEAD >/dev/null + ' + test_perf "rev-list HEAD ($ENABLE)" ' + git -c core.useodboveripc=$ENABLE rev-list --objects HEAD >/dev/null + ' + test_perf "rev-list HEAD ($ENABLE)" ' + git -c core.useodboveripc=$ENABLE rev-list --objects HEAD >/dev/null + ' + test_perf "rev-list HEAD ($ENABLE)" ' + git -c core.useodboveripc=$ENABLE rev-list --objects HEAD >/dev/null + ' + + test_perf "rev-list --all ($ENABLE)" ' + git -c core.useodboveripc=$ENABLE rev-list --objects --all >/dev/null + ' + test_perf "rev-list --all ($ENABLE)" ' + git -c core.useodboveripc=$ENABLE rev-list --objects --all >/dev/null + ' + test_perf "rev-list --all ($ENABLE)" ' + git -c core.useodboveripc=$ENABLE rev-list --objects --all >/dev/null + ' + test_perf "rev-list --all ($ENABLE)" ' + git -c core.useodboveripc=$ENABLE rev-list --objects --all >/dev/null + ' +} + +ENABLE=0 +test_odb_daemon_suite + +test_expect_success "Start odb--daemon" ' + (git odb--daemon --run &) +' + +ENABLE=1 +test_odb_daemon_suite + +test_expect_success "Stop odb--daemon" ' + git odb--daemon --stop +' + +test_done From a0f0ae30f3107b5a8464322124441016d49add20 Mon Sep 17 00:00:00 2001 From: Jeff Hostetler Date: Mon, 19 Apr 2021 04:21:06 -0400 Subject: [PATCH 32/34] odb--daemon: disable resize on oidmap Signed-off-by: Jeff Hostetler --- builtin/odb--daemon.c | 1 + 1 file changed, 1 insertion(+) diff --git a/builtin/odb--daemon.c b/builtin/odb--daemon.c index 7e540388600887..6b2358f6cad524 100644 --- a/builtin/odb--daemon.c +++ b/builtin/odb--daemon.c @@ -263,6 +263,7 @@ static int do_run_daemon(void) odb_over_ipc__set_is_daemon(); oidmap_init(&my_oidmap, 1024 * 1024); + hashmap_disable_item_counting(&my_oidmap.map); // TODO Create mutexes and locks // From d69e1400e5c5abbfeb4bdafda9a518fe8b5fcbeb Mon Sep 17 00:00:00 2001 From: Jeff Hostetler Date: Tue, 20 Apr 2021 11:03:06 -0400 Subject: [PATCH 33/34] odb-over-ipc: fix coccinelle suggested change Signed-off-by: Jeff Hostetler --- odb-over-ipc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/odb-over-ipc.c b/odb-over-ipc.c index f5347029425f36..f9788920c49b81 100644 --- a/odb-over-ipc.c +++ b/odb-over-ipc.c @@ -172,7 +172,7 @@ int odb_over_ipc__get_oid(struct repository *r, const struct object_id *oid, content = answer.buf + sizeof(*resp); content_len = answer.len - sizeof(*resp); - if (oidcmp(&resp->oid, oid)) { + if (!oideq(&resp->oid, oid)) { // TODO Think about the _LOOKUP_REPLACE code here BUG("received different OID"); } From eb1d228f25e02fde593a19dae40aa78b9d5c08aa Mon Sep 17 00:00:00 2001 From: Jeff Hostetler Date: Tue, 20 Apr 2021 11:07:25 -0400 Subject: [PATCH 34/34] hackathon: add results to summarize my week of hacking Signed-off-by: Jeff Hostetler --- hackathon-notes-odb-over-ipc.md | 94 +++++++++++++++++++++++++++++++++ 1 file changed, 94 insertions(+) create mode 100644 hackathon-notes-odb-over-ipc.md diff --git a/hackathon-notes-odb-over-ipc.md b/hackathon-notes-odb-over-ipc.md new file mode 100644 index 00000000000000..d1f6b188c81199 --- /dev/null +++ b/hackathon-notes-odb-over-ipc.md @@ -0,0 +1,94 @@ +# Hackathon Results + +(_I'm going to write up my results here in MD and attach it to my notes, rather than creating a separate slide deck that will just get lost._) + +## Title Page + +Hackathon Project: Git ODB over IPC +@jeffhostetler, Git-Client Team +April 12-16, 2021 + +https://github.com/jeffhostetler/git/pull/12 + +Based upon a suggestion from @derrickstolee + +## Questions + +1. Can we improve the performance of local Git commands with a local object database (ODB) daemon? + - All object lookups are modified to ask the daemon rather than directly accessing the disk. + - The daemon responds to client object requests, accesses the disk, and provides advanced cacheing. +2. Can we build upon the "Simple IPC" layer and thread pool mechanism created for FSMonitor? + +## ODB Background + +Individual objects (commits, trees, blobs) are stored in a highly-compacted and storage-efficient set of files on disk: + +1. Shared packfiles (`*.pack`, `*.idx`, and `*.midx`) files in one or more Git Alternates. +2. Private packfiles (`*.pack`, `*.idx`, and `*.midx`) in `.git/objects/pack/`. +3. Loose objects (`.git/objects/XX/*`) + +Packfiles use compression and delta-chains to reduce disk space (and network downloads), but it can be expensive to find and regenerate an individual object. + +## Opportunities for Perf + +Each Git command needs to access the ODB to lookup individual objects. Conceptually, there are four steps required: + +1. Scan the various shared and private packfile directories for `*.idx` and `*.midx` indexes and construct a list of in-memory dictionaries of object locations within packfiles. (_While this list may be lazily created, it is still a startup cost in each Git process._) +2. When fetching an individual object, `mmap` packfile any fragments necessary to reconstruct the object. (_These fragments are mapped with generous address rounding on both ends, so we usually have good overlap with future object requests. However, `mmap` isn't free and we do have to pay for page-faults. And again, this memory mapping has to happen for each Git process._) +3. Within a packfile, an object is `zlib` compressed and it must be zlib-inflated to regenerate it. This is expensive. (_When I was working on parallel checkout, I noticed that a significant percentage of the time was spent in zlib._) +4. Within a packfile, an object may be part of a delta-chain and may require a sequence of the [2] and [3] steps to regenerate the "data-base" before the current object may be regenerated. + +Each of these steps gives us a "perf opportunity". + +1. An ODB daemon could manage (preserve between clients) the list of index dictionaries. (_Extra credit if it also watches the file system for created/deleted .idx files and automatically adapts._) +2. An ODB daemon could similarly manage the packfile memory mappings. +3. An ODB daemon could cache regenerated objects and respond without touching the disk, re-inflating, or re-de-delta-ing it. + +## The State of the HACK + +1. Create `git odb--daemon` + 1.1. This is a long-running thread pool-based service/daemon modeled on my FSMonitor daemon. + 1.2. It speaks IPC over Windows Named Pipes or Unix domain sockets. + 1.3. It caches all fetched objects and can re-serve them without touching the disk. + +2. IPC upgrades + 2.1. Keep Alive: I upgraded the "Simple IPC" code used in FSMonitor to support multiple requests/responses on the same connection. (FSMonitor only needed a simple single request/response model.) + 2.2. Binary Mode: I also upgraded it to allow binary transfers. + +3. In all Git commands: I intercept all object lookups and route to the ODB daemon instead of directly touching the disk. + +## Limitations of the HACK + +_This is a hackathon project, so I did cut some corners._ + +1. On the client side, I intercepted all object lookups and route them to the ODB daemon and bypassed any client-side cacheing available. A proper version would work with the existing in-process cache. + +2. On the daemon side, I added a simple `oidmap` cache to remember and re-serve previously fetched objects. No attempt was made to manage this cache -- it will just consume all memory in the daemon process. + +3. On the daemon side, I do have a thread pool to service concurrent clients, but I didn't bother to make the `oidmap` cache thread safe or deal with any thread issues with in the existing ODB code. + +4. The daemon is fairly limited and just responds to object requests. There is no packfile management or pre-loading. + +## Demo Results (using git.git) + +| | r1 | r2 | r3 | r4 | **avg** | r1 | r2 | r3 | r4 | **avg** | +/- | +| --- | -- | -- | -- | -- | --- | -- | -- | -- | -- | --- | --- | +| [1] | 0.07 | 0.07 | 0.07 | 0.07 | **0.07** | 0.08 | 0.08 | 0.08 | 0.10 | **0.085** | +0.015 | +| [2] | 0.07 | 0.07 | 0.07 | 0.07 | **0.07** | 0.08 | 0.07 | 0.07 | 0.07 | **0.073** | +0.003 | +| [3] | 0.05 | 0.04 | 0.04 | 0.04 | **0.043** | 0.04 | 0.05 | 0.05 | 0.05 | **0.048** | +0.005 | +| [4] | 0.06 | 0.06 | 0.05 | 0.06 | **0.058** | 0.07 | 0.07 | 0.06 | 0.06 | **0.065** | +0.007 | +| [5] | 7.93 | 7.85 | 7.92 | 8.07 | **7.943** | 7.88 | 7.94 | 8.06 | 8.56 | **8.110** | +0.167 | +| [6] | 9.84 | 9.80 | 9.70 | 10.97 | **10.078** | 9.89 | 9.69 | 9.82 | 9.70 | **9.775** | -0.303 | + +[1] `git status >/dev/null` +[2] `git diff HEAD~1 >/dev/null` +[3] `git log -200 >/dev/null` +[4] `git rev-list --objects HEAD^{tree} >/dev/null` +[5] `git rev-list --objects HEAD >/dev/null` +[6] `git rev-list --objects --all >/dev/null` + +## Results + +Perf-wise, the before and after numbers are about equal. More testing is needed to see where time is being spent and where we can optimize, but this (relatively stupid) daemon demonstrates that an IPC-based solution is possible. + +This lets us think about creating a smarter daemon to manage the ODB and access to it.