@matheusbsbs / matheus-bagliano / commits / 9fec7b2130

connected: refactor iterator to return next object ID directly

The object ID iterator used by the connectivity checks returns the next object ID via an out-parameter and then uses a return code to indicate whether an item was found. This is a bit roundabout: instead of a separate error code, we can just return the next object ID directly and use `NULL` pointers as indicator that the iterator got no items left. Furthermore, this avoids a copy of the object ID. Refactor the iterator and all its implementations to return object IDs directly. This brings a tiny performance improvement when doing a mirror-fetch of a repository with about 2.3M refs: Benchmark #1: 328dc58b49919c43897240f2eabfa30be2ce32a4~: git-fetch Time (mean ± σ): 30.110 s ± 0.148 s [User: 27.161 s, System: 5.075 s] Range (min … max): 29.934 s … 30.406 s 10 runs Benchmark #2: 328dc58b49919c43897240f2eabfa30be2ce32a4: git-fetch Time (mean ± σ): 29.899 s ± 0.109 s [User: 26.916 s, System: 5.104 s] Range (min … max): 29.696 s … 29.996 s 10 runs Summary '328dc58b49919c43897240f2eabfa30be2ce32a4: git-fetch' ran 1.01 ± 0.01 times faster than '328dc58b49919c43897240f2eabfa30be2ce32a4~: git-fetch' While this 1% speedup could be labelled as statistically insignificant, the speedup is consistent on my machine. Furthermore, this is an end to end test, so it is expected that the improvement in the connectivity check itself is more significant. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Patrick Steinhardt committed Sep 1, 2021 at 15:09 UTC 9fec7b213045135655354e864d15894175428d5a

6 files changed +25 -31

builtin/clone.c

+3 -5

@@ -657,7 +657,7 @@ static void write_followtags(const struct ref *refs, const char *msg)

657 }

658 }

659

660 -static int iterate_ref_map(void *cb_data, struct object_id *oid)

660 +static const struct object_id *iterate_ref_map(void *cb_data)

661 {

662 struct ref **rm = cb_data;

663 struct ref *ref = *rm;

@@ -668,13 +668,11 @@ static int iterate_ref_map(void *cb_data, struct object_id *oid)

668 */

669 while (ref && !ref->peer_ref)

670 ref = ref->next;

671 - /* Returning -1 notes "end of list" to the caller. */

671 if (!ref)

673 - return -1;

672 + return NULL;

673

675 - oidcpy(oid, &ref->old_oid);

674 *rm = ref->next;

677 - return 0;

675 + return &ref->old_oid;

676 }

677

678 static void update_remote_refs(const struct ref *refs,

builtin/fetch.c

+3 -4

@@ -962,7 +962,7 @@ static int update_local_ref(struct ref *ref,

962 }

963 }

964

965 -static int iterate_ref_map(void *cb_data, struct object_id *oid)

965 +static const struct object_id *iterate_ref_map(void *cb_data)

966 {

967 struct ref **rm = cb_data;

968 struct ref *ref = *rm;

@@ -970,10 +970,9 @@ static int iterate_ref_map(void *cb_data, struct object_id *oid)

970 while (ref && ref->status == REF_STATUS_REJECT_SHALLOW)

971 ref = ref->next;

972 if (!ref)

973 - return -1; /* end of the list */

973 + return NULL;

974 *rm = ref->next;

975 - oidcpy(oid, &ref->old_oid);

976 - return 0;

975 + return &ref->old_oid;

976 }

977

978 struct fetch_head {

builtin/receive-pack.c

+7 -10

@@ -1306,7 +1306,7 @@ static void refuse_unconfigured_deny_delete_current(void)

1306 rp_error("%s", _(refuse_unconfigured_deny_delete_current_msg));

1307 }

1308

1309 -static int command_singleton_iterator(void *cb_data, struct object_id *oid);

1309 +static const struct object_id *command_singleton_iterator(void *cb_data);

1310 static int update_shallow_ref(struct command *cmd, struct shallow_info *si)

1311 {

1312 struct shallow_lock shallow_lock = SHALLOW_LOCK_INIT;

@@ -1731,16 +1731,15 @@ static void check_aliased_updates(struct command *commands)

1731 string_list_clear(&ref_list, 0);

1732 }

1733

1734 -static int command_singleton_iterator(void *cb_data, struct object_id *oid)

1734 +static const struct object_id *command_singleton_iterator(void *cb_data)

1735 {

1736 struct command **cmd_list = cb_data;

1737 struct command *cmd = *cmd_list;

1738

1739 if (!cmd || is_null_oid(&cmd->new_oid))

1740 - return -1; /* end of list */

1740 + return NULL;

1741 *cmd_list = NULL; /* this returns only one */

1742 - oidcpy(oid, &cmd->new_oid);

1743 - return 0;

1742 + return &cmd->new_oid;

1743 }

1744

1745 static void set_connectivity_errors(struct command *commands,

@@ -1770,7 +1769,7 @@ struct iterate_data {

1769 struct shallow_info *si;

1770 };

1771

1773 -static int iterate_receive_command_list(void *cb_data, struct object_id *oid)

1772 +static const struct object_id *iterate_receive_command_list(void *cb_data)

1773 {

1774 struct iterate_data *data = cb_data;

1775 struct command **cmd_list = &data->cmds;

@@ -1781,13 +1780,11 @@ static int iterate_receive_command_list(void *cb_data, struct object_id *oid)

1780 /* to be checked in update_shallow_ref() */

1781 continue;

1782 if (!is_null_oid(&cmd->new_oid) && !cmd->skip_update) {

1784 - oidcpy(oid, &cmd->new_oid);

1783 *cmd_list = cmd->next;

1786 - return 0;

1784 + return &cmd->new_oid;

1785 }

1786 }

1789 - *cmd_list = NULL;

1790 - return -1; /* end of list */

1787 + return NULL;

1788 }

1789

1790 static void reject_updates_to_hidden(struct command *commands)

connected.c

+8 -7

@@ -24,7 +24,7 @@ int check_connected(oid_iterate_fn fn, void *cb_data,

24 struct child_process rev_list = CHILD_PROCESS_INIT;

25 FILE *rev_list_in;

26 struct check_connected_options defaults = CHECK_CONNECTED_INIT;

27 - struct object_id oid;

27 + const struct object_id *oid;

28 int err = 0;

29 struct packed_git *new_pack = NULL;

30 struct transport *transport;

@@ -34,7 +34,8 @@ int check_connected(oid_iterate_fn fn, void *cb_data,

34 opt = &defaults;

35 transport = opt->transport;

37 - if (fn(cb_data, &oid)) {

37 + oid = fn(cb_data);

38 + if (!oid) {

39 if (opt->err_fd)

40 close(opt->err_fd);

41 return err;

@@ -73,7 +74,7 @@ int check_connected(oid_iterate_fn fn, void *cb_data,

74 for (p = get_all_packs(the_repository); p; p = p->next) {

75 if (!p->pack_promisor)

76 continue;

76 - if (find_pack_entry_one(oid.hash, p))

77 + if (find_pack_entry_one(oid->hash, p))

78 goto promisor_pack_found;

79 }

80 /*

@@ -83,7 +84,7 @@ int check_connected(oid_iterate_fn fn, void *cb_data,

84 goto no_promisor_pack_found;

85 promisor_pack_found:

86 ;

86 - } while (!fn(cb_data, &oid));

87 + } while ((oid = fn(cb_data)) != NULL);

88 return 0;

89 }

@@ -132,12 +133,12 @@ no_promisor_pack_found:

133 * are sure the ref is good and not sending it to

134 * rev-list for verification.

135 */

135 - if (new_pack && find_pack_entry_one(oid.hash, new_pack))

136 + if (new_pack && find_pack_entry_one(oid->hash, new_pack))

137 continue;

138

138 - if (fprintf(rev_list_in, "%s\n", oid_to_hex(&oid)) < 0)

139 + if (fprintf(rev_list_in, "%s\n", oid_to_hex(oid)) < 0)

140 break;

140 - } while (!fn(cb_data, &oid));

141 + } while ((oid = fn(cb_data)) != NULL);

142

143 if (ferror(rev_list_in) || fflush(rev_list_in)) {

144 if (errno != EPIPE && errno != EINVAL)

connected.h

+1 -1

@@ -9,7 +9,7 @@ struct transport;

9 * When called after returning the name for the last object, return -1

10 * to signal EOF, otherwise return 0.

11 */

12 -typedef int (*oid_iterate_fn)(void *, struct object_id *oid);

12 +typedef const struct object_id *(*oid_iterate_fn)(void *);

14 /*

15 * Named-arguments struct for check_connected. All arguments are

fetch-pack.c

+3 -4

@@ -1912,16 +1912,15 @@ static void update_shallow(struct fetch_pack_args *args,

1912 oid_array_clear(&ref);

1913 }

1914

1915 -static int iterate_ref_map(void *cb_data, struct object_id *oid)

1915 +static const struct object_id *iterate_ref_map(void *cb_data)

1916 {

1917 struct ref **rm = cb_data;

1918 struct ref *ref = *rm;

1919

1920 if (!ref)

1921 - return -1; /* end of the list */

1921 + return NULL;

1922 *rm = ref->next;

1923 - oidcpy(oid, &ref->old_oid);

1924 - return 0;

1923 + return &ref->old_oid;

1924 }

1925

1926 struct ref *fetch_pack(struct fetch_pack_args *args,