@matheusbsbs / matheus-bagliano / commits / 7c6bd25c7d

files-backend: don't rewrite the `packed-refs` file unnecessarily

Even when we are deleting references, we needn't overwrite the `packed-refs` file if the references that we are deleting only exist as loose references. Implement this optimization as follows: * Add a function `is_packed_transaction_needed()`, which checks whether a given packed-refs transaction actually needs to be carried out (i.e., it returns false if the transaction obviously wouldn't have any effect). This function must be called while holding the `packed-refs` lock to avoid races. * Change `files_transaction_prepare()` to check whether the packed-refs transaction is actually needed. If not, squelch it, but continue holding the `packed-refs` lock until the end of the transaction to avoid races. This fixes a mild regression caused by dc39e09942 (files_ref_store: use a transaction to update packed refs, 2017-09-08). Before that commit, unnecessary rewrites of `packed-refs` were suppressed by `repack_without_refs()`. But the transaction-based writing introduced by that commit didn't perform that optimization. Note that the pre-dc39e09942 code still had to *read* the whole `packed-refs` file to determine that the rewrite could be skipped, so the performance for the cases that the write could be elided was `O(N)` in the number of packed references both before and after dc39e09942. But after that commit the constant factor increased. This commit reimplements the optimization of eliding unnecessary `packed-refs` rewrites. That, plus the fact that since cfa2e29c34 (packed_ref_store: get rid of the `ref_cache` entirely, 2017-03-17) we don't necessarily have to read the whole `packed-refs` file at all, means that deletes of one or a few loose references can now be done with `O(n lg N)` effort, where `n` is the number of loose references being deleted and `N` is the total number of packed references. This commit fixes two tests in t1409. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Michael Haggerty committed Oct 28, 2017 at 11:16 UTC 7c6bd25c7d65e777b5bb26ad5c8248fb3607e6d5

4 files changed +122 -3

refs/files-backend.c

+17 -1

@@ -2562,7 +2562,23 @@ static int files_transaction_prepare(struct ref_store *ref_store,

2562 goto cleanup;

2563 }

2564 backend_data->packed_refs_locked = 1;

2565 - ret = ref_transaction_prepare(packed_transaction, err);

2565 +

2566 + if (is_packed_transaction_needed(refs->packed_ref_store,

2567 + packed_transaction)) {

2568 + ret = ref_transaction_prepare(packed_transaction, err);

2569 + } else {

2570 + /*

2571 + * We can skip rewriting the `packed-refs`

2572 + * file. But we do need to leave it locked, so

2573 + * that somebody else doesn't pack a reference

2574 + * that we are trying to delete.

2575 + */

2576 + if (ref_transaction_abort(packed_transaction, err)) {

2577 + ret = TRANSACTION_GENERIC_ERROR;

2578 + goto cleanup;

2579 + }

2580 + backend_data->packed_transaction = NULL;

2581 + }

2582 }

2583

2584 cleanup:

refs/packed-backend.c

+94

@@ -754,6 +754,100 @@ error:

754 return -1;

755 }

756

757 +int is_packed_transaction_needed(struct ref_store *ref_store,

758 + struct ref_transaction *transaction)

759 +{

760 + struct packed_ref_store *refs = packed_downcast(

761 + ref_store,

762 + REF_STORE_READ,

763 + "is_packed_transaction_needed");

764 + struct strbuf referent = STRBUF_INIT;

765 + size_t i;

766 + int ret;

767 +

768 + if (!is_lock_file_locked(&refs->lock))

769 + BUG("is_packed_transaction_needed() called while unlocked");

770 +

771 + /*

772 + * We're only going to bother returning false for the common,

773 + * trivial case that references are only being deleted, their

774 + * old values are not being checked, and the old `packed-refs`

775 + * file doesn't contain any of those reference(s). This gives

776 + * false positives for some other cases that could

777 + * theoretically be optimized away:

778 + *

779 + * 1. It could be that the old value is being verified without

780 + * setting a new value. In this case, we could verify the

781 + * old value here and skip the update if it agrees. If it

782 + * disagrees, we could either let the update go through

783 + * (the actual commit would re-detect and report the

784 + * problem), or come up with a way of reporting such an

785 + * error to *our* caller.

786 + *

787 + * 2. It could be that a new value is being set, but that it

788 + * is identical to the current packed value of the

789 + * reference.

790 + *

791 + * Neither of these cases will come up in the current code,

792 + * because the only caller of this function passes to it a

793 + * transaction that only includes `delete` updates with no

794 + * `old_id`. Even if that ever changes, false positives only

795 + * cause an optimization to be missed; they do not affect

796 + * correctness.

797 + */

798 +

799 + /*

800 + * Start with the cheap checks that don't require old

801 + * reference values to be read:

802 + */

803 + for (i = 0; i < transaction->nr; i++) {

804 + struct ref_update *update = transaction->updates[i];

805 +

806 + if (update->flags & REF_HAVE_OLD)

807 + /* Have to check the old value -> needed. */

808 + return 1;

809 +

810 + if ((update->flags & REF_HAVE_NEW) && !is_null_oid(&update->new_oid))

811 + /* Have to set a new value -> needed. */

812 + return 1;

813 + }

814 +

815 + /*

816 + * The transaction isn't checking any old values nor is it

817 + * setting any nonzero new values, so it still might be able

818 + * to be skipped. Now do the more expensive check: the update

819 + * is needed if any of the updates is a delete, and the old

820 + * `packed-refs` file contains a value for that reference.

821 + */

822 + ret = 0;

823 + for (i = 0; i < transaction->nr; i++) {

824 + struct ref_update *update = transaction->updates[i];

825 + unsigned int type;

826 + struct object_id oid;

827 +

828 + if (!(update->flags & REF_HAVE_NEW))

829 + /*

830 + * This reference isn't being deleted -> not

831 + * needed.

832 + */

833 + continue;

834 +

835 + if (!refs_read_raw_ref(ref_store, update->refname,

836 + oid.hash, &referent, &type) ||

837 + errno != ENOENT) {

838 + /*

839 + * We have to actually delete that reference

840 + * -> this transaction is needed.

841 + */

842 + ret = 1;

843 + break;

844 + }

845 + }

846 +

847 + strbuf_release(&referent);

848 + return ret;

849 +}

850 +

851 struct packed_transaction_backend_data {

852 /* True iff the transaction owns the packed-refs lock. */

853 int own_lock;

refs/packed-backend.h

@@ -23,4 +23,13 @@ int packed_refs_lock(struct ref_store *ref_store, int flags, struct strbuf *err)

23 void packed_refs_unlock(struct ref_store *ref_store);

24 int packed_refs_is_locked(struct ref_store *ref_store);

26 +/*

27 + * Return true if `transaction` really needs to be carried out against

28 + * the specified packed_ref_store, or false if it can be skipped

29 + * (i.e., because it is an obvious NOOP). `ref_store` must be locked

30 + * before calling this function.

31 + */

32 +int is_packed_transaction_needed(struct ref_store *ref_store,

33 + struct ref_transaction *transaction);

34 +

35 #endif /* REFS_PACKED_BACKEND_H */

t/t1409-avoid-packing-refs.sh

+2 -2

@@ -26,7 +26,7 @@ test_expect_success 'setup' '

26 C=$(git rev-parse HEAD)

27 '

29 -test_expect_failure 'do not create packed-refs file gratuitously' '

29 +test_expect_success 'do not create packed-refs file gratuitously' '

30 test_must_fail test -f .git/packed-refs &&

31 git update-ref refs/heads/foo $A &&

32 test_must_fail test -f .git/packed-refs &&

@@ -107,7 +107,7 @@ test_expect_success 'leave packed-refs untouched on verify of loose' '

107 check_packed_refs_marked

108 '

109

110 -test_expect_failure 'leave packed-refs untouched on delete of loose' '

110 +test_expect_success 'leave packed-refs untouched on delete of loose' '

111 git pack-refs --all &&

112 git update-ref refs/heads/loose-delete $A &&

113 mark_packed_refs &&