Re: [PATCH] refs.c: enable large transactions

From: Stefan Beller <sbeller@google.com>
Date: Apr 21, 2015, 17:24 UTC
Message-ID: <CAGZ79kay11Pd4ni1j6jSYbenSXVfUP-6irPBc_r1=gUZFe-Hjw@mail.gmail.com>
In-Reply-To: <xmqqegndieqd.fsf@gitster.dls.corp.google.com>

On Tue, Apr 21, 2015 at 10:16 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 10 quoted lines

> Stefan Beller <sbeller@google.com> writes:
>
>> +     /*
>> +      * We may want to open many files in a large transaction, so come up with
>> +      * a reasonable maximum, keep some spares for stdin/out and other open
>> +      * files.
>> +      */
>> +     int remaining_fds = get_max_fd_limit() - 32;
>
> Can this go negative?  If it does so, does it matter?  I think the

Yes it can go negative. It doesn't matter as we'd then run into the "close_lock_file(update->lock->lk);" case below.

I thought about putting a cap on it to not let it go negative in the first place, but I did not find an easily accessible max() function, as I'd like to write it as

    int remaining_fds = max(get_max_fd_limit() - 32, 0);

to have it in one line. The alternative of

    int remaining_fds = get_max_fd_limit() - 32;
    ...
    if (remaining_fds < 0)
        remaining_fds = 0

seemed to cuttered to me. Yet another alternative

     int remaining_fds = get_max_fd_limit() - 32 < 0 ? 0 :
get_max_fd_limit() - 32;

is also not appealing as we'd have to call get_max_fd_limit 2 times. That's why I thought going negative is not as bad, but have readable code instead.

As you think about the possibility of remaining_fds being negative, this might not be the best idea though

Show 17 quoted lines

> code doesn't barf, but starting from a negative "remaining" feels
> cryptic, compared to starting from a zero "remaining".
>
>>       struct ref_update **updates = transaction->updates;
>>       struct string_list refs_to_delete = STRING_LIST_INIT_NODUP;
>>       struct string_list_item *ref_to_delete;
>> @@ -3762,6 +3770,11 @@ int ref_transaction_commit(struct ref_transaction *transaction,
>>                                   update->refname);
>>                       goto cleanup;
>>               }
>> +             if (remaining_fds > 0) {
>> +                     remaining_fds--;
>> +             } else {
>> +                     close_lock_file(update->lock->lk);
>> +             }
>
> Any plan to add more code to these blocks in future updates?

I'll remove the braces. :(

Show 25 quoted lines

>
> Thanks.
>
>> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
>> index 7a69f1a..636d3a1 100755
>> --- a/t/t1400-update-ref.sh
>> +++ b/t/t1400-update-ref.sh
>> @@ -1071,7 +1071,7 @@ run_with_limited_open_files () {
>>
>>  test_lazy_prereq ULIMIT_FILE_DESCRIPTORS 'run_with_limited_open_files true'
>>
>> -test_expect_failure ULIMIT_FILE_DESCRIPTORS 'large transaction creating branches does not burst open file limit' '
>> +test_expect_success ULIMIT_FILE_DESCRIPTORS 'large transaction creating branches does not burst open file limit' '
>>  (
>>       for i in $(test_seq 33)
>>       do
>> @@ -1082,7 +1082,7 @@ test_expect_failure ULIMIT_FILE_DESCRIPTORS 'large transaction creating branches
>>  )
>>  '
>>
>> -test_expect_failure ULIMIT_FILE_DESCRIPTORS 'large transaction deleting branches does not burst open file limit' '
>> +test_expect_success ULIMIT_FILE_DESCRIPTORS 'large transaction deleting branches does not burst open file limit' '
>>  (
>>       for i in $(test_seq 33)
>>       do