fetch-pack: add tracing for negotiation rounds

Currently, negotiation for V0/V1/V2 fetch have trace2 regions covering
the entire negotiation process. However, we'd like additional data, such
as timing for each round of negotiation or the number of "haves" in each
round. Additionally, "independent negotiation" (AKA push negotiation)
has no tracing at all. Having this data would allow us to compare the
performance of the various negotation implementations, and to debug
unexpectedly slow fetch & push sessions.

Add per-round trace2 regions for all negotiation implementations (V0+V1,
V2, and independent negotiation), as well as an overall region for
independent negotiation. Add trace2 data logging for the number of haves
and "in vain" objects for each round, and for the total number of rounds
once negotiation completes.  Finally, add a few checks into various
tests to verify that the number of rounds is logged as expected.

Signed-off-by: Josh Steadmon <steadmon@google.com>
Acked-by: Jeff Hostetler <jeffhost@microsoft.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/fetch-pack.c b/fetch-pack.c
index d35be4177b107768d6af30c5260f16ddc8cbb5a0..a1a508ee72e2f6f58ed4b61430bf2a6d61c00651 100644 (file)
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -323,6 +323,7 @@ static int find_common(struct fetch_negotiator *negotiator,
 {
        int fetching;
        int count = 0, flushes = 0, flush_at = INITIAL_FLUSH, retval;
+       int negotiation_round = 0, haves = 0;
        const struct object_id *oid;
        unsigned in_vain = 0;
        int got_continue = 0;
@@ -461,9 +462,19 @@ static int find_common(struct fetch_negotiator *negotiator,
                packet_buf_write(&req_buf, "have %s\n", oid_to_hex(oid));
                print_verbose(args, "have %s", oid_to_hex(oid));
                in_vain++;
+               haves++;
                if (flush_at <= ++count) {
                        int ack;
 
+                       negotiation_round++;
+                       trace2_region_enter_printf("negotiation_v0_v1", "round",
+                                                  the_repository, "%d",
+                                                  negotiation_round);
+                       trace2_data_intmax("negotiation_v0_v1", the_repository,
+                                          "haves_added", haves);
+                       trace2_data_intmax("negotiation_v0_v1", the_repository,
+                                          "in_vain", in_vain);
+                       haves = 0;
                        packet_buf_flush(&req_buf);
                        send_request(args, fd[1], &req_buf);
                        strbuf_setlen(&req_buf, state_len);
@@ -485,6 +496,9 @@ static int find_common(struct fetch_negotiator *negotiator,
                                                      ack, oid_to_hex(result_oid));
                                switch (ack) {
                                case ACK:
+                                       trace2_region_leave_printf("negotiation_v0_v1", "round",
+                                                                  the_repository, "%d",
+                                                                  negotiation_round);
                                        flushes = 0;
                                        multi_ack = 0;
                                        retval = 0;
@@ -510,6 +524,7 @@ static int find_common(struct fetch_negotiator *negotiator,
                                                const char *hex = oid_to_hex(result_oid);
                                                packet_buf_write(&req_buf, "have %s\n", hex);
                                                state_len = req_buf.len;
+                                               haves++;
                                                /*
                                                 * Reset in_vain because an ack
                                                 * for this commit has not been
@@ -528,6 +543,9 @@ static int find_common(struct fetch_negotiator *negotiator,
                                }
                        } while (ack);
                        flushes--;
+                       trace2_region_leave_printf("negotiation_v0_v1", "round",
+                                                  the_repository, "%d",
+                                                  negotiation_round);
                        if (got_continue && MAX_IN_VAIN < in_vain) {
                                print_verbose(args, _("giving up"));
                                break; /* give up */
@@ -538,6 +556,8 @@ static int find_common(struct fetch_negotiator *negotiator,
        }
 done:
        trace2_region_leave("fetch-pack", "negotiation_v0_v1", the_repository);
+       trace2_data_intmax("negotiation_v0_v1", the_repository, "total_rounds",
+                          negotiation_round);
        if (!got_ready || !no_done) {
                packet_buf_write(&req_buf, "done\n");
                send_request(args, fd[1], &req_buf);
@@ -1381,6 +1401,8 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,
 
        haves_added = add_haves(negotiator, &req_buf, haves_to_send);
        *in_vain += haves_added;
+       trace2_data_intmax("negotiation_v2", the_repository, "haves_added", haves_added);
+       trace2_data_intmax("negotiation_v2", the_repository, "in_vain", *in_vain);
        if (!haves_added || (seen_ack && *in_vain >= MAX_IN_VAIN)) {
                /* Send Done */
                packet_buf_write(&req_buf, "done\n");
@@ -1623,6 +1645,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
        struct oidset common = OIDSET_INIT;
        struct packet_reader reader;
        int in_vain = 0, negotiation_started = 0;
+       int negotiation_round = 0;
        int haves_to_send = INITIAL_FLUSH;
        struct fetch_negotiator negotiator_alloc;
        struct fetch_negotiator *negotiator;
@@ -1679,12 +1702,20 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
                                                    "negotiation_v2",
                                                    the_repository);
                        }
+                       negotiation_round++;
+                       trace2_region_enter_printf("negotiation_v2", "round",
+                                                  the_repository, "%d",
+                                                  negotiation_round);
                        if (send_fetch_request(negotiator, fd[1], args, ref,
                                               &common,
                                               &haves_to_send, &in_vain,
                                               reader.use_sideband,
-                                              seen_ack))
+                                              seen_ack)) {
+                               trace2_region_leave_printf("negotiation_v2", "round",
+                                                          the_repository, "%d",
+                                                          negotiation_round);
                                state = FETCH_GET_PACK;
+                       }
                        else
                                state = FETCH_PROCESS_ACKS;
                        break;
@@ -1697,6 +1728,9 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
                                seen_ack = 1;
                                oidset_insert(&common, &common_oid);
                        }
+                       trace2_region_leave_printf("negotiation_v2", "round",
+                                                  the_repository, "%d",
+                                                  negotiation_round);
                        if (received_ready) {
                                /*
                                 * Don't check for response delimiter; get_pack() will
@@ -1712,6 +1746,8 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
                        trace2_region_leave("fetch-pack",
                                            "negotiation_v2",
                                            the_repository);
+                       trace2_data_intmax("negotiation_v2", the_repository,
+                                          "total_rounds", negotiation_round);
                        /* Check for shallow-info section */
                        if (process_section_header(&reader, "shallow-info", 1))
                                receive_shallow_info(args, &reader, shallows, si);
@@ -2091,6 +2127,7 @@ void negotiate_using_fetch(const struct oid_array *negotiation_tips,
        int in_vain = 0;
        int seen_ack = 0;
        int last_iteration = 0;
+       int negotiation_round = 0;
        timestamp_t min_generation = GENERATION_NUMBER_INFINITY;
 
        fetch_negotiator_init(the_repository, &negotiator);
@@ -2104,11 +2141,17 @@ void negotiate_using_fetch(const struct oid_array *negotiation_tips,
                           add_to_object_array,
                           &nt_object_array);
 
+       trace2_region_enter("fetch-pack", "negotiate_using_fetch", the_repository);
        while (!last_iteration) {
                int haves_added;
                struct object_id common_oid;
                int received_ready = 0;
 
+               negotiation_round++;
+
+               trace2_region_enter_printf("negotiate_using_fetch", "round",
+                                          the_repository, "%d",
+                                          negotiation_round);
                strbuf_reset(&req_buf);
                write_fetch_command_and_capabilities(&req_buf, server_options);
 
@@ -2119,6 +2162,11 @@ void negotiate_using_fetch(const struct oid_array *negotiation_tips,
                if (!haves_added || (seen_ack && in_vain >= MAX_IN_VAIN))
                        last_iteration = 1;
 
+               trace2_data_intmax("negotiate_using_fetch", the_repository,
+                                  "haves_added", haves_added);
+               trace2_data_intmax("negotiate_using_fetch", the_repository,
+                                  "in_vain", in_vain);
+
                /* Send request */
                packet_buf_flush(&req_buf);
                if (write_in_full(fd[1], req_buf.buf, req_buf.len) < 0)
@@ -2151,7 +2199,13 @@ void negotiate_using_fetch(const struct oid_array *negotiation_tips,
                                                 REACH_SCRATCH, 0,
                                                 min_generation))
                        last_iteration = 1;
+               trace2_region_leave_printf("negotiation", "round",
+                                          the_repository, "%d",
+                                          negotiation_round);
        }
+       trace2_region_enter("fetch-pack", "negotiate_using_fetch", the_repository);
+       trace2_data_intmax("negotiate_using_fetch", the_repository,
+                          "total_rounds", negotiation_round);
        clear_common_flag(acked_commits);
        strbuf_release(&req_buf);
 }

diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index ee6d2dde9f35677fb9c83a228839bb7ad405a4fa..d18f2823d86e8b94c5331b1087f962469fc91f2c 100755 (executable)
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -407,6 +407,7 @@ test_expect_success 'in_vain not triggered before first ACK' '
 '
 
 test_expect_success 'in_vain resetted upon ACK' '
+       test_when_finished rm -f log trace2 &&
        rm -rf myserver myclient &&
        git init myserver &&
 
@@ -432,7 +433,8 @@ test_expect_success 'in_vain resetted upon ACK' '
        # first. The 256th commit is common between the client and the server,
        # and should reset in_vain. This allows negotiation to continue until
        # the client reports that first_anotherbranch_commit is common.
-       git -C myclient fetch --progress origin main 2>log &&
+       GIT_TRACE2_EVENT="$(pwd)/trace2" git -C myclient fetch --progress origin main 2>log &&
+       grep \"key\":\"total_rounds\",\"value\":\"6\" trace2 &&
        test_i18ngrep "Total 3 " log
 '
 

diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index f3356f9ea8cc1d893f756bf9af9806d210904124..3211002d466867fb2abb99eeff897029a7fdfa71 100755 (executable)
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -200,7 +200,10 @@ test_expect_success 'push with negotiation' '
        test_commit -C testrepo unrelated_commit &&
        git -C testrepo config receive.hideRefs refs/remotes/origin/first_commit &&
        test_when_finished "rm event" &&
-       GIT_TRACE2_EVENT="$(pwd)/event" git -c protocol.version=2 -c push.negotiate=1 push testrepo refs/heads/main:refs/remotes/origin/main &&
+       GIT_TRACE2_EVENT="$(pwd)/event" \
+               git -c protocol.version=2 -c push.negotiate=1 \
+               push testrepo refs/heads/main:refs/remotes/origin/main &&
+       grep \"key\":\"total_rounds\",\"value\":\"1\" event &&
        grep_wrote 2 event # 1 commit, 1 tree
 '
 
@@ -224,7 +227,10 @@ test_expect_success 'push with negotiation does not attempt to fetch submodules'
        git push testrepo $the_first_commit:refs/remotes/origin/first_commit &&
        test_commit -C testrepo unrelated_commit &&
        git -C testrepo config receive.hideRefs refs/remotes/origin/first_commit &&
-       git -c submodule.recurse=true -c protocol.version=2 -c push.negotiate=1 push testrepo refs/heads/main:refs/remotes/origin/main 2>err &&
+       GIT_TRACE2_EVENT="$(pwd)/event"  git -c submodule.recurse=true \
+               -c protocol.version=2 -c push.negotiate=1 \
+               push testrepo refs/heads/main:refs/remotes/origin/main 2>err &&
+       grep \"key\":\"total_rounds\",\"value\":\"1\" event &&
        ! grep "Fetching submodule" err
 '
 

diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index cf3be0584f40d13ba187cd8c04efc358910714a7..2e57de9c12a39a63b870079c17b014ad24fb93c3 100755 (executable)
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -743,7 +743,11 @@ test_expect_success 'batch missing blob request during checkout' '
 
        # Ensure that there is only one negotiation by checking that there is
        # only "done" line sent. ("done" marks the end of negotiation.)
-       GIT_TRACE_PACKET="$(pwd)/trace" git -C client checkout HEAD^ &&
+       GIT_TRACE_PACKET="$(pwd)/trace" \
+               GIT_TRACE2_EVENT="$(pwd)/trace2_event" \
+               git -C client -c trace2.eventNesting=5 checkout HEAD^ &&
+       grep \"key\":\"total_rounds\",\"value\":\"1\" trace2_event >trace_lines &&
+       test_line_count = 1 trace_lines &&
        grep "fetch> done" trace >done_lines &&
        test_line_count = 1 done_lines
 '

diff --git a/t/t5703-upload-pack-ref-in-want.sh b/t/t5703-upload-pack-ref-in-want.sh
index 9d6cd7d98649c0fe0f40c474d5ed88528ac9ba8c..df74f80061c564b7f69961f0f3d665b1afca460f 100755 (executable)
--- a/t/t5703-upload-pack-ref-in-want.sh
+++ b/t/t5703-upload-pack-ref-in-want.sh
@@ -229,14 +229,16 @@ test_expect_success 'setup repos for fetching with ref-in-want tests' '
 '
 
 test_expect_success 'fetching with exact OID' '
-       test_when_finished "rm -f log" &&
+       test_when_finished "rm -f log trace2" &&
 
        rm -rf local &&
        cp -r "$LOCAL_PRISTINE" local &&
        oid=$(git -C "$REPO" rev-parse d) &&
-       GIT_TRACE_PACKET="$(pwd)/log" git -C local fetch origin \
+       GIT_TRACE_PACKET="$(pwd)/log" GIT_TRACE2_EVENT="$(pwd)/trace2" \
+               git -C local fetch origin \
                "$oid":refs/heads/actual &&
 
+       grep \"key\":\"total_rounds\",\"value\":\"2\" trace2 &&
        git -C "$REPO" rev-parse "d" >expected &&
        git -C local rev-parse refs/heads/actual >actual &&
        test_cmp expected actual &&