* [PATCH 0/2] add trace2 regions to fetch & push
@ 2019-10-02 23:49 Josh Steadmon
2019-10-02 23:49 ` [PATCH 1/2] fetch: add trace2 instrumentation Josh Steadmon
` (3 more replies)
0 siblings, 4 replies; 11+ messages in thread
From: Josh Steadmon @ 2019-10-02 23:49 UTC (permalink / raw)
To: git
We'd like to collect better statistics about where the time is spent in
fetches and pushes so that we can hopefully identify some areas for
future optimization. So let's add some trace2 regions around some of the
fetch/push phases so we can break down their timing.
Josh Steadmon (2):
fetch: add trace2 instrumentation
push: add trace2 instrumentation
builtin/fetch.c | 22 +++++++++++++++-------
builtin/push.c | 2 ++
fetch-pack.c | 13 ++++++++++++-
transport.c | 14 ++++++++++++--
4 files changed, 41 insertions(+), 10 deletions(-)
--
2.23.0.444.g18eeb5a265-goog
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/2] fetch: add trace2 instrumentation
2019-10-02 23:49 [PATCH 0/2] add trace2 regions to fetch & push Josh Steadmon
@ 2019-10-02 23:49 ` Josh Steadmon
2019-10-02 23:49 ` [PATCH 2/2] push: " Josh Steadmon
` (2 subsequent siblings)
3 siblings, 0 replies; 11+ messages in thread
From: Josh Steadmon @ 2019-10-02 23:49 UTC (permalink / raw)
To: git
Add trace2 regions to fetch-pack.c and builtins/fetch.c to better track
time spent in the various phases of a fetch:
* listing refs
* negotiation for protocol versions v0-v2
* fetching refs
* consuming refs
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
builtin/fetch.c | 22 +++++++++++++++-------
fetch-pack.c | 13 ++++++++++++-
2 files changed, 27 insertions(+), 8 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 67c0eb88c6..ee3dc085bb 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1071,8 +1071,11 @@ static int check_exist_and_connected(struct ref *ref_map)
static int fetch_refs(struct transport *transport, struct ref *ref_map)
{
int ret = check_exist_and_connected(ref_map);
- if (ret)
+ if (ret) {
+ trace2_region_enter("fetch", "fetch_refs", the_repository);
ret = transport_fetch_refs(transport, ref_map);
+ trace2_region_leave("fetch", "fetch_refs", the_repository);
+ }
if (!ret)
/*
* Keep the new pack's ".keep" file around to allow the caller
@@ -1088,11 +1091,14 @@ static int consume_refs(struct transport *transport, struct ref *ref_map)
{
int connectivity_checked = transport->smart_options
? transport->smart_options->connectivity_checked : 0;
- int ret = store_updated_refs(transport->url,
- transport->remote->name,
- connectivity_checked,
- ref_map);
+ int ret;
+ trace2_region_enter("fetch", "consume_refs", the_repository);
+ ret = store_updated_refs(transport->url,
+ transport->remote->name,
+ connectivity_checked,
+ ref_map);
transport_unlock_pack(transport);
+ trace2_region_leave("fetch", "consume_refs", the_repository);
return ret;
}
@@ -1337,9 +1343,11 @@ static int do_fetch(struct transport *transport,
argv_array_push(&ref_prefixes, "refs/tags/");
}
- if (must_list_refs)
+ if (must_list_refs) {
+ trace2_region_enter("fetch", "remote_refs", the_repository);
remote_refs = transport_get_remote_refs(transport, &ref_prefixes);
- else
+ trace2_region_leave("fetch", "remote_refs", the_repository);
+ } else
remote_refs = NULL;
argv_array_clear(&ref_prefixes);
diff --git a/fetch-pack.c b/fetch-pack.c
index 6ccc6294ea..c016eeab1d 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -382,6 +382,7 @@ static int find_common(struct fetch_negotiator *negotiator,
state_len = 0;
}
+ trace2_region_enter("fetch-pack", "negotiation_v0_v1", the_repository);
flushes = 0;
retval = -1;
if (args->no_dependents)
@@ -466,6 +467,7 @@ static int find_common(struct fetch_negotiator *negotiator,
}
}
done:
+ trace2_region_leave("fetch-pack", "negotiation_v0_v1", the_repository);
if (!got_ready || !no_done) {
packet_buf_write(&req_buf, "done\n");
send_request(args, fd[1], &req_buf);
@@ -1378,7 +1380,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
enum fetch_state state = FETCH_CHECK_LOCAL;
struct oidset common = OIDSET_INIT;
struct packet_reader reader;
- int in_vain = 0;
+ int in_vain = 0, negotiation_started = 0;
int haves_to_send = INITIAL_FLUSH;
struct fetch_negotiator negotiator;
fetch_negotiator_init(r, &negotiator);
@@ -1421,6 +1423,12 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
}
break;
case FETCH_SEND_REQUEST:
+ if (!negotiation_started) {
+ negotiation_started = 1;
+ trace2_region_enter("fetch-pack",
+ "negotiation_v2",
+ the_repository);
+ }
if (send_fetch_request(&negotiator, fd[1], args, ref,
&common,
&haves_to_send, &in_vain,
@@ -1444,6 +1452,9 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
}
break;
case FETCH_GET_PACK:
+ trace2_region_leave("fetch-pack",
+ "negotiation_v2",
+ the_repository);
/* Check for shallow-info section */
if (process_section_header(&reader, "shallow-info", 1))
receive_shallow_info(args, &reader, shallows, si);
--
2.23.0.444.g18eeb5a265-goog
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/2] push: add trace2 instrumentation
2019-10-02 23:49 [PATCH 0/2] add trace2 regions to fetch & push Josh Steadmon
2019-10-02 23:49 ` [PATCH 1/2] fetch: add trace2 instrumentation Josh Steadmon
@ 2019-10-02 23:49 ` Josh Steadmon
2019-10-07 21:46 ` [PATCH 0/2] add trace2 regions to fetch & push Jonathan Tan
2019-10-07 22:35 ` [PATCH v2 " Josh Steadmon
3 siblings, 0 replies; 11+ messages in thread
From: Josh Steadmon @ 2019-10-02 23:49 UTC (permalink / raw)
To: git
Add trace2 regions in transport.c and builtin/push.c to better track
time spent in various phases of pushing:
* Listing refs
* Checking submodules
* Pushing submodules
* Pushing refs
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
builtin/push.c | 2 ++
transport.c | 14 ++++++++++++--
2 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/builtin/push.c b/builtin/push.c
index 3742daf7b0..cc1292a566 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -357,8 +357,10 @@ static int push_with_options(struct transport *transport, struct refspec *rs,
if (verbosity > 0)
fprintf(stderr, _("Pushing to %s\n"), transport->url);
+ trace2_region_enter("push", "transport_push", the_repository);
err = transport_push(the_repository, transport,
rs, flags, &reject_reasons);
+ trace2_region_leave("push", "transport_push", the_repository);
if (err != 0) {
fprintf(stderr, "%s", push_get_color(PUSH_COLOR_ERROR));
error(_("failed to push some refs to '%s'"), transport->url);
diff --git a/transport.c b/transport.c
index ae558af944..f313f288de 100644
--- a/transport.c
+++ b/transport.c
@@ -1145,8 +1145,10 @@ int transport_push(struct repository *r,
refspec_ref_prefixes(rs, &ref_prefixes);
+ trace2_region_enter("transport_push", "get_refs_list", the_repository);
remote_refs = transport->vtable->get_refs_list(transport, 1,
&ref_prefixes);
+ trace2_region_leave("transport_push", "get_refs_list", the_repository);
argv_array_clear(&ref_prefixes);
@@ -1182,6 +1184,7 @@ int transport_push(struct repository *r,
struct ref *ref = remote_refs;
struct oid_array commits = OID_ARRAY_INIT;
+ trace2_region_enter("transport_push", "push_submodules", the_repository);
for (; ref; ref = ref->next)
if (!is_null_oid(&ref->new_oid))
oid_array_append(&commits,
@@ -1194,9 +1197,11 @@ int transport_push(struct repository *r,
transport->push_options,
pretend)) {
oid_array_clear(&commits);
+ trace2_region_leave("transport_push", "push_submodules", the_repository);
die(_("failed to push all needed submodules"));
}
oid_array_clear(&commits);
+ trace2_region_leave("transport_push", "push_submodules", the_repository);
}
if (((flags & TRANSPORT_RECURSE_SUBMODULES_CHECK) ||
@@ -1207,6 +1212,7 @@ int transport_push(struct repository *r,
struct string_list needs_pushing = STRING_LIST_INIT_DUP;
struct oid_array commits = OID_ARRAY_INIT;
+ trace2_region_enter("transport_push", "check_submodules", the_repository);
for (; ref; ref = ref->next)
if (!is_null_oid(&ref->new_oid))
oid_array_append(&commits,
@@ -1217,15 +1223,19 @@ int transport_push(struct repository *r,
transport->remote->name,
&needs_pushing)) {
oid_array_clear(&commits);
+ trace2_region_leave("transport_push", "check_submodules", the_repository);
die_with_unpushed_submodules(&needs_pushing);
}
string_list_clear(&needs_pushing, 0);
oid_array_clear(&commits);
+ trace2_region_leave("transport_push", "check_submodules", the_repository);
}
- if (!(flags & TRANSPORT_RECURSE_SUBMODULES_ONLY))
+ if (!(flags & TRANSPORT_RECURSE_SUBMODULES_ONLY)) {
+ trace2_region_enter("transport_push", "push_refs", the_repository);
push_ret = transport->vtable->push_refs(transport, remote_refs, flags);
- else
+ trace2_region_leave("transport_push", "push_refs", the_repository);
+ } else
push_ret = 0;
err = push_had_errors(remote_refs);
ret = push_ret | err;
--
2.23.0.444.g18eeb5a265-goog
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 0/2] add trace2 regions to fetch & push
2019-10-02 23:49 [PATCH 0/2] add trace2 regions to fetch & push Josh Steadmon
2019-10-02 23:49 ` [PATCH 1/2] fetch: add trace2 instrumentation Josh Steadmon
2019-10-02 23:49 ` [PATCH 2/2] push: " Josh Steadmon
@ 2019-10-07 21:46 ` Jonathan Tan
2019-10-07 22:36 ` Josh Steadmon
2019-10-07 22:35 ` [PATCH v2 " Josh Steadmon
3 siblings, 1 reply; 11+ messages in thread
From: Jonathan Tan @ 2019-10-07 21:46 UTC (permalink / raw)
To: steadmon; +Cc: git, Jonathan Tan
> We'd like to collect better statistics about where the time is spent in
> fetches and pushes so that we can hopefully identify some areas for
> future optimization. So let's add some trace2 regions around some of the
> fetch/push phases so we can break down their timing.
Thanks.
Patch 1 looks good to me - different regions at the same level
(builtin/fetch.c, so it will be just for "git fetch") and one specific
one just for negotiation, which has to be in fetch-pack.c because only
that file operates at that level.
Patch 2 mostly looks good to me too - unlike fetch, a lot happens in
transport.c, so it's reasonable to put most of the regions there. One
comment: in transport_push(), should trace2_region_{enter,leave} take
"r" instead of "the_repository"?
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 0/2] add trace2 regions to fetch & push
2019-10-02 23:49 [PATCH 0/2] add trace2 regions to fetch & push Josh Steadmon
` (2 preceding siblings ...)
2019-10-07 21:46 ` [PATCH 0/2] add trace2 regions to fetch & push Jonathan Tan
@ 2019-10-07 22:35 ` Josh Steadmon
2019-10-07 22:35 ` [PATCH v2 1/2] fetch: add trace2 instrumentation Josh Steadmon
` (2 more replies)
3 siblings, 3 replies; 11+ messages in thread
From: Josh Steadmon @ 2019-10-07 22:35 UTC (permalink / raw)
To: git, jonathantanmy
We'd like to collect better statistics about where the time is spent in
fetches and pushes so that we can hopefully identify some areas for
future optimization. So let's add some trace2 regions around some of the
fetch/push phases so we can break down their timing.
Changes since V1:
* Use the repository struct argument in transport_push(), rather than
the global the_repository.
Josh Steadmon (2):
fetch: add trace2 instrumentation
push: add trace2 instrumentation
builtin/fetch.c | 22 +++++++++++++++-------
builtin/push.c | 2 ++
fetch-pack.c | 13 ++++++++++++-
transport.c | 14 ++++++++++++--
4 files changed, 41 insertions(+), 10 deletions(-)
Range-diff against v1:
1: 054936f40b ! 1: fe6108b6f9 push: add trace2 instrumentation
@@ transport.c: int transport_push(struct repository *r,
refspec_ref_prefixes(rs, &ref_prefixes);
-+ trace2_region_enter("transport_push", "get_refs_list", the_repository);
++ trace2_region_enter("transport_push", "get_refs_list", r);
remote_refs = transport->vtable->get_refs_list(transport, 1,
&ref_prefixes);
-+ trace2_region_leave("transport_push", "get_refs_list", the_repository);
++ trace2_region_leave("transport_push", "get_refs_list", r);
argv_array_clear(&ref_prefixes);
@@ transport.c: int transport_push(struct repository *r,
struct ref *ref = remote_refs;
struct oid_array commits = OID_ARRAY_INIT;
-+ trace2_region_enter("transport_push", "push_submodules", the_repository);
++ trace2_region_enter("transport_push", "push_submodules", r);
for (; ref; ref = ref->next)
if (!is_null_oid(&ref->new_oid))
oid_array_append(&commits,
@@ transport.c: int transport_push(struct repository *r,
transport->push_options,
pretend)) {
oid_array_clear(&commits);
-+ trace2_region_leave("transport_push", "push_submodules", the_repository);
++ trace2_region_leave("transport_push", "push_submodules", r);
die(_("failed to push all needed submodules"));
}
oid_array_clear(&commits);
-+ trace2_region_leave("transport_push", "push_submodules", the_repository);
++ trace2_region_leave("transport_push", "push_submodules", r);
}
if (((flags & TRANSPORT_RECURSE_SUBMODULES_CHECK) ||
@@ transport.c: int transport_push(struct repository *r,
struct string_list needs_pushing = STRING_LIST_INIT_DUP;
struct oid_array commits = OID_ARRAY_INIT;
-+ trace2_region_enter("transport_push", "check_submodules", the_repository);
++ trace2_region_enter("transport_push", "check_submodules", r);
for (; ref; ref = ref->next)
if (!is_null_oid(&ref->new_oid))
oid_array_append(&commits,
@@ transport.c: int transport_push(struct repository *r,
transport->remote->name,
&needs_pushing)) {
oid_array_clear(&commits);
-+ trace2_region_leave("transport_push", "check_submodules", the_repository);
++ trace2_region_leave("transport_push", "check_submodules", r);
die_with_unpushed_submodules(&needs_pushing);
}
string_list_clear(&needs_pushing, 0);
oid_array_clear(&commits);
-+ trace2_region_leave("transport_push", "check_submodules", the_repository);
++ trace2_region_leave("transport_push", "check_submodules", r);
}
- if (!(flags & TRANSPORT_RECURSE_SUBMODULES_ONLY))
+ if (!(flags & TRANSPORT_RECURSE_SUBMODULES_ONLY)) {
-+ trace2_region_enter("transport_push", "push_refs", the_repository);
++ trace2_region_enter("transport_push", "push_refs", r);
push_ret = transport->vtable->push_refs(transport, remote_refs, flags);
- else
-+ trace2_region_leave("transport_push", "push_refs", the_repository);
++ trace2_region_leave("transport_push", "push_refs", r);
+ } else
push_ret = 0;
err = push_had_errors(remote_refs);
--
2.23.0.581.g78d2f28ef7-goog
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 1/2] fetch: add trace2 instrumentation
2019-10-07 22:35 ` [PATCH v2 " Josh Steadmon
@ 2019-10-07 22:35 ` Josh Steadmon
2019-10-07 22:35 ` [PATCH v2 2/2] push: " Josh Steadmon
2019-10-07 23:55 ` [PATCH v2 0/2] add trace2 regions to fetch & push Jonathan Tan
2 siblings, 0 replies; 11+ messages in thread
From: Josh Steadmon @ 2019-10-07 22:35 UTC (permalink / raw)
To: git, jonathantanmy
Add trace2 regions to fetch-pack.c and builtins/fetch.c to better track
time spent in the various phases of a fetch:
* listing refs
* negotiation for protocol versions v0-v2
* fetching refs
* consuming refs
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
builtin/fetch.c | 22 +++++++++++++++-------
fetch-pack.c | 13 ++++++++++++-
2 files changed, 27 insertions(+), 8 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 67c0eb88c6..ee3dc085bb 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1071,8 +1071,11 @@ static int check_exist_and_connected(struct ref *ref_map)
static int fetch_refs(struct transport *transport, struct ref *ref_map)
{
int ret = check_exist_and_connected(ref_map);
- if (ret)
+ if (ret) {
+ trace2_region_enter("fetch", "fetch_refs", the_repository);
ret = transport_fetch_refs(transport, ref_map);
+ trace2_region_leave("fetch", "fetch_refs", the_repository);
+ }
if (!ret)
/*
* Keep the new pack's ".keep" file around to allow the caller
@@ -1088,11 +1091,14 @@ static int consume_refs(struct transport *transport, struct ref *ref_map)
{
int connectivity_checked = transport->smart_options
? transport->smart_options->connectivity_checked : 0;
- int ret = store_updated_refs(transport->url,
- transport->remote->name,
- connectivity_checked,
- ref_map);
+ int ret;
+ trace2_region_enter("fetch", "consume_refs", the_repository);
+ ret = store_updated_refs(transport->url,
+ transport->remote->name,
+ connectivity_checked,
+ ref_map);
transport_unlock_pack(transport);
+ trace2_region_leave("fetch", "consume_refs", the_repository);
return ret;
}
@@ -1337,9 +1343,11 @@ static int do_fetch(struct transport *transport,
argv_array_push(&ref_prefixes, "refs/tags/");
}
- if (must_list_refs)
+ if (must_list_refs) {
+ trace2_region_enter("fetch", "remote_refs", the_repository);
remote_refs = transport_get_remote_refs(transport, &ref_prefixes);
- else
+ trace2_region_leave("fetch", "remote_refs", the_repository);
+ } else
remote_refs = NULL;
argv_array_clear(&ref_prefixes);
diff --git a/fetch-pack.c b/fetch-pack.c
index 6ccc6294ea..c016eeab1d 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -382,6 +382,7 @@ static int find_common(struct fetch_negotiator *negotiator,
state_len = 0;
}
+ trace2_region_enter("fetch-pack", "negotiation_v0_v1", the_repository);
flushes = 0;
retval = -1;
if (args->no_dependents)
@@ -466,6 +467,7 @@ static int find_common(struct fetch_negotiator *negotiator,
}
}
done:
+ trace2_region_leave("fetch-pack", "negotiation_v0_v1", the_repository);
if (!got_ready || !no_done) {
packet_buf_write(&req_buf, "done\n");
send_request(args, fd[1], &req_buf);
@@ -1378,7 +1380,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
enum fetch_state state = FETCH_CHECK_LOCAL;
struct oidset common = OIDSET_INIT;
struct packet_reader reader;
- int in_vain = 0;
+ int in_vain = 0, negotiation_started = 0;
int haves_to_send = INITIAL_FLUSH;
struct fetch_negotiator negotiator;
fetch_negotiator_init(r, &negotiator);
@@ -1421,6 +1423,12 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
}
break;
case FETCH_SEND_REQUEST:
+ if (!negotiation_started) {
+ negotiation_started = 1;
+ trace2_region_enter("fetch-pack",
+ "negotiation_v2",
+ the_repository);
+ }
if (send_fetch_request(&negotiator, fd[1], args, ref,
&common,
&haves_to_send, &in_vain,
@@ -1444,6 +1452,9 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
}
break;
case FETCH_GET_PACK:
+ trace2_region_leave("fetch-pack",
+ "negotiation_v2",
+ the_repository);
/* Check for shallow-info section */
if (process_section_header(&reader, "shallow-info", 1))
receive_shallow_info(args, &reader, shallows, si);
--
2.23.0.581.g78d2f28ef7-goog
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 2/2] push: add trace2 instrumentation
2019-10-07 22:35 ` [PATCH v2 " Josh Steadmon
2019-10-07 22:35 ` [PATCH v2 1/2] fetch: add trace2 instrumentation Josh Steadmon
@ 2019-10-07 22:35 ` Josh Steadmon
2019-10-07 23:55 ` [PATCH v2 0/2] add trace2 regions to fetch & push Jonathan Tan
2 siblings, 0 replies; 11+ messages in thread
From: Josh Steadmon @ 2019-10-07 22:35 UTC (permalink / raw)
To: git, jonathantanmy
Add trace2 regions in transport.c and builtin/push.c to better track
time spent in various phases of pushing:
* Listing refs
* Checking submodules
* Pushing submodules
* Pushing refs
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
builtin/push.c | 2 ++
transport.c | 14 ++++++++++++--
2 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/builtin/push.c b/builtin/push.c
index 3742daf7b0..cc1292a566 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -357,8 +357,10 @@ static int push_with_options(struct transport *transport, struct refspec *rs,
if (verbosity > 0)
fprintf(stderr, _("Pushing to %s\n"), transport->url);
+ trace2_region_enter("push", "transport_push", the_repository);
err = transport_push(the_repository, transport,
rs, flags, &reject_reasons);
+ trace2_region_leave("push", "transport_push", the_repository);
if (err != 0) {
fprintf(stderr, "%s", push_get_color(PUSH_COLOR_ERROR));
error(_("failed to push some refs to '%s'"), transport->url);
diff --git a/transport.c b/transport.c
index ae558af944..83379a037d 100644
--- a/transport.c
+++ b/transport.c
@@ -1145,8 +1145,10 @@ int transport_push(struct repository *r,
refspec_ref_prefixes(rs, &ref_prefixes);
+ trace2_region_enter("transport_push", "get_refs_list", r);
remote_refs = transport->vtable->get_refs_list(transport, 1,
&ref_prefixes);
+ trace2_region_leave("transport_push", "get_refs_list", r);
argv_array_clear(&ref_prefixes);
@@ -1182,6 +1184,7 @@ int transport_push(struct repository *r,
struct ref *ref = remote_refs;
struct oid_array commits = OID_ARRAY_INIT;
+ trace2_region_enter("transport_push", "push_submodules", r);
for (; ref; ref = ref->next)
if (!is_null_oid(&ref->new_oid))
oid_array_append(&commits,
@@ -1194,9 +1197,11 @@ int transport_push(struct repository *r,
transport->push_options,
pretend)) {
oid_array_clear(&commits);
+ trace2_region_leave("transport_push", "push_submodules", r);
die(_("failed to push all needed submodules"));
}
oid_array_clear(&commits);
+ trace2_region_leave("transport_push", "push_submodules", r);
}
if (((flags & TRANSPORT_RECURSE_SUBMODULES_CHECK) ||
@@ -1207,6 +1212,7 @@ int transport_push(struct repository *r,
struct string_list needs_pushing = STRING_LIST_INIT_DUP;
struct oid_array commits = OID_ARRAY_INIT;
+ trace2_region_enter("transport_push", "check_submodules", r);
for (; ref; ref = ref->next)
if (!is_null_oid(&ref->new_oid))
oid_array_append(&commits,
@@ -1217,15 +1223,19 @@ int transport_push(struct repository *r,
transport->remote->name,
&needs_pushing)) {
oid_array_clear(&commits);
+ trace2_region_leave("transport_push", "check_submodules", r);
die_with_unpushed_submodules(&needs_pushing);
}
string_list_clear(&needs_pushing, 0);
oid_array_clear(&commits);
+ trace2_region_leave("transport_push", "check_submodules", r);
}
- if (!(flags & TRANSPORT_RECURSE_SUBMODULES_ONLY))
+ if (!(flags & TRANSPORT_RECURSE_SUBMODULES_ONLY)) {
+ trace2_region_enter("transport_push", "push_refs", r);
push_ret = transport->vtable->push_refs(transport, remote_refs, flags);
- else
+ trace2_region_leave("transport_push", "push_refs", r);
+ } else
push_ret = 0;
err = push_had_errors(remote_refs);
ret = push_ret | err;
--
2.23.0.581.g78d2f28ef7-goog
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 0/2] add trace2 regions to fetch & push
2019-10-07 21:46 ` [PATCH 0/2] add trace2 regions to fetch & push Jonathan Tan
@ 2019-10-07 22:36 ` Josh Steadmon
2019-10-08 3:53 ` Junio C Hamano
0 siblings, 1 reply; 11+ messages in thread
From: Josh Steadmon @ 2019-10-07 22:36 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git
On 2019.10.07 14:46, Jonathan Tan wrote:
> > We'd like to collect better statistics about where the time is spent in
> > fetches and pushes so that we can hopefully identify some areas for
> > future optimization. So let's add some trace2 regions around some of the
> > fetch/push phases so we can break down their timing.
>
> Thanks.
>
> Patch 1 looks good to me - different regions at the same level
> (builtin/fetch.c, so it will be just for "git fetch") and one specific
> one just for negotiation, which has to be in fetch-pack.c because only
> that file operates at that level.
>
> Patch 2 mostly looks good to me too - unlike fetch, a lot happens in
> transport.c, so it's reasonable to put most of the regions there. One
> comment: in transport_push(), should trace2_region_{enter,leave} take
> "r" instead of "the_repository"?
Ah yeah, thanks for the catch. Fixed in V2.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 0/2] add trace2 regions to fetch & push
2019-10-07 22:35 ` [PATCH v2 " Josh Steadmon
2019-10-07 22:35 ` [PATCH v2 1/2] fetch: add trace2 instrumentation Josh Steadmon
2019-10-07 22:35 ` [PATCH v2 2/2] push: " Josh Steadmon
@ 2019-10-07 23:55 ` Jonathan Tan
2 siblings, 0 replies; 11+ messages in thread
From: Jonathan Tan @ 2019-10-07 23:55 UTC (permalink / raw)
To: steadmon; +Cc: git, jonathantanmy
> Changes since V1:
> * Use the repository struct argument in transport_push(), rather than
> the global the_repository.
Thanks, the patches now look good to me. I verified that the repository
argument to the trace functions just cause a different repo ID to be
printed, which is what we want (e.g. fn_region_enter_printf_va_fl() in
tr2_tgt_event.c, which appears in the vtable defined at the bottom of
the file).
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/2] add trace2 regions to fetch & push
2019-10-07 22:36 ` Josh Steadmon
@ 2019-10-08 3:53 ` Junio C Hamano
2019-10-08 4:18 ` Junio C Hamano
0 siblings, 1 reply; 11+ messages in thread
From: Junio C Hamano @ 2019-10-08 3:53 UTC (permalink / raw)
To: Josh Steadmon; +Cc: Jonathan Tan, git
Josh Steadmon <steadmon@google.com> writes:
> On 2019.10.07 14:46, Jonathan Tan wrote:
>> > We'd like to collect better statistics about where the time is spent in
>> > fetches and pushes so that we can hopefully identify some areas for
>> > future optimization. So let's add some trace2 regions around some of the
>> > fetch/push phases so we can break down their timing.
>>
>> Thanks.
>>
>> Patch 1 looks good to me - different regions at the same level
>> (builtin/fetch.c, so it will be just for "git fetch") and one specific
>> one just for negotiation, which has to be in fetch-pack.c because only
>> that file operates at that level.
>>
>> Patch 2 mostly looks good to me too - unlike fetch, a lot happens in
>> transport.c, so it's reasonable to put most of the regions there. One
>> comment: in transport_push(), should trace2_region_{enter,leave} take
>> "r" instead of "the_repository"?
>
> Ah yeah, thanks for the catch. Fixed in V2.
Yuck. It's already in 'next', isn't it?
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/2] add trace2 regions to fetch & push
2019-10-08 3:53 ` Junio C Hamano
@ 2019-10-08 4:18 ` Junio C Hamano
0 siblings, 0 replies; 11+ messages in thread
From: Junio C Hamano @ 2019-10-08 4:18 UTC (permalink / raw)
To: Josh Steadmon; +Cc: Jonathan Tan, git
Junio C Hamano <gitster@pobox.com> writes:
> Yuck. It's already in 'next', isn't it?
Yuck, indeed. Here is your v2 in an incremental form to be queued
on top.
Thanks.
-- >8 --
Subject: [PATCH 3/2] transport: push codepath can take arbitrary repository
The previous step added annotations with "the_repository" to various
functions in the push codepath in the transport layer, but they all
can take arbitrary repository pointer, and may be working on a
repository that is not the_repository. Fix them.
Signed-off-by: Josh Steadmon <steadmon@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
transport.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/transport.c b/transport.c
index f313f288de..83379a037d 100644
--- a/transport.c
+++ b/transport.c
@@ -1145,10 +1145,10 @@ int transport_push(struct repository *r,
refspec_ref_prefixes(rs, &ref_prefixes);
- trace2_region_enter("transport_push", "get_refs_list", the_repository);
+ trace2_region_enter("transport_push", "get_refs_list", r);
remote_refs = transport->vtable->get_refs_list(transport, 1,
&ref_prefixes);
- trace2_region_leave("transport_push", "get_refs_list", the_repository);
+ trace2_region_leave("transport_push", "get_refs_list", r);
argv_array_clear(&ref_prefixes);
@@ -1184,7 +1184,7 @@ int transport_push(struct repository *r,
struct ref *ref = remote_refs;
struct oid_array commits = OID_ARRAY_INIT;
- trace2_region_enter("transport_push", "push_submodules", the_repository);
+ trace2_region_enter("transport_push", "push_submodules", r);
for (; ref; ref = ref->next)
if (!is_null_oid(&ref->new_oid))
oid_array_append(&commits,
@@ -1197,11 +1197,11 @@ int transport_push(struct repository *r,
transport->push_options,
pretend)) {
oid_array_clear(&commits);
- trace2_region_leave("transport_push", "push_submodules", the_repository);
+ trace2_region_leave("transport_push", "push_submodules", r);
die(_("failed to push all needed submodules"));
}
oid_array_clear(&commits);
- trace2_region_leave("transport_push", "push_submodules", the_repository);
+ trace2_region_leave("transport_push", "push_submodules", r);
}
if (((flags & TRANSPORT_RECURSE_SUBMODULES_CHECK) ||
@@ -1212,7 +1212,7 @@ int transport_push(struct repository *r,
struct string_list needs_pushing = STRING_LIST_INIT_DUP;
struct oid_array commits = OID_ARRAY_INIT;
- trace2_region_enter("transport_push", "check_submodules", the_repository);
+ trace2_region_enter("transport_push", "check_submodules", r);
for (; ref; ref = ref->next)
if (!is_null_oid(&ref->new_oid))
oid_array_append(&commits,
@@ -1223,18 +1223,18 @@ int transport_push(struct repository *r,
transport->remote->name,
&needs_pushing)) {
oid_array_clear(&commits);
- trace2_region_leave("transport_push", "check_submodules", the_repository);
+ trace2_region_leave("transport_push", "check_submodules", r);
die_with_unpushed_submodules(&needs_pushing);
}
string_list_clear(&needs_pushing, 0);
oid_array_clear(&commits);
- trace2_region_leave("transport_push", "check_submodules", the_repository);
+ trace2_region_leave("transport_push", "check_submodules", r);
}
if (!(flags & TRANSPORT_RECURSE_SUBMODULES_ONLY)) {
- trace2_region_enter("transport_push", "push_refs", the_repository);
+ trace2_region_enter("transport_push", "push_refs", r);
push_ret = transport->vtable->push_refs(transport, remote_refs, flags);
- trace2_region_leave("transport_push", "push_refs", the_repository);
+ trace2_region_leave("transport_push", "push_refs", r);
} else
push_ret = 0;
err = push_had_errors(remote_refs);
^ permalink raw reply related [flat|nested] 11+ messages in thread
end of thread, other threads:[~2019-10-08 4:18 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-02 23:49 [PATCH 0/2] add trace2 regions to fetch & push Josh Steadmon
2019-10-02 23:49 ` [PATCH 1/2] fetch: add trace2 instrumentation Josh Steadmon
2019-10-02 23:49 ` [PATCH 2/2] push: " Josh Steadmon
2019-10-07 21:46 ` [PATCH 0/2] add trace2 regions to fetch & push Jonathan Tan
2019-10-07 22:36 ` Josh Steadmon
2019-10-08 3:53 ` Junio C Hamano
2019-10-08 4:18 ` Junio C Hamano
2019-10-07 22:35 ` [PATCH v2 " Josh Steadmon
2019-10-07 22:35 ` [PATCH v2 1/2] fetch: add trace2 instrumentation Josh Steadmon
2019-10-07 22:35 ` [PATCH v2 2/2] push: " Josh Steadmon
2019-10-07 23:55 ` [PATCH v2 0/2] add trace2 regions to fetch & push Jonathan Tan
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).