[v2,0/2] add trace2 regions to fetch & push

Message

Josh Steadmon Oct. 7, 2019, 10:35 p.m. UTC

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);

Comments

Jonathan Tan Oct. 7, 2019, 11:55 p.m. UTC | #1

> 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).