On Thu, Jan 20 2022, Han Xin wrote: > From: Han Xin <hanxin.hx@xxxxxxxxxxxxxxx> > > Changes since v8: > * Rename "assert_no_loose ()" into "test_no_loose ()" in > "t5329-unpack-large-objects.sh". Remove "assert_no_pack ()" and use > "test_dir_is_empty" instead. > > * Revert changes to "create_tmpfile()" and error handling is now in > "start_loose_object_common()". > > * Remove "finalize_object_file_with_mtime()" which seems to be an overkill > for "write_loose_object()" now. > > * Remove the commit "object-file.c: remove the slash for directory_size()", > it can be in a separate patch if necessary. > > Han Xin (4): > unpack-objects: low memory footprint for get_data() in dry_run mode > object-file.c: refactor write_loose_object() to several steps > object-file.c: add "stream_loose_object()" to handle large object > unpack-objects: unpack_non_delta_entry() read data in a stream > > Ævar Arnfjörð Bjarmason (1): > object-file API: add a format_object_header() function I sent https://lore.kernel.org/git/cover-00.10-00000000000-20220201T144803Z-avarab@xxxxxxxxx/ today which suggests splitting out the 5/5 cleanup you'd integrated. I then rebased these patches of yours on top of that, the result is here: https://github.com/avar/git/tree/han-xin-avar/unpack-loose-object-streaming-9 The range-diff to your version is below. There's a few unrelated fixes/nits in it. I think with/without basing this on top of my series above your patches here look good with the nits pointed out in the diff below addressed (and some don't need to be). I.e. the dependency on it is rather trivial, and the two could be split up. What do you think is a good way to proceed? I could just submit the below as a proposed v10 if you'd like & agree... 1: 553a9377eb3 ! 1: 61fcfe7b840 unpack-objects: low memory footprint for get_data() in dry_run mode @@ Commit message unpack-objects: low memory footprint for get_data() in dry_run mode As the name implies, "get_data(size)" will allocate and return a given - size of memory. Allocating memory for a large blob object may cause the + amount of memory. Allocating memory for a large blob object may cause the system to run out of memory. Before preparing to replace calling of "get_data()" to unpack large blob objects in latter commits, refactor "get_data()" to reduce memory footprint for dry_run mode. @@ Commit message Suggested-by: Jiang Xin <zhiyou.jx@xxxxxxxxxxxxxxx> Signed-off-by: Han Xin <hanxin.hx@xxxxxxxxxxxxxxx> + Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> ## builtin/unpack-objects.c ## @@ builtin/unpack-objects.c: static void use(int bytes) @@ t/t5328-unpack-large-objects.sh (new) + +test_no_loose () { + glob=dest.git/objects/?? && -+ echo "$glob" >expect && -+ eval "echo $glob" >actual && ++ echo $glob >expect && ++ echo "$glob" >actual && + test_cmp expect actual +} + -: ----------- > 2: c6b0437db03 object-file.c: do fsync() and close() before post-write die() 2: 88c91affd61 ! 3: 77bcfe3da6f object-file.c: refactor write_loose_object() to several steps @@ Commit message When writing a large blob using "write_loose_object()", we have to pass a buffer with the whole content of the blob, and this behavior will consume lots of memory and may cause OOM. We will introduce a stream - version function ("stream_loose_object()") in latter commit to resolve + version function ("stream_loose_object()") in later commit to resolve this issue. - Before introducing a stream vesion function for writing loose object, - do some refactoring on "write_loose_object()" to reuse code for both - versions. + Before introducing that streaming function, do some refactoring on + "write_loose_object()" to reuse code for both versions. Rewrite "write_loose_object()" as follows: @@ Commit message 3. Compress data. - 4. Move common steps for ending zlib stream into a new funciton + 4. Move common steps for ending zlib stream into a new function "end_loose_object_common()". 5. Close fd and finalize the object file. @@ Commit message Helped-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> Helped-by: Jiang Xin <zhiyou.jx@xxxxxxxxxxxxxxx> Signed-off-by: Han Xin <hanxin.hx@xxxxxxxxxxxxxxx> + Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> ## object-file.c ## @@ object-file.c: static int create_tmpfile(struct strbuf *tmp, const char *filename) return fd; } ++/** ++ * Common steps for loose object writers to start writing loose ++ * objects: ++ * ++ * - Create tmpfile for the loose object. ++ * - Setup zlib stream for compression. ++ * - Start to feed header to zlib stream. ++ * ++ * Returns a "fd", which should later be provided to ++ * end_loose_object_common(). ++ */ +static int start_loose_object_common(struct strbuf *tmp_file, + const char *filename, unsigned flags, + git_zstream *stream, @@ object-file.c: static int create_tmpfile(struct strbuf *tmp, const char *filenam + return fd; +} + -+static void end_loose_object_common(int ret, git_hash_ctx *c, ++/** ++ * Common steps for loose object writers to end writing loose objects: ++ * ++ * - End the compression of zlib stream. ++ * - Get the calculated oid to "parano_oid". ++ * - fsync() and close() the "fd" ++ */ ++static void end_loose_object_common(int fd, int ret, git_hash_ctx *c, + git_zstream *stream, + struct object_id *parano_oid, + const struct object_id *expected_oid, @@ object-file.c: static int create_tmpfile(struct strbuf *tmp, const char *filenam + if (ret != Z_OK) + die(_(die_msg2_fmt), ret, expected_oid); + the_hash_algo->final_oid_fn(parano_oid, c); ++ ++ /* ++ * We already did a write_buffer() to the "fd", let's fsync() ++ * and close(). ++ * ++ * We might still die() on a subsequent sanity check, but ++ * let's not add to that confusion by not flushing any ++ * outstanding writes to disk first. ++ */ ++ close_loose_object(fd); +} + static int write_loose_object(const struct object_id *oid, char *hdr, @@ object-file.c: static int write_loose_object(const struct object_id *oid, char * - while (git_deflate(&stream, 0) == Z_OK) - ; /* nothing */ - the_hash_algo->update_fn(&c, hdr, hdrlen); -+ /* Common steps for write_loose_object and stream_loose_object to -+ * start writing loose oject: -+ * -+ * - Create tmpfile for the loose object. -+ * - Setup zlib stream for compression. -+ * - Start to feed header to zlib stream. -+ */ + fd = start_loose_object_common(&tmp_file, filename.buf, flags, + &stream, compressed, sizeof(compressed), + &c, hdr, hdrlen); @@ object-file.c: static int write_loose_object(const struct object_id *oid, char * - die(_("deflateEnd on object %s failed (%d)"), oid_to_hex(oid), - ret); - the_hash_algo->final_oid_fn(¶no_oid, &c); -+ /* Common steps for write_loose_object and stream_loose_object to -+ * end writing loose oject: -+ * -+ * - End the compression of zlib stream. -+ * - Get the calculated oid to "parano_oid". -+ */ -+ end_loose_object_common(ret, &c, &stream, ¶no_oid, oid, +- +- /* +- * We already did a write_buffer() to the "fd", let's fsync() +- * and close(). +- * +- * We might still die() on a subsequent sanity check, but +- * let's not add to that confusion by not flushing any +- * outstanding writes to disk first. +- */ +- close_loose_object(fd); ++ end_loose_object_common(fd, ret, &c, &stream, ¶no_oid, oid, + N_("unable to deflate new object %s (%d)"), + N_("deflateEnd on object %s failed (%d)")); -+ + if (!oideq(oid, ¶no_oid)) die(_("confused by unstable object source data for %s"), - oid_to_hex(oid)); 3: 054a00ed21d ! 4: 71c10e734d1 object-file.c: add "stream_loose_object()" to handle large object @@ Commit message Add a new function "stream_loose_object()", which is a stream version of "write_loose_object()" but with a low memory footprint. We will use this - function to unpack large blob object in latter commit. + function to unpack large blob object in later commit. Another difference with "write_loose_object()" is that we have no chance to run "write_object_file_prepare()" to calculate the oid in advance. In "write_loose_object()", we know the oid and we can write the temporary file in the same directory as the final object, but for an object with an undetermined oid, we don't know the exact directory for - the object, so we have to save the temporary file in ".git/objects/" - directory instead. + the object. + + Still, we need to save the temporary file we're preparing + somewhere. We'll do that in the top-level ".git/objects/" + directory (or whatever "GIT_OBJECT_DIRECTORY" is set to). Once we've + streamed it we'll know the OID, and will move it to its canonical + path. "freshen_packed_object()" or "freshen_loose_object()" will be called inside "stream_loose_object()" after obtaining the "oid". @@ object-file.c: static int freshen_packed_object(const struct object_id *oid) + + /* Since oid is not determined, save tmp file to odb path. */ + strbuf_addf(&filename, "%s/", get_object_directory()); -+ hdrlen = xsnprintf(hdr, sizeof(hdr), "%s %"PRIuMAX, type_name(OBJ_BLOB), len) + 1; ++ hdrlen = format_object_header(hdr, sizeof(hdr), OBJ_BLOB, len); + + /* Common steps for write_loose_object and stream_loose_object to + * start writing loose oject: @@ object-file.c: static int freshen_packed_object(const struct object_id *oid) + * - End the compression of zlib stream. + * - Get the calculated oid. + */ -+ end_loose_object_common(ret, &c, &stream, oid, NULL, ++ end_loose_object_common(fd, ret, &c, &stream, oid, NULL, + N_("unable to stream deflate new object (%d)"), + N_("deflateEnd on stream object failed (%d)")); + -+ close_loose_object(fd); -+ + if (freshen_packed_object(oid) || freshen_loose_object(oid)) { + unlink_or_warn(tmp_file.buf); + goto cleanup; @@ object-file.c: static int freshen_packed_object(const struct object_id *oid) +} + int write_object_file_flags(const void *buf, unsigned long len, - const char *type, struct object_id *oid, + enum object_type type, struct object_id *oid, unsigned flags) ## object-store.h ## @@ object-store.h: static inline int write_object_file(const void *buf, unsigned lo +int stream_loose_object(struct input_stream *in_stream, size_t len, + struct object_id *oid); -+ - int hash_object_file_literally(const void *buf, unsigned long len, - const char *type, struct object_id *oid, - unsigned flags); + int hash_write_object_file_literally(const void *buf, unsigned long len, + const char *type, struct object_id *oid, + unsigned flags); -: ----------- > 5: 3c1d788d69d core doc: modernize core.bigFileThreshold documentation 4: 6bcba6bce66 ! 6: 8b83f6d6b83 unpack-objects: unpack_non_delta_entry() read data in a stream @@ Metadata Author: Han Xin <hanxin.hx@xxxxxxxxxxxxxxx> ## Commit message ## - unpack-objects: unpack_non_delta_entry() read data in a stream + unpack-objects: use stream_loose_object() to unpack large objects - We used to call "get_data()" in "unpack_non_delta_entry()" to read the - entire contents of a blob object, no matter how big it is. This - implementation may consume all the memory and cause OOM. + Make use of the stream_loose_object() function introduced in the + preceding commit to unpack large objects. Before this we'd need to + malloc() the size of the blob before unpacking it, which could cause + OOM with very large blobs. - By implementing a zstream version of input_stream interface, we can use - a small fixed buffer for "unpack_non_delta_entry()". However, unpack - non-delta objects from a stream instead of from an entrie buffer will - have 10% performance penalty. + We could use this new interface to unpack all blobs, but doing so + would result in a performance penalty of around 10%, as the below + "hyperfine" benchmark will show. We therefore limit this to files + larger than "core.bigFileThreshold": $ hyperfine \ --setup \ @@ Commit message -c core.bigFileThreshold=16k unpack-objects <small.pack' in 'HEAD~1' - Therefore, only unpack objects larger than the "core.bigFileThreshold" - in zstream. Until now, the config variable has been used in the - following cases, and our new case belongs to the packfile category. + An earlier version of this patch introduced a new + "core.bigFileStreamingThreshold" instead of re-using the existing + "core.bigFileThreshold" variable[1]. As noted in a detailed overview + of its users in [2] using it has several different meanings. - * Archive: + Still, we consider it good enough to simply re-use it. While it's + possible that someone might want to e.g. consider objects "small" for + the purposes of diffing but "big" for the purposes of writing them + such use-cases are probably too obscure to worry about. We can always + split up "core.bigFileThreshold" in the future if there's a need for + that. - + archive.c: write_entry(): write large blob entries to archive in - stream. - - * Loose objects: - - + object-file.c: index_fd(): when hashing large files in worktree, - read files in a stream, and create one packfile per large blob if - want to save files to git object store. - - + object-file.c: read_loose_object(): when checking loose objects - using "git-fsck", do not read full content of large loose objects. - - * Packfile: - - + fast-import.c: parse_and_store_blob(): streaming large blob from - foreign source to packfile. - - + index-pack.c: check_collison(): read and check large blob in stream. - - + index-pack.c: unpack_entry_data(): do not return the entire - contents of the big blob from packfile, but uses a fixed buf to - perform some integrity checks on the object. - - + pack-check.c: verify_packfile(): used by "git-fsck" and will call - check_object_signature() to check large blob in pack with the - streaming interface. - - + pack-objects.c: get_object_details(): set "no_try_delta" for large - blobs when counting objects. - - + pack-objects.c: write_no_reuse_object(): streaming large blob to - pack. - - + unpack-objects.c: unpack_non_delta_entry(): unpack large blob in - stream from packfile. - - * Others: - - + diff.c: diff_populate_filespec(): treat large blob file as binary. - - + streaming.c: istream_source(): as a helper of "open_istream()" to - select proper streaming interface to read large blob from packfile. + 1. https://lore.kernel.org/git/20211210103435.83656-1-chiyutianyi@xxxxxxxxx/ + 2. https://lore.kernel.org/git/20220120112114.47618-5-chiyutianyi@xxxxxxxxx/ Helped-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> Helped-by: Derrick Stolee <stolee@xxxxxxxxx> Helped-by: Jiang Xin <zhiyou.jx@xxxxxxxxxxxxxxx> Signed-off-by: Han Xin <hanxin.hx@xxxxxxxxxxxxxxx> + ## Documentation/config/core.txt ## +@@ Documentation/config/core.txt: usage, at the slight expense of increased disk usage. + * Will be generally be streamed when written, which avoids excessive + memory usage, at the cost of some fixed overhead. Commands that make + use of this include linkgit:git-archive[1], +-linkgit:git-fast-import[1], linkgit:git-index-pack[1] and +-linkgit:git-fsck[1]. ++linkgit:git-fast-import[1], linkgit:git-index-pack[1], ++linkgit:git-unpack-objects[1] and linkgit:git-fsck[1]. + + core.excludesFile:: + Specifies the pathname to the file that contains patterns to + ## builtin/unpack-objects.c ## @@ builtin/unpack-objects.c: static void added_object(unsigned nr, enum object_type type, } 5: 1bfaf89ee0b < -: ----------- object-file API: add a format_object_header() function
