git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Neeraj Singh <nksingh85@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: "Neeraj Singh via GitGitGadget" <gitgitgadget@gmail.com>,
	"Git List" <git@vger.kernel.org>,
	"Johannes Schindelin" <Johannes.Schindelin@gmx.de>,
	"Jeff King" <peff@peff.net>,
	"Jeff Hostetler" <jeffhost@microsoft.com>,
	"Christoph Hellwig" <hch@lst.de>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Randall S. Becker" <rsbecker@nexbridge.com>,
	"Neeraj K. Singh" <neerajsi@microsoft.com>
Subject: Re: [PATCH v3 6/6] core.fsyncobjectfiles: enable batch mode for testing
Date: Wed, 15 Sep 2021 15:43:38 -0700	[thread overview]
Message-ID: <CANQDOdc8F7a3ZeTDpUWrt8uUntnX4jHYxyj96SPwH-P=kMrneg@mail.gmail.com> (raw)
In-Reply-To: <xmqqtuilyfls.fsf@gitster.g>

On Wed, Sep 15, 2021 at 9:21 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Neeraj Singh via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Neeraj Singh <neerajsi@microsoft.com>
> >
> > Signed-off-by: Neeraj Singh <neerajsi@microsoft.com>
> > ---
> >  environment.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/environment.c b/environment.c
> > index 3e23eafff80..27d5e11267e 100644
> > --- a/environment.c
> > +++ b/environment.c
> > @@ -43,7 +43,7 @@ const char *git_hooks_path;
> >  int zlib_compression_level = Z_BEST_SPEED;
> >  int core_compression_level;
> >  int pack_compression_level = Z_DEFAULT_COMPRESSION;
> > -enum FSYNC_OBJECT_FILES_MODE fsync_object_files;
> > +enum FSYNC_OBJECT_FILES_MODE fsync_object_files = FSYNC_OBJECT_FILES_BATCH;
> >  size_t packed_git_window_size = DEFAULT_PACKED_GIT_WINDOW_SIZE;
> >  size_t packed_git_limit = DEFAULT_PACKED_GIT_LIMIT;
> >  size_t delta_base_cache_limit = 96 * 1024 * 1024;
>
> Despite what the title of the change claims, this is not "enable for
> testing", but "enable for everybody even in production", isn't it?
>
> I'd prefer we do not do this, certainly not for "testing".
>
> If setting the variable to "batch" were meant to eventually improve
> performance for all different flavours of workload, I do not think
> we would mind if we set it to "batch" for those who opt into the
> "experimental" set of features by setting the feature.experimental
> configuration variable to true.  And after a few development cycles
> when the feature proves to be useful for everybody, we may want to
> apply this patch under a justification that is different from "for
> testing".
>
> On the other hand, if this is meant to help 85% of people while
> degrading the remainder of workflow, I do not think we would want to
> see this change without a warning that says something along the
> lines of "under rare circumstances (e.g. if you employ such and such
> workflow), the new default value used for the core.fsyncObjectFiles
> configuration variable will hurt performance."
>
> Since this is about answering the question "between performance and
> crash resilience, where do you as an end user strike the balance for
> your needs?", I do not think it falls into either of the above two
> categories.
>
> The only plausible justification I can think of to apply a "we
> default to 'batch' for everybody" patch with is something like:
>
>     Now with the 'batch' setting for core.fsyncObjectFiles, unlike
>     'true' that paid very high overhead, the overhead to ensure our
>     writes hit the disk platters has so greatly been reduced that it
>     hurts the performance only negligibly.  Let's switch the default
>     from the unsafe value of 'false' to safer and performant value
>     of 'batch'.
>
> I however doubt with the current round of patches, we are there yet.

Sorry for being unclear here (and perhaps including an improper patch).
This commit is mainly to ensure that we get coverage of batch mode on all
platforms in the CI infrastructure.  I don't believe it should be included in
mainline git without significantly more discussion and experimentation.

However, I'd hope that Git for Windows would be able to adopt batch mode
by default when they pull this series in. They are currently enabling fsync
by default.

Batch mode does have more cost, particularly on rotational media.
I think git should eventually enable batch mode by default with the proviso that
maintainers and people running ephemeral CI infrastructure should turn
fsync off
if they care more about speed than durability.

Do you think that feature.experimental is a good place to put this right away,
or should we just leave this as an option that Git for Windows can pick up and
leave the other platforms alone?

Thanks,
Neeraj

  reply	other threads:[~2021-09-15 22:43 UTC|newest]

Thread overview: 160+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-25  1:51 [PATCH 0/2] [RFC] Implement a bulk-checkin option for core.fsyncObjectFiles Neeraj K. Singh via GitGitGadget
2021-08-25  1:51 ` [PATCH 1/2] object-file: use futimes rather than utime Neeraj Singh via GitGitGadget
2021-08-25 13:51   ` Johannes Schindelin
2021-08-25 22:08     ` Neeraj Singh
2021-08-25  1:51 ` [PATCH 2/2] core.fsyncobjectfiles: batch disk flushes Neeraj Singh via GitGitGadget
2021-08-25  5:38   ` Christoph Hellwig
2021-08-25 17:40     ` Neeraj Singh
2021-08-26  5:54       ` Christoph Hellwig
2021-08-25 16:11   ` Ævar Arnfjörð Bjarmason
2021-08-26  0:49     ` Neeraj Singh
2021-08-26  5:50       ` Christoph Hellwig
2021-08-28  0:20         ` Neeraj Singh
2021-08-28  6:57           ` Christoph Hellwig
2021-08-31 19:59             ` Neeraj Singh
2021-09-01  5:09               ` Christoph Hellwig
2021-08-26  5:57     ` Christoph Hellwig
2021-08-25 18:52   ` Johannes Schindelin
2021-08-25 21:26     ` Junio C Hamano
2021-08-26  1:19     ` Neeraj Singh
2021-08-25 16:58 ` [PATCH 0/2] [RFC] Implement a bulk-checkin option for core.fsyncObjectFiles Neeraj Singh
2021-08-27 23:49 ` [PATCH v2 0/6] Implement a batched fsync " Neeraj K. Singh via GitGitGadget
2021-08-27 23:49   ` [PATCH v2 1/6] object-file: use futimens rather than utime Neeraj Singh via GitGitGadget
2021-08-27 23:49   ` [PATCH v2 2/6] bulk-checkin: rename 'state' variable and separate 'plugged' boolean Neeraj Singh via GitGitGadget
2021-08-27 23:49   ` [PATCH v2 3/6] core.fsyncobjectfiles: batched disk flushes Neeraj Singh via GitGitGadget
2021-08-27 23:49   ` [PATCH v2 4/6] core.fsyncobjectfiles: add windows support for batch mode Neeraj Singh via GitGitGadget
2021-08-27 23:49   ` [PATCH v2 5/6] update-index: use the bulk-checkin infrastructure Neeraj Singh via GitGitGadget
2021-08-27 23:49   ` [PATCH v2 6/6] core.fsyncobjectfiles: performance tests for add and stash Neeraj Singh via GitGitGadget
2021-09-07 19:44   ` [PATCH v2 0/6] Implement a batched fsync option for core.fsyncObjectFiles Neeraj Singh
2021-09-07 19:50     ` Ævar Arnfjörð Bjarmason
2021-09-07 19:54     ` Randall S. Becker
2021-09-08  0:54       ` Neeraj Singh
2021-09-08  1:22         ` Ævar Arnfjörð Bjarmason
2021-09-08 14:04           ` Randall S. Becker
2021-09-08 19:01           ` Neeraj Singh
2021-09-08  0:55     ` Neeraj Singh
2021-09-08  6:44       ` Junio C Hamano
2021-09-08  6:49         ` Christoph Hellwig
2021-09-08 13:57           ` Randall S. Becker
2021-09-08 14:13             ` 'Christoph Hellwig'
2021-09-08 14:25               ` Randall S. Becker
2021-09-08 16:34         ` Neeraj Singh
2021-09-08 19:12           ` Junio C Hamano
2021-09-08 19:20             ` Neeraj Singh
2021-09-08 19:23           ` Ævar Arnfjörð Bjarmason
2021-09-14  3:38   ` [PATCH v3 " Neeraj K. Singh via GitGitGadget
2021-09-14  3:38     ` [PATCH v3 1/6] bulk-checkin: rename 'state' variable and separate 'plugged' boolean Neeraj Singh via GitGitGadget
2021-09-14  3:38     ` [PATCH v3 2/6] core.fsyncobjectfiles: batched disk flushes Neeraj Singh via GitGitGadget
2021-09-14 10:39       ` Bagas Sanjaya
2021-09-14 19:05         ` Neeraj Singh
2021-09-14 19:34       ` Junio C Hamano
2021-09-14 20:33         ` Junio C Hamano
2021-09-15  4:55         ` Neeraj Singh
2021-09-14  3:38     ` [PATCH v3 3/6] core.fsyncobjectfiles: add windows support for batch mode Neeraj Singh via GitGitGadget
2021-09-14  3:38     ` [PATCH v3 4/6] update-index: use the bulk-checkin infrastructure Neeraj Singh via GitGitGadget
2021-09-14 19:35       ` Junio C Hamano
2021-09-14  3:38     ` [PATCH v3 5/6] core.fsyncobjectfiles: performance tests for add and stash Neeraj Singh via GitGitGadget
2021-09-14  3:38     ` [PATCH v3 6/6] core.fsyncobjectfiles: enable batch mode for testing Neeraj Singh via GitGitGadget
2021-09-15 16:21       ` Junio C Hamano
2021-09-15 22:43         ` Neeraj Singh [this message]
2021-09-15 23:12           ` Junio C Hamano
2021-09-16  6:19             ` Junio C Hamano
2021-09-14  5:49     ` [PATCH v3 0/6] Implement a batched fsync option for core.fsyncObjectFiles Christoph Hellwig
2021-09-20 22:15     ` [PATCH v4 " Neeraj K. Singh via GitGitGadget
2021-09-20 22:15       ` [PATCH v4 1/6] bulk-checkin: rename 'state' variable and separate 'plugged' boolean Neeraj Singh via GitGitGadget
2021-09-20 22:15       ` [PATCH v4 2/6] core.fsyncobjectfiles: batched disk flushes Neeraj Singh via GitGitGadget
2021-09-21 23:16         ` Ævar Arnfjörð Bjarmason
2021-09-22  1:23           ` Neeraj Singh
2021-09-22  2:02             ` Ævar Arnfjörð Bjarmason
2021-09-22 19:46             ` Neeraj Singh
2021-09-20 22:15       ` [PATCH v4 3/6] core.fsyncobjectfiles: add windows support for batch mode Neeraj Singh via GitGitGadget
2021-09-21 23:42         ` Ævar Arnfjörð Bjarmason
2021-09-22  1:23           ` Neeraj Singh
2021-09-20 22:15       ` [PATCH v4 4/6] update-index: use the bulk-checkin infrastructure Neeraj Singh via GitGitGadget
2021-09-21 23:46         ` Ævar Arnfjörð Bjarmason
2021-09-22  1:27           ` Neeraj Singh
2021-09-23 22:32             ` Neeraj Singh
2021-09-20 22:15       ` [PATCH v4 5/6] core.fsyncobjectfiles: tests for batch mode Neeraj Singh via GitGitGadget
2021-09-21 23:54         ` Ævar Arnfjörð Bjarmason
2021-09-22  1:30           ` Neeraj Singh
2021-09-22  1:58             ` Ævar Arnfjörð Bjarmason
2021-09-22 17:55               ` Neeraj Singh
2021-09-22 20:01                 ` Ævar Arnfjörð Bjarmason
2021-09-20 22:15       ` [PATCH v4 6/6] core.fsyncobjectfiles: performance tests for add and stash Neeraj Singh via GitGitGadget
2021-09-24 20:12       ` [PATCH v5 0/7] Implement a batched fsync option for core.fsyncObjectFiles Neeraj K. Singh via GitGitGadget
2021-09-24 20:12         ` [PATCH v5 1/7] object-file.c: do not rename in a temp odb Neeraj Singh via GitGitGadget
2021-09-24 20:12         ` [PATCH v5 2/7] bulk-checkin: rename 'state' variable and separate 'plugged' boolean Neeraj Singh via GitGitGadget
2021-09-24 20:12         ` [PATCH v5 3/7] core.fsyncobjectfiles: batched disk flushes Neeraj Singh via GitGitGadget
2021-09-24 21:47           ` Neeraj Singh
2021-09-24 20:12         ` [PATCH v5 4/7] update-index: use the bulk-checkin infrastructure Neeraj Singh via GitGitGadget
2021-09-24 21:49           ` Neeraj Singh
2021-09-24 20:12         ` [PATCH v5 5/7] unpack-objects: " Neeraj Singh via GitGitGadget
2021-09-24 20:12         ` [PATCH v5 6/7] core.fsyncobjectfiles: tests for batch mode Neeraj Singh via GitGitGadget
2021-09-24 20:12         ` [PATCH v5 7/7] core.fsyncobjectfiles: performance tests for add and stash Neeraj Singh via GitGitGadget
2021-09-24 23:31         ` [PATCH v5 0/7] Implement a batched fsync option for core.fsyncObjectFiles Neeraj Singh
2021-09-24 23:53         ` [PATCH v6 0/8] " Neeraj K. Singh via GitGitGadget
2021-09-24 23:53           ` [PATCH v6 1/8] object-file.c: do not rename in a temp odb Neeraj Singh via GitGitGadget
2021-09-24 23:53           ` [PATCH v6 2/8] bulk-checkin: rename 'state' variable and separate 'plugged' boolean Neeraj Singh via GitGitGadget
2021-09-24 23:53           ` [PATCH v6 3/8] core.fsyncobjectfiles: batched disk flushes Neeraj Singh via GitGitGadget
2021-09-25  3:15             ` Bagas Sanjaya
2021-09-27  0:27               ` Neeraj Singh
2021-09-24 23:53           ` [PATCH v6 4/8] core.fsyncobjectfiles: add windows support for batch mode Neeraj Singh via GitGitGadget
2021-09-27 20:07             ` Junio C Hamano
2021-09-27 20:55               ` Neeraj Singh
2021-09-27 21:03                 ` Neeraj Singh
2021-09-27 23:53                   ` Junio C Hamano
2021-09-24 23:53           ` [PATCH v6 5/8] update-index: use the bulk-checkin infrastructure Neeraj Singh via GitGitGadget
2021-09-24 23:53           ` [PATCH v6 6/8] unpack-objects: " Neeraj Singh via GitGitGadget
2021-09-24 23:53           ` [PATCH v6 7/8] core.fsyncobjectfiles: tests for batch mode Neeraj Singh via GitGitGadget
2021-09-24 23:53           ` [PATCH v6 8/8] core.fsyncobjectfiles: performance tests for add and stash Neeraj Singh via GitGitGadget
2021-09-28 23:32           ` [PATCH v7 0/9] Implement a batched fsync option for core.fsyncObjectFiles Neeraj K. Singh via GitGitGadget
2021-09-28 23:32             ` [PATCH v7 1/9] object-file.c: do not rename in a temp odb Neeraj Singh via GitGitGadget
2021-09-28 23:55               ` Jeff King
2021-09-29  0:10                 ` Neeraj Singh
2021-09-28 23:32             ` [PATCH v7 2/9] tmp-objdir: new API for creating temporary writable databases Neeraj Singh via GitGitGadget
2021-09-29  8:41               ` Elijah Newren
2021-09-29 16:40                 ` Neeraj Singh
2021-09-28 23:32             ` [PATCH v7 3/9] bulk-checkin: rename 'state' variable and separate 'plugged' boolean Neeraj Singh via GitGitGadget
2021-09-28 23:32             ` [PATCH v7 4/9] core.fsyncobjectfiles: batched disk flushes Neeraj Singh via GitGitGadget
2021-09-28 23:32             ` [PATCH v7 5/9] core.fsyncobjectfiles: add windows support for batch mode Neeraj Singh via GitGitGadget
2021-09-28 23:32             ` [PATCH v7 6/9] update-index: use the bulk-checkin infrastructure Neeraj Singh via GitGitGadget
2021-09-28 23:32             ` [PATCH v7 7/9] unpack-objects: " Neeraj Singh via GitGitGadget
2021-09-28 23:32             ` [PATCH v7 8/9] core.fsyncobjectfiles: tests for batch mode Neeraj Singh via GitGitGadget
2021-09-28 23:32             ` [PATCH v7 9/9] core.fsyncobjectfiles: performance tests for add and stash Neeraj Singh via GitGitGadget
2021-10-04 16:57             ` [PATCH v8 0/9] Implement a batched fsync option for core.fsyncObjectFiles Neeraj K. Singh via GitGitGadget
2021-10-04 16:57               ` [PATCH v8 1/9] tmp-objdir: new API for creating temporary writable databases Neeraj Singh via GitGitGadget
2021-10-04 16:57               ` [PATCH v8 2/9] tmp-objdir: disable ref updates when replacing the primary odb Neeraj Singh via GitGitGadget
2021-10-04 16:57               ` [PATCH v8 3/9] bulk-checkin: rename 'state' variable and separate 'plugged' boolean Neeraj Singh via GitGitGadget
2021-10-04 16:57               ` [PATCH v8 4/9] core.fsyncobjectfiles: batched disk flushes Neeraj Singh via GitGitGadget
2021-10-04 16:57               ` [PATCH v8 5/9] core.fsyncobjectfiles: add windows support for batch mode Neeraj Singh via GitGitGadget
2021-10-04 16:57               ` [PATCH v8 6/9] update-index: use the bulk-checkin infrastructure Neeraj Singh via GitGitGadget
2021-10-04 16:57               ` [PATCH v8 7/9] unpack-objects: " Neeraj Singh via GitGitGadget
2021-10-04 16:57               ` [PATCH v8 8/9] core.fsyncobjectfiles: tests for batch mode Neeraj Singh via GitGitGadget
2021-10-04 16:57               ` [PATCH v8 9/9] core.fsyncobjectfiles: performance tests for add and stash Neeraj Singh via GitGitGadget
2021-11-15 23:50               ` [PATCH v9 0/9] Implement a batched fsync option for core.fsyncObjectFiles Neeraj K. Singh via GitGitGadget
2021-11-15 23:50                 ` [PATCH v9 1/9] tmp-objdir: new API for creating temporary writable databases Neeraj Singh via GitGitGadget
2021-11-30 21:27                   ` Elijah Newren
2021-11-30 21:52                     ` Neeraj Singh
2021-11-30 22:36                       ` Elijah Newren
2021-11-15 23:50                 ` [PATCH v9 2/9] tmp-objdir: disable ref updates when replacing the primary odb Neeraj Singh via GitGitGadget
2021-11-16  7:23                   ` Ævar Arnfjörð Bjarmason
2021-11-16 20:38                     ` Neeraj Singh
2021-11-15 23:50                 ` [PATCH v9 3/9] bulk-checkin: rename 'state' variable and separate 'plugged' boolean Neeraj Singh via GitGitGadget
2021-11-15 23:50                 ` [PATCH v9 4/9] core.fsyncobjectfiles: batched disk flushes Neeraj Singh via GitGitGadget
2021-11-15 23:50                 ` [PATCH v9 5/9] core.fsyncobjectfiles: add windows support for batch mode Neeraj Singh via GitGitGadget
2021-11-15 23:51                 ` [PATCH v9 6/9] update-index: use the bulk-checkin infrastructure Neeraj Singh via GitGitGadget
2021-11-15 23:51                 ` [PATCH v9 7/9] unpack-objects: " Neeraj Singh via GitGitGadget
2021-11-15 23:51                 ` [PATCH v9 8/9] core.fsyncobjectfiles: tests for batch mode Neeraj Singh via GitGitGadget
2021-11-15 23:51                 ` [PATCH v9 9/9] core.fsyncobjectfiles: performance tests for add and stash Neeraj Singh via GitGitGadget
2021-11-16  8:02                 ` [PATCH v9 0/9] Implement a batched fsync option for core.fsyncObjectFiles Ævar Arnfjörð Bjarmason
2021-11-17  7:06                   ` Neeraj Singh
2021-11-17  7:24                     ` Ævar Arnfjörð Bjarmason
2021-11-18  5:03                       ` Neeraj Singh
2021-12-01 14:15                         ` Ævar Arnfjörð Bjarmason
2022-03-09 23:02                           ` Ævar Arnfjörð Bjarmason
2022-03-10  1:16                             ` Neeraj Singh
2022-03-10 14:01                               ` Ævar Arnfjörð Bjarmason
2022-03-10 17:52                                 ` Neeraj Singh
2022-03-10 18:08                                   ` rsbecker
2022-03-10 18:43                                     ` Neeraj Singh
2022-03-10 18:48                                       ` rsbecker

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CANQDOdc8F7a3ZeTDpUWrt8uUntnX4jHYxyj96SPwH-P=kMrneg@mail.gmail.com' \
    --to=nksingh85@gmail.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=gitster@pobox.com \
    --cc=hch@lst.de \
    --cc=jeffhost@microsoft.com \
    --cc=neerajsi@microsoft.com \
    --cc=peff@peff.net \
    --cc=rsbecker@nexbridge.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).