Re: [PATCH] refs.c: enable large transactions
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
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
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
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
