Show 216 quoted lines
> The are many ways in which fast-import ends up calling gfi_unpack_entry,
> and fery few work-arounds. I've patched fast-import for it to be smarter
> in corner cases, allowing some additional work-arounds, but it's just
> too easy to fall on gfi_unpack_entry again, so I abandonned that path.
>
> The problem with gfi_unpack_entry is that if something has been written
> to the pack after last time it was used, it closes all pack windows. On
> OSX, this triggers munmap, which shows up in performance profiles.
>
> To give an idea how bad this is, here is how long it takes to clone
> https://hg.mozilla.org/mozilla-unified/ with the master branch of
> git-cinnabar (which uses fast-import) on a mac mini: more than 5 hours.
> I can't actually give the exact number, because it was killed, after
> spending 2 hours importing 1.77M files and 3 hours importing 120k
> manifests.
>
> The same clone, with a variant of this patch, *finished* in 2 hours and
> 10 minutes, spending 24 minutes importing the same 1.77M files and only
> 13 minutes to cover the same 120k manifests. It took an hour and 20
> minutes to cover the remaining 210k manifests. You can imagine how long
> it would have taken without the patch if it hadn't been killed...
>
> Now, this is proof of concept level. There are many things that are not
> right with this patch, starting from the fact it doesn't handle
> checkpoints, and isn't safe for every kind of integer overflows. Or
> malloc'ating exactly packed_git_window_size bytes (which on 64-bits
> systems, is 1GB), etc.
>
> But it feels to me this kind of fake pack window is a cheap way to
> counter the slowness of munmap on OSX, although the fact that it's a
> hack around the pack code in sha1_file.c is not very nice. Maybe a
> better start to fix the issue would be to add better interfaces to
> the pack code to handle pack writers that can read at the same time.
>
> Thoughts?
>
> Past related discussions:
> $gmane/291717
> $gmane/273465
> ---
> fast-import.c | 90 +++++++++++++++++++++++++++++++++++++++++------------------
> 1 file changed, 63 insertions(+), 27 deletions(-)
>
> diff --git a/fast-import.c b/fast-import.c
> index c504ef7..4e26883 100644
> --- a/fast-import.c
> +++ b/fast-import.c
> @@ -316,6 +316,7 @@ static struct atom_str **atom_table;
> static struct pack_idx_option pack_idx_opts;
> static unsigned int pack_id;
> static struct sha1file *pack_file;
> +static struct pack_window *pack_win;
> static struct packed_git *pack_data;
> static struct packed_git **all_packs;
> static off_t pack_size;
> @@ -862,6 +863,39 @@ static struct tree_content *dup_tree_content(struct tree_content *s)
> return d;
> }
>
> +static void _sha1write(struct sha1file *f, const void *buf, unsigned int count)
> +{
> + sha1write(f, buf, count);
> + /* Always last used */
> + pack_win->last_used = -1;
> + pack_win->inuse_cnt = -1;
> +
> + pack_data->pack_size += count;
> +
> + if (packed_git_window_size - pack_win->len >= count) {
> + memcpy(pack_win->base + pack_win->len - 20, buf, count);
> + pack_win->len += count;
> + } else {
> + struct pack_window *cursor = NULL;
> + /* We're sliding the window, so we don't need to memcpy
> + * everything. */
> + pack_win->offset += ((pack_win->len - 20 + count)
> + / packed_git_window_size) * packed_git_window_size;
> + pack_win->len = count % packed_git_window_size -
> + (packed_git_window_size - pack_win->len);
> + memcpy(pack_win->base, buf + count - pack_win->len + 20,
> + pack_win->len - 20);
> +
> + /* Ensure a pack window on the data before that, otherwise,
> + * use_pack() may try to create a window that overlaps with
> + * this one, and that won't work because it won't be complete. */
> + sha1flush(f);
> + use_pack(pack_data, &cursor,
> + pack_win->offset - packed_git_window_size, NULL);
> + unuse_pack(&cursor);
> + }
> +}
> +
> static void start_packfile(void)
> {
> static char tmp_file[PATH_MAX];
> @@ -873,15 +907,22 @@ static void start_packfile(void)
> "pack/tmp_pack_XXXXXX");
> FLEX_ALLOC_STR(p, pack_name, tmp_file);
> p->pack_fd = pack_fd;
> + p->pack_size = 20;
> p->do_not_close = 1;
> pack_file = sha1fd(pack_fd, p->pack_name);
>
> + p->windows = pack_win = xcalloc(1, sizeof(*p->windows));
> + pack_win->offset = 0;
> + pack_win->len = 20;
> + pack_win->base = xmalloc(packed_git_window_size);
> + pack_win->next = NULL;
> +
> hdr.hdr_signature = htonl(PACK_SIGNATURE);
> hdr.hdr_version = htonl(2);
> hdr.hdr_entries = 0;
> - sha1write(pack_file, &hdr, sizeof(hdr));
> -
> pack_data = p;
> + _sha1write(pack_file, &hdr, sizeof(hdr));
> +
> pack_size = sizeof(hdr);
> object_count = 0;
>
> @@ -954,10 +995,25 @@ static void unkeep_all_packs(void)
> static void end_packfile(void)
> {
> static int running;
> + struct pack_window *win, *prev;
>
> if (running || !pack_data)
> return;
>
> + /* Remove the fake pack window first */
> + for (prev = NULL, win = pack_data->windows; win;
> + prev = win, win = win->next) {
> + if (win != pack_win)
> + continue;
> + if (prev)
> + prev->next = win->next;
> + else
> + pack_data->windows = win->next;
> + break;
> + }
> + free(pack_win->base);
> + free(pack_win);
> +
> running = 1;
> clear_delta_base_cache();
> if (object_count) {
> @@ -1122,22 +1178,22 @@ static int store_object(
> e->depth = last->depth + 1;
>
> hdrlen = encode_in_pack_object_header(OBJ_OFS_DELTA, deltalen, hdr);
> - sha1write(pack_file, hdr, hdrlen);
> + _sha1write(pack_file, hdr, hdrlen);
> pack_size += hdrlen;
>
> hdr[pos] = ofs & 127;
> while (ofs >>= 7)
> hdr[--pos] = 128 | (--ofs & 127);
> - sha1write(pack_file, hdr + pos, sizeof(hdr) - pos);
> + _sha1write(pack_file, hdr + pos, sizeof(hdr) - pos);
> pack_size += sizeof(hdr) - pos;
> } else {
> e->depth = 0;
> hdrlen = encode_in_pack_object_header(type, dat->len, hdr);
> - sha1write(pack_file, hdr, hdrlen);
> + _sha1write(pack_file, hdr, hdrlen);
> pack_size += hdrlen;
> }
>
> - sha1write(pack_file, out, s.total_out);
> + _sha1write(pack_file, out, s.total_out);
> pack_size += s.total_out;
>
> e->idx.crc32 = crc32_end(pack_file);
> @@ -1220,7 +1276,7 @@ static void stream_blob(uintmax_t len, unsigned char *sha1out, uintmax_t mark)
>
> if (!s.avail_out || status == Z_STREAM_END) {
> size_t n = s.next_out - out_buf;
> - sha1write(pack_file, out_buf, n);
> + _sha1write(pack_file, out_buf, n);
> pack_size += n;
> s.next_out = out_buf;
> s.avail_out = out_sz;
> @@ -1295,26 +1351,6 @@ static void *gfi_unpack_entry(
> {
> enum object_type type;
> struct packed_git *p = all_packs[oe->pack_id];
> - if (p == pack_data && p->pack_size < (pack_size + 20)) {
> - /* The object is stored in the packfile we are writing to
> - * and we have modified it since the last time we scanned
> - * back to read a previously written object. If an old
> - * window covered [p->pack_size, p->pack_size + 20) its
> - * data is stale and is not valid. Closing all windows
> - * and updating the packfile length ensures we can read
> - * the newly written data.
> - */
> - close_pack_windows(p);
> - sha1flush(pack_file);
> -
> - /* We have to offer 20 bytes additional on the end of
> - * the packfile as the core unpacker code assumes the
> - * footer is present at the file end and must promise
> - * at least 20 bytes within any window it maps. But
> - * we don't actually create the footer here.
> - */
> - p->pack_size = pack_size + 20;
> - }
> return unpack_entry(p, oe->idx.offset, &type, sizep);
> }
>
> --
> 2.9.0.dirty
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.htmlMike Hommey <mh@glandium.org>