public inbox for git@vger.kernel.org 
 help / color / mirror / Atom feed

From: Junio C Hamano <gitster@pobox•com>
To: Kevin Willford <kewillf@microsoft•com>
Cc: git@vger•kernel.org, peff@peff•net, peartben@gmail•com
Subject: Re: [PATCH 3/3] read-cache: avoid allocating every ondisk entry when writing
Date: Mon, 21 Aug 2017 16:42:16 -0700	[thread overview]
Message-ID: <xmqqwp5w4hev.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <20170821212432.47364-4-kewillf@microsoft.com> (Kevin Willford's message of "Mon, 21 Aug 2017 15:24:32 -0600")

Kevin Willford <kewillf@microsoft•com> writes:

> When writing the index for each entry an ondisk struct will be
> allocated and freed in ce_write_entry.  We can do better by
> using a ondisk struct on the stack for each entry.
>
> This is accomplished by using a stack ondisk_cache_entry_extended
> outside looping through the entries in do_write_index.  Only the
> fixed fields of this struct are used when writing and depending on
> whether it is extended or not the flags2 field will be written.
> The name field is not used and instead the cache_entry name field
> is used directly when writing out the name.  Because ce_write is
> using a buffer and memcpy to fill the buffer before flushing to disk,
> we don't have to worry about doing multiple ce_write calls.

The code already heavily assumes that the only difference between
ondisk_cache_entry and its _extended variant are identical in their
earlier parts, so having a single instance of the larger one on the
stack and passing it around makes sense as an optimization.

Multiple chomped calls to ce_write() does make me nervous, not due
to potential I/O cost (which is none) but to potential programming
mistakes for people who touch the code.  The version immediately
after this patch looks correct in that once "result" is set to non
zero to indicate an error nothing is written out, but keeping that
property over time, when a newer index format is eventually invented,
may become an additional burden.

But overall, I think this is a good change.  Will queue all three.

Thanks.

     prev parent reply	other threads:[~2017-08-21 23:42 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-08-21 21:24 [PATCH 0/3] read-cache: use stack ondisk struct when writing index Kevin Willford
2017-08-21 21:24 ` [PATCH 1/3] perf: add test for writing the index Kevin Willford
2017-08-21 21:24 ` [PATCH 2/3] read-cache: fix memory leak in do_write_index Kevin Willford
2017-08-21 23:02   ` Junio C Hamano
2017-08-21 21:24 ` [PATCH 3/3] read-cache: avoid allocating every ondisk entry when writing Kevin Willford
2017-08-21 23:42   ` Junio C Hamano [this message]

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

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

  git send-email \
    --in-reply-to=xmqqwp5w4hev.fsf@gitster.mtv.corp.google.com \
    --to=gitster@pobox$(echo .)com \
    --cc=git@vger$(echo .)kernel.org \
    --cc=kewillf@microsoft$(echo .)com \
    --cc=peartben@gmail$(echo .)com \
    --cc=peff@peff$(echo .)net \
    /path/to/YOUR_REPLY

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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox