Re: [PATCH v2 13/21] packed_ref_cache: keep the `packed-refs` file mmapped if possible
Michael Haggerty writes: >> +struct stat st; >> +size_t size, bytes_read; > > Coverity helpfully pointed out that `bytes_read` has to be signed: > `ssize_t`. I'll fix that in the next round after waiting for other comments. Thanks. The macOS build at Travis also was also upset about it, so I'd tentatively queue a trivial squash on top.
Re: [PATCH v2 13/21] packed_ref_cache: keep the `packed-refs` file mmapped if possible
On 09/20/2017 08:40 PM, Jeff King wrote:
> [...]
> The overall strategy for this compile-time knob makes sense, but one
> thing confused me:
>
>> +ifdef MMAP_PREVENTS_DELETE
>> +BASIC_CFLAGS += -DMMAP_PREVENTS_DELETE
>> +else
>> +ifdef USE_WIN32_MMAP
>> +BASIC_CFLAGS += -DMMAP_PREVENTS_DELETE
>> +endif
>> +endif
>
> So setting the knob does what you'd expect. But if you don't set it,
> then we still auto-tweak it based on the USE_WIN32_MMAP knob. Do we need
> that? It seems like we set our new knob in config.mak.uname any time
> we'd set USE_WIN32_MMAP. So this only has an effect in two cases:
>
> 1. You aren't on Windows, but you set USE_WIN32_MMAP yourself.
>
> 2. You are on Windows, but you manually unset MMAP_PREVENTS_DELETE.
>
> I expect both cases are rare (and would probably involve somebody
> actively debugging these knobs). Probably it's a minor convenience in
> case 1, but in case 2 it would be actively confusing, I'd think.
Makes sense. I'll delete the `else` clause from the above hunk.
On 09/20/2017 08:51 PM, Jeff King wrote:
> On Wed, Sep 20, 2017 at 02:40:58PM -0400, Jeff King wrote:
>
>>> +enum mmap_strategy {
>>> + /*
>>> +* Don't use mmap() at all for reading `packed-refs`.
>>> +*/
>>> + MMAP_NONE,
>>> +
>>> + /*
>>> +* Can use mmap() for reading `packed-refs`, but the file must
>>> +* not remain mmapped. This is the usual option on Windows,
>>> +* where you cannot rename a new version of a file onto a file
>>> +* that is currently mmapped.
>>> +*/
>>> + MMAP_TEMPORARY,
>>
>> I suspect you originally distinguished these cases so that NO_MMAP does
>> not read into a fake-mmap buffer, followed by us copying it into another
>> buffer. But AFAICT we handle the "NONE" and "TEMPORARY" cases exactly
>> the same (by just doing a read_in_full() into our own buffer). Do we
>> actually need separate strategies?
>
> In case you are reading these sequentially, I think I talked myself into
> the utility of this during the next patch. ;)
Sorry about that. I'll add a forward breadcrumb to the log message of
this commit.
Michael
Re: [PATCH v2 13/21] packed_ref_cache: keep the `packed-refs` file mmapped if possible
On Wed, Sep 20, 2017 at 02:40:58PM -0400, Jeff King wrote:
> > +enum mmap_strategy {
> > + /*
> > +* Don't use mmap() at all for reading `packed-refs`.
> > +*/
> > + MMAP_NONE,
> > +
> > + /*
> > +* Can use mmap() for reading `packed-refs`, but the file must
> > +* not remain mmapped. This is the usual option on Windows,
> > +* where you cannot rename a new version of a file onto a file
> > +* that is currently mmapped.
> > +*/
> > + MMAP_TEMPORARY,
>
> I suspect you originally distinguished these cases so that NO_MMAP does
> not read into a fake-mmap buffer, followed by us copying it into another
> buffer. But AFAICT we handle the "NONE" and "TEMPORARY" cases exactly
> the same (by just doing a read_in_full() into our own buffer). Do we
> actually need separate strategies?
In case you are reading these sequentially, I think I talked myself into
the utility of this during the next patch. ;)
-Peff
Re: [PATCH v2 13/21] packed_ref_cache: keep the `packed-refs` file mmapped if possible
On Tue, Sep 19, 2017 at 08:22:21AM +0200, Michael Haggerty wrote:
> Keep a copy of the `packed-refs` file contents in memory for as long
> as a `packed_ref_cache` object is in use:
>
> * If the system allows it, keep the `packed-refs` file mmapped.
>
> * If not (either because the system doesn't support `mmap()` at all,
> or because a file that is currently mmapped cannot be replaced via
> `rename()`), then make a copy of the file's contents in
> heap-allocated space, and keep that around instead.
>
> We base the choice of behavior on a new build-time switch,
> `MMAP_PREVENTS_DELETE`. By default, this switch is set for Windows
> variants.
>
> This whole change is still pointless, because we only read the
> `packed-refs` file contents immediately after instantiating the
> `packed_ref_cache`. But that will soon change.
The overall strategy for this compile-time knob makes sense, but one
thing confused me:
> +ifdef MMAP_PREVENTS_DELETE
> + BASIC_CFLAGS += -DMMAP_PREVENTS_DELETE
> +else
> + ifdef USE_WIN32_MMAP
> + BASIC_CFLAGS += -DMMAP_PREVENTS_DELETE
> + endif
> +endif
So setting the knob does what you'd expect. But if you don't set it,
then we still auto-tweak it based on the USE_WIN32_MMAP knob. Do we need
that? It seems like we set our new knob in config.mak.uname any time
we'd set USE_WIN32_MMAP. So this only has an effect in two cases:
1. You aren't on Windows, but you set USE_WIN32_MMAP yourself.
2. You are on Windows, but you manually unset MMAP_PREVENTS_DELETE.
I expect both cases are rare (and would probably involve somebody
actively debugging these knobs). Probably it's a minor convenience in
case 1, but in case 2 it would be actively confusing, I'd think.
> +enum mmap_strategy {
> + /*
> + * Don't use mmap() at all for reading `packed-refs`.
> + */
> + MMAP_NONE,
> +
> + /*
> + * Can use mmap() for reading `packed-refs`, but the file must
> + * not remain mmapped. This is the usual option on Windows,
> + * where you cannot rename a new version of a file onto a file
> + * that is currently mmapped.
> + */
> + MMAP_TEMPORARY,
I suspect you originally distinguished these cases so that NO_MMAP does
not read into a fake-mmap buffer, followed by us copying it into another
buffer. But AFAICT we handle the "NONE" and "TEMPORARY" cases exactly
the same (by just doing a read_in_full() into our own buffer). Do we
actually need separate strategies?
-Peff
Re: [PATCH v2 13/21] packed_ref_cache: keep the `packed-refs` file mmapped if possible
On 09/19/2017 08:22 AM, Michael Haggerty wrote:
> Keep a copy of the `packed-refs` file contents in memory for as long
> as a `packed_ref_cache` object is in use:
>
> * If the system allows it, keep the `packed-refs` file mmapped.
>
> * If not (either because the system doesn't support `mmap()` at all,
> or because a file that is currently mmapped cannot be replaced via
> `rename()`), then make a copy of the file's contents in
> heap-allocated space, and keep that around instead.
>
> We base the choice of behavior on a new build-time switch,
> `MMAP_PREVENTS_DELETE`. By default, this switch is set for Windows
> variants.
>
> This whole change is still pointless, because we only read the
> `packed-refs` file contents immediately after instantiating the
> `packed_ref_cache`. But that will soon change.
>
> Signed-off-by: Michael Haggerty
> ---
> Makefile | 10 +++
> config.mak.uname | 3 +
> refs/packed-backend.c | 184
> ++
> 3 files changed, 155 insertions(+), 42 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index f2bb7f2f63..7a49f99c4f 100644
> [...]
> diff --git a/refs/packed-backend.c b/refs/packed-backend.c
> index 0fe41a7203..4981516f1e 100644
> --- a/refs/packed-backend.c
> +++ b/refs/packed-backend.c
> [...]
> @@ -304,6 +371,61 @@ struct ref_iterator *mmapped_ref_iterator_begin(
> return ref_iterator;
> }
>
> +/*
> + * Depending on `mmap_strategy`, either mmap or read the contents of
> + * the `packed-refs` file into the `packed_refs` instance. Return 1 if
> + * the file existed and was read, or 0 if the file was absent. Die on
> + * errors.
> + */
> +static int load_contents(struct packed_ref_cache *packed_refs)
> +{
> + int fd;
> + struct stat st;
> + size_t size, bytes_read;
Coverity helpfully pointed out that `bytes_read` has to be signed:
`ssize_t`. I'll fix that in the next round after waiting for other comments.
> [...]
Michael
[PATCH v2 13/21] packed_ref_cache: keep the `packed-refs` file mmapped if possible
Keep a copy of the `packed-refs` file contents in memory for as long
as a `packed_ref_cache` object is in use:
* If the system allows it, keep the `packed-refs` file mmapped.
* If not (either because the system doesn't support `mmap()` at all,
or because a file that is currently mmapped cannot be replaced via
`rename()`), then make a copy of the file's contents in
heap-allocated space, and keep that around instead.
We base the choice of behavior on a new build-time switch,
`MMAP_PREVENTS_DELETE`. By default, this switch is set for Windows
variants.
This whole change is still pointless, because we only read the
`packed-refs` file contents immediately after instantiating the
`packed_ref_cache`. But that will soon change.
Signed-off-by: Michael Haggerty
---
Makefile | 10 +++
config.mak.uname | 3 +
refs/packed-backend.c | 184 ++
3 files changed, 155 insertions(+), 42 deletions(-)
diff --git a/Makefile b/Makefile
index f2bb7f2f63..7a49f99c4f 100644
--- a/Makefile
+++ b/Makefile
@@ -200,6 +200,9 @@ all::
#
# Define NO_MMAP if you want to avoid mmap.
#
+# Define MMAP_PREVENTS_DELETE if a file that is currently mmapped cannot be
+# deleted or cannot be replaced using rename().
+#
# Define NO_SYS_POLL_H if you don't have sys/poll.h.
#
# Define NO_POLL if you do not have or don't want to use poll().
@@ -1383,6 +1386,13 @@ else
COMPAT_OBJS += compat/win32mmap.o
endif
endif
+ifdef MMAP_PREVENTS_DELETE
+ BASIC_CFLAGS += -DMMAP_PREVENTS_DELETE
+else
+ ifdef USE_WIN32_MMAP
+ BASIC_CFLAGS += -DMMAP_PREVENTS_DELETE
+ endif
+endif
ifdef OBJECT_CREATION_USES_RENAMES
COMPAT_CFLAGS += -DOBJECT_CREATION_MODE=1
endif
diff --git a/config.mak.uname b/config.mak.uname
index 6604b130f8..685a80d138 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -184,6 +184,7 @@ ifeq ($(uname_O),Cygwin)
UNRELIABLE_FSTAT = UnfortunatelyYes
SPARSE_FLAGS = -isystem /usr/include/w32api -Wno-one-bit-signed-bitfield
OBJECT_CREATION_USES_RENAMES = UnfortunatelyNeedsTo
+ MMAP_PREVENTS_DELETE = UnfortunatelyYes
COMPAT_OBJS += compat/cygwin.o
FREAD_READS_DIRECTORIES = UnfortunatelyYes
endif
@@ -353,6 +354,7 @@ ifeq ($(uname_S),Windows)
NO_ST_BLOCKS_IN_STRUCT_STAT = YesPlease
NO_NSEC = YesPlease
USE_WIN32_MMAP = YesPlease
+ MMAP_PREVENTS_DELETE = UnfortunatelyYes
# USE_NED_ALLOCATOR = YesPlease
UNRELIABLE_FSTAT = UnfortunatelyYes
OBJECT_CREATION_USES_RENAMES = UnfortunatelyNeedsTo
@@ -501,6 +503,7 @@ ifneq (,$(findstring MINGW,$(uname_S)))
NO_ST_BLOCKS_IN_STRUCT_STAT = YesPlease
NO_NSEC = YesPlease
USE_WIN32_MMAP = YesPlease
+ MMAP_PREVENTS_DELETE = UnfortunatelyYes
USE_NED_ALLOCATOR = YesPlease
UNRELIABLE_FSTAT = UnfortunatelyYes
OBJECT_CREATION_USES_RENAMES = UnfortunatelyNeedsTo
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index 0fe41a7203..4981516f1e 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -7,6 +7,35 @@
#include "../iterator.h"
#include "../lockfile.h"
+enum mmap_strategy {
+ /*
+* Don't use mmap() at all for reading `packed-refs`.
+*/
+ MMAP_NONE,
+
+ /*
+* Can use mmap() for reading `packed-refs`, but the file must
+* not remain mmapped. This is the usual option on Windows,
+* where you cannot rename a new version of a file onto a file
+* that is currently mmapped.
+*/
+ MMAP_TEMPORARY,
+
+ /*
+* It is OK to leave the `packed-refs` file mmapped while
+* arbitrary other code is running.
+*/
+ MMAP_OK
+};
+
+#if defined(NO_MMAP)
+static enum mmap_strategy mmap_strategy = MMAP_NONE;
+#elif defined(MMAP_PREVENTS_DELETE)
+static enum mmap_strategy mmap_strategy = MMAP_TEMPORARY;
+#else
+static enum mmap_strategy mmap_strategy = MMAP_OK;
+#endif
+
struct packed_ref_store;
struct packed_ref_cache {
@@ -18,6 +47,21 @@ struct packed_ref_cache {
struct ref_cache *cache;
+ /* Is the `packed-refs` file currently mmapped? */
+ int mmapped;
+
+ /*
+* The contents of the `packed-refs` file. If the file is
+* mmapped, this points at the mmapped contents of the file.
+* If not, this points at heap-allocated memory containing the
+* contents. If there were no contents (e.g., because the file
+* didn't exist), `buf` and `eof` are both NULL.
+*/
+ char *buf, *eof;
+
+ /* The size of the header line, if any; otherwise, 0: */
+ size_t header_len;
+
/*
* What is the peeled state of this cache? (This is usually
* determined from the header of the "packed-refs" file.)
@@ -76,6 +120,26 @@ static void acquire_packed_ref_cache(struct
packed_ref_cache *packed_refs)
packed_refs->
