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

2015-04-21 Thread Junio C Hamano 
Stefan Beller sbel...@google.com writes:

 This replaces the latest patch on origin/sb/remove-fd-from-ref-lock

Thanks, will replace.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2015-04-21 Thread Junio C Hamano 
Stefan Beller sbel...@google.com writes:

 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.

Just to set the standard yardstick straight, either is fine, but I
would say the latter is slightly preferrable from readability's
point of view.  Rows of dense single lines get tiring to read pretty
quickly.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2015-04-21 Thread Stefan Beller 
On Tue, Apr 21, 2015 at 10:16 AM, Junio C Hamano gits...@pobox.com wrote:
 Stefan Beller sbel...@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


 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. :(


 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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2015-04-21 Thread Junio C Hamano 
Stefan Beller sbel...@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
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?

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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] refs.c: enable large transactions

2015-04-20 Thread Stefan Beller 
This is another attempt on enabling large transactions
(large in terms of open file descriptors). We keep track of how many
lock files are opened by the ref_transaction_commit function.
When more than a reasonable amount of files is open, we close
the file descriptors to make sure the transaction can continue.

Another idea I had during implementing this was to move this file
closing into the lock file API, such that only a certain amount of
lock files can be open at any given point in time and we'd be 'garbage
collecting' open fds when necessary in any relevant call to the lock
file API. This would have brought the advantage of having such
functionality available in other users of the lock file API as well.
The downside however is the over complication, you really need to always
check for (lock-fd != -1) all the time, which may slow down other parts
of the code, which did not ask for such a feature.

Signed-off-by: Stefan Beller sbel...@google.com
---

This replaces the latest patch on origin/sb/remove-fd-from-ref-lock
The test suite passes now

 refs.c| 13 +
 t/t1400-update-ref.sh |  4 ++--
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/refs.c b/refs.c
index 4f495bd..1e8cb16 100644
--- a/refs.c
+++ b/refs.c
@@ -3041,6 +3041,8 @@ static int write_ref_sha1(struct ref_lock *lock,
errno = EINVAL;
return -1;
}
+   if (lock-lk-fd == -1)
+   reopen_lock_file(lock-lk);
if (write_in_full(lock-lk-fd, sha1_to_hex(sha1), 40) != 40 ||
write_in_full(lock-lk-fd, term, 1) != 1 ||
close_ref(lock)  0) {
@@ -3719,6 +3721,12 @@ int ref_transaction_commit(struct ref_transaction 
*transaction,
 {
int ret = 0, i;
int n = transaction-nr;
+   /*
+* 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;
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);
+   }
}
 
/* Perform updates first so live commits remain referenced */
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
-- 
2.4.0.rc2.5.g4c2045b.dirty

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html