From: Han Xin <redacted>
When calling "unpack_non_delta_entry()", will allocate full memory for
the whole size of the unpacked object and write the buffer to loose file
on disk. This may lead to OOM for the git-unpack-objects process when
unpacking a very large object.
In function "unpack_delta_entry()", will also allocate full memory to
buffer the whole delta, but since there will be no delta for an object
larger than "core.bigFileThreshold", this issue is moderate.
To resolve the OOM issue in "git-unpack-objects", we can unpack large
object to file in stream, and use the setting of "core.bigFileThreshold" as
the threshold for large object.
Reviewed-by: Jiang Xin <redacted>
Signed-off-by: Han Xin <redacted>
---
builtin/unpack-objects.c | 41 +++++++-
object-file.c | 149 +++++++++++++++++++++++++++---
object-store.h | 9 ++
t/t5590-receive-unpack-objects.sh | 92 ++++++++++++++++++
4 files changed, 279 insertions(+), 12 deletions(-)
create mode 100755 t/t5590-receive-unpack-objects.sh
@@ -320,11 +320,50 @@ static void added_object(unsigned nr, enum object_type type,}}+staticvoidfill_stream(structgit_zstream*stream)+{+stream->next_in=fill(1);+stream->avail_in=len;+}++staticvoiduse_stream(structgit_zstream*stream)+{+use(len-stream->avail_in);+}++staticvoidwrite_stream_blob(unsignednr,unsignedlongsize)+{+structgit_zstream_readerreader;+structobject_id*oid=&obj_list[nr].oid;++reader.fill=&fill_stream;+reader.use=&use_stream;++if(write_stream_object_file(&reader,size,type_name(OBJ_BLOB),+oid,dry_run))+die("failed to write object in stream");+if(strict&&!dry_run){+structblob*blob=lookup_blob(the_repository,oid);+if(blob)+blob->object.flags|=FLAG_WRITTEN;+else+die("invalid blob object from stream");+}+obj_list[nr].obj=NULL;+}+staticvoidunpack_non_delta_entry(enumobject_typetype,unsignedlongsize,unsignednr){-void*buf=get_data(size);+void*buf;++/* Write large blob in stream without allocating full buffer. */+if(type==OBJ_BLOB&&size>big_file_threshold){+write_stream_blob(nr,size);+return;+}+buf=get_data(size);if(!dry_run&&buf)write_object(nr,type,buf,size);else
@@ -1913,6 +1913,28 @@ static int create_tmpfile(struct strbuf *tmp, const char *filename)returnfd;}+staticintwrite_object_buffer(structgit_zstream*stream,git_hash_ctx*c,+intfd,unsignedchar*compressed,+intcompressed_len,constvoid*buf,+size_tlen,intflush)+{+intret;++stream->next_in=(void*)buf;+stream->avail_in=len;+do{+unsignedchar*in0=stream->next_in;+ret=git_deflate(stream,flush);+the_hash_algo->update_fn(c,in0,stream->next_in-in0);+if(write_buffer(fd,compressed,stream->next_out-compressed)<0)+die(_("unable to write loose object file"));+stream->next_out=compressed;+stream->avail_out=compressed_len;+}while(ret==Z_OK);++returnret;+}+staticintwrite_loose_object(conststructobject_id*oid,char*hdr,inthdrlen,constvoid*buf,unsignedlonglen,time_tmtime)
@@ -1949,17 +1971,9 @@ static int write_loose_object(const struct object_id *oid, char *hdr,the_hash_algo->update_fn(&c,hdr,hdrlen);/* Then the data itself.. */-stream.next_in=(void*)buf;-stream.avail_in=len;-do{-unsignedchar*in0=stream.next_in;-ret=git_deflate(&stream,Z_FINISH);-the_hash_algo->update_fn(&c,in0,stream.next_in-in0);-if(write_buffer(fd,compressed,stream.next_out-compressed)<0)-die(_("unable to write loose object file"));-stream.next_out=compressed;-stream.avail_out=sizeof(compressed);-}while(ret==Z_OK);+ret=write_object_buffer(&stream,&c,fd,compressed,+sizeof(compressed),buf,len,+Z_FINISH);if(ret!=Z_STREAM_END)die(_("unable to deflate new object %s (%d)"),oid_to_hex(oid),
@@ -2020,6 +2034,119 @@ int write_object_file(const void *buf, unsigned long len, const char *type,returnwrite_loose_object(oid,hdr,hdrlen,buf,len,0);}+intwrite_stream_object_file(structgit_zstream_reader*reader,+unsignedlonglen,constchar*type,+structobject_id*oid,+intdry_run)+{+git_zstreamistream,ostream;+unsignedcharbuf[8192],compressed[4096];+charhdr[MAX_HEADER_LEN];+intistatus,ostatus,fd=0,hdrlen,dirlen,flush=0;+intret=0;+git_hash_ctxc;+structstrbuftmp_file=STRBUF_INIT;+structstrbuffilename=STRBUF_INIT;++/* Write tmpfile in objects dir, because oid is unknown */+if(!dry_run){+strbuf_addstr(&filename,the_repository->objects->odb->path);+strbuf_addch(&filename,'/');+fd=create_tmpfile(&tmp_file,filename.buf);+if(fd<0){+if(errno==EACCES)+ret=error(_("insufficient permission for adding an object to repository database %s"),+get_object_directory());+else+ret=error_errno(_("unable to create temporary file"));+gotocleanup;+}+}++memset(&istream,0,sizeof(istream));+istream.next_out=buf;+istream.avail_out=sizeof(buf);+git_inflate_init(&istream);++if(!dry_run){+/* Set it up */+git_deflate_init(&ostream,zlib_compression_level);+ostream.next_out=compressed;+ostream.avail_out=sizeof(compressed);+the_hash_algo->init_fn(&c);++/* First header */+hdrlen=xsnprintf(hdr,sizeof(hdr),"%s %"PRIuMAX,type,+(uintmax_t)len)+1;+ostream.next_in=(unsignedchar*)hdr;+ostream.avail_in=hdrlen;+while(git_deflate(&ostream,0)==Z_OK)+;/* nothing */+the_hash_algo->update_fn(&c,hdr,hdrlen);+}++/* Then the data itself */+do{+unsignedchar*last_out=istream.next_out;+reader->fill(&istream);+istatus=git_inflate(&istream,0);+if(istatus==Z_STREAM_END)+flush=Z_FINISH;+reader->use(&istream);+if(!dry_run)+ostatus=write_object_buffer(&ostream,&c,fd,compressed,+sizeof(compressed),last_out,+istream.next_out-last_out,+flush);+istream.next_out=buf;+istream.avail_out=sizeof(buf);+}while(istatus==Z_OK);++if(istream.total_out!=len||istatus!=Z_STREAM_END)+die(_("inflate returned %d"),istatus);+git_inflate_end(&istream);++if(dry_run)+gotocleanup;++if(ostatus!=Z_STREAM_END)+die(_("unable to deflate new object (%d)"),ostatus);+ostatus=git_deflate_end_gently(&ostream);+if(ostatus!=Z_OK)+die(_("deflateEnd on object failed (%d)"),ostatus);+the_hash_algo->final_fn(oid->hash,&c);+close_loose_object(fd);++/* We get the oid now */+loose_object_path(the_repository,&filename,oid);++dirlen=directory_size(filename.buf);+if(dirlen){+structstrbufdir=STRBUF_INIT;+/*+*Makesurethedirectoryexists;notethatthecontents+*ofthebufferareundefinedaftermkstempreturnsan+*error,sowehavetorewritethewholebufferfrom+*scratch.+*/+strbuf_add(&dir,filename.buf,dirlen-1);+if(mkdir(dir.buf,0777)&&errno!=EEXIST){+unlink_or_warn(tmp_file.buf);+strbuf_release(&dir);+ret=-1;+gotocleanup;+}+strbuf_release(&dir);+}++ret=finalize_object_file(tmp_file.buf,filename.buf);++cleanup:+strbuf_release(&tmp_file);+strbuf_release(&filename);+returnret;+}+inthash_object_file_literally(constvoid*buf,unsignedlonglen,constchar*type,structobject_id*oid,unsignedflags)
@@ -0,0 +1,92 @@+#!/bin/sh+#+# Copyright (c) 2021 Han Xin+#++test_description='Test unpack-objects when receive pack'++GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main+exportGIT_TEST_DEFAULT_INITIAL_BRANCH_NAME++../test-lib.sh++test_expect_success"create commit with big blobs (1.5 MB)"'+test-toolgenrandomfoo1500000>big-blob&&+test_commit--appendfoobig-blob&&+test-toolgenrandombar1500000>big-blob&&+test_commit--appendbarbig-blob&&+(+cd.git&&+findobjects/??-typef|sort+)>expect&&+gitrepack-ad+'++test_expect_success'setup GIT_ALLOC_LIMIT to 1MB''+GIT_ALLOC_LIMIT=1m&&+exportGIT_ALLOC_LIMIT+'++test_expect_success'prepare dest repository''+gitinit--baredest.git&&+git-Cdest.gitconfigcore.bigFileThreshold2m&&+git-Cdest.gitconfigreceive.unpacklimit100+'++test_expect_success'fail to push: cannot allocate''+test_must_failgitpushdest.gitHEAD2>err&&+test_i18ngrep"remote: fatal: attempting to allocate"err&&+(+cddest.git&&+findobjects/??-typef|sort+)>actual&&+!test_cmpexpectactual+'++test_expect_success'set a lower bigfile threshold''+git-Cdest.gitconfigcore.bigFileThreshold1m+'++test_expect_success'unpack big object in stream''+gitpushdest.gitHEAD&&+git-Cdest.gitfsck&&+(+cddest.git&&+findobjects/??-typef|sort+)>actual&&+test_cmpexpectactual+'++test_expect_success'setup for unpack-objects dry-run test''+PACK=$(echomain|gitpack-objects--progress--revstest)&&+unsetGIT_ALLOC_LIMIT&&+gitinit--bareunpack-test.git+'++test_expect_success'unpack-objects dry-run with large threshold''+(+cdunpack-test.git&&+gitconfigcore.bigFileThreshold2m&&+gitunpack-objects-n<../test-$PACK.pack+)&&+(+cdunpack-test.git&&+findobjects/-typef+)>actual&&+test_must_be_emptyactual+'++test_expect_success'unpack-objects dry-run with small threshold''+(+cdunpack-test.git&&+gitconfigcore.bigFileThreshold1m&&+gitunpack-objects-n<../test-$PACK.pack+)&&+(+cdunpack-test.git&&+findobjects/-typef+)>actual&&+test_must_be_emptyactual+'++test_done
Any suggestions?
Han Xin [off-list ref] 于2021年10月9日周六 下午4:21写道：
quoted hunk
From: Han Xin <redacted>
When calling "unpack_non_delta_entry()", will allocate full memory for
the whole size of the unpacked object and write the buffer to loose file
on disk. This may lead to OOM for the git-unpack-objects process when
unpacking a very large object.
In function "unpack_delta_entry()", will also allocate full memory to
buffer the whole delta, but since there will be no delta for an object
larger than "core.bigFileThreshold", this issue is moderate.
To resolve the OOM issue in "git-unpack-objects", we can unpack large
object to file in stream, and use the setting of "core.bigFileThreshold" as
the threshold for large object.
Reviewed-by: Jiang Xin <redacted>
Signed-off-by: Han Xin <redacted>
---
builtin/unpack-objects.c | 41 +++++++-
object-file.c | 149 +++++++++++++++++++++++++++---
object-store.h | 9 ++
t/t5590-receive-unpack-objects.sh | 92 ++++++++++++++++++
4 files changed, 279 insertions(+), 12 deletions(-)
create mode 100755 t/t5590-receive-unpack-objects.sh
@@ -320,11 +320,50 @@ static void added_object(unsigned nr, enum object_type type,}}+staticvoidfill_stream(structgit_zstream*stream)+{+stream->next_in=fill(1);+stream->avail_in=len;+}++staticvoiduse_stream(structgit_zstream*stream)+{+use(len-stream->avail_in);+}++staticvoidwrite_stream_blob(unsignednr,unsignedlongsize)+{+structgit_zstream_readerreader;+structobject_id*oid=&obj_list[nr].oid;++reader.fill=&fill_stream;+reader.use=&use_stream;++if(write_stream_object_file(&reader,size,type_name(OBJ_BLOB),+oid,dry_run))+die("failed to write object in stream");+if(strict&&!dry_run){+structblob*blob=lookup_blob(the_repository,oid);+if(blob)+blob->object.flags|=FLAG_WRITTEN;+else+die("invalid blob object from stream");+}+obj_list[nr].obj=NULL;+}+staticvoidunpack_non_delta_entry(enumobject_typetype,unsignedlongsize,unsignednr){-void*buf=get_data(size);+void*buf;++/* Write large blob in stream without allocating full buffer. */+if(type==OBJ_BLOB&&size>big_file_threshold){+write_stream_blob(nr,size);+return;+}+buf=get_data(size);if(!dry_run&&buf)write_object(nr,type,buf,size);else
@@ -1913,6 +1913,28 @@ static int create_tmpfile(struct strbuf *tmp, const char *filename)returnfd;}+staticintwrite_object_buffer(structgit_zstream*stream,git_hash_ctx*c,+intfd,unsignedchar*compressed,+intcompressed_len,constvoid*buf,+size_tlen,intflush)+{+intret;++stream->next_in=(void*)buf;+stream->avail_in=len;+do{+unsignedchar*in0=stream->next_in;+ret=git_deflate(stream,flush);+the_hash_algo->update_fn(c,in0,stream->next_in-in0);+if(write_buffer(fd,compressed,stream->next_out-compressed)<0)+die(_("unable to write loose object file"));+stream->next_out=compressed;+stream->avail_out=compressed_len;+}while(ret==Z_OK);++returnret;+}+staticintwrite_loose_object(conststructobject_id*oid,char*hdr,inthdrlen,constvoid*buf,unsignedlonglen,time_tmtime)
@@ -1949,17 +1971,9 @@ static int write_loose_object(const struct object_id *oid, char *hdr,the_hash_algo->update_fn(&c,hdr,hdrlen);/* Then the data itself.. */-stream.next_in=(void*)buf;-stream.avail_in=len;-do{-unsignedchar*in0=stream.next_in;-ret=git_deflate(&stream,Z_FINISH);-the_hash_algo->update_fn(&c,in0,stream.next_in-in0);-if(write_buffer(fd,compressed,stream.next_out-compressed)<0)-die(_("unable to write loose object file"));-stream.next_out=compressed;-stream.avail_out=sizeof(compressed);-}while(ret==Z_OK);+ret=write_object_buffer(&stream,&c,fd,compressed,+sizeof(compressed),buf,len,+Z_FINISH);if(ret!=Z_STREAM_END)die(_("unable to deflate new object %s (%d)"),oid_to_hex(oid),
@@ -2020,6 +2034,119 @@ int write_object_file(const void *buf, unsigned long len, const char *type,returnwrite_loose_object(oid,hdr,hdrlen,buf,len,0);}+intwrite_stream_object_file(structgit_zstream_reader*reader,+unsignedlonglen,constchar*type,+structobject_id*oid,+intdry_run)+{+git_zstreamistream,ostream;+unsignedcharbuf[8192],compressed[4096];+charhdr[MAX_HEADER_LEN];+intistatus,ostatus,fd=0,hdrlen,dirlen,flush=0;+intret=0;+git_hash_ctxc;+structstrbuftmp_file=STRBUF_INIT;+structstrbuffilename=STRBUF_INIT;++/* Write tmpfile in objects dir, because oid is unknown */+if(!dry_run){+strbuf_addstr(&filename,the_repository->objects->odb->path);+strbuf_addch(&filename,'/');+fd=create_tmpfile(&tmp_file,filename.buf);+if(fd<0){+if(errno==EACCES)+ret=error(_("insufficient permission for adding an object to repository database %s"),+get_object_directory());+else+ret=error_errno(_("unable to create temporary file"));+gotocleanup;+}+}++memset(&istream,0,sizeof(istream));+istream.next_out=buf;+istream.avail_out=sizeof(buf);+git_inflate_init(&istream);++if(!dry_run){+/* Set it up */+git_deflate_init(&ostream,zlib_compression_level);+ostream.next_out=compressed;+ostream.avail_out=sizeof(compressed);+the_hash_algo->init_fn(&c);++/* First header */+hdrlen=xsnprintf(hdr,sizeof(hdr),"%s %"PRIuMAX,type,+(uintmax_t)len)+1;+ostream.next_in=(unsignedchar*)hdr;+ostream.avail_in=hdrlen;+while(git_deflate(&ostream,0)==Z_OK)+;/* nothing */+the_hash_algo->update_fn(&c,hdr,hdrlen);+}++/* Then the data itself */+do{+unsignedchar*last_out=istream.next_out;+reader->fill(&istream);+istatus=git_inflate(&istream,0);+if(istatus==Z_STREAM_END)+flush=Z_FINISH;+reader->use(&istream);+if(!dry_run)+ostatus=write_object_buffer(&ostream,&c,fd,compressed,+sizeof(compressed),last_out,+istream.next_out-last_out,+flush);+istream.next_out=buf;+istream.avail_out=sizeof(buf);+}while(istatus==Z_OK);++if(istream.total_out!=len||istatus!=Z_STREAM_END)+die(_("inflate returned %d"),istatus);+git_inflate_end(&istream);++if(dry_run)+gotocleanup;++if(ostatus!=Z_STREAM_END)+die(_("unable to deflate new object (%d)"),ostatus);+ostatus=git_deflate_end_gently(&ostream);+if(ostatus!=Z_OK)+die(_("deflateEnd on object failed (%d)"),ostatus);+the_hash_algo->final_fn(oid->hash,&c);+close_loose_object(fd);++/* We get the oid now */+loose_object_path(the_repository,&filename,oid);++dirlen=directory_size(filename.buf);+if(dirlen){+structstrbufdir=STRBUF_INIT;+/*+*Makesurethedirectoryexists;notethatthecontents+*ofthebufferareundefinedaftermkstempreturnsan+*error,sowehavetorewritethewholebufferfrom+*scratch.+*/+strbuf_add(&dir,filename.buf,dirlen-1);+if(mkdir(dir.buf,0777)&&errno!=EEXIST){+unlink_or_warn(tmp_file.buf);+strbuf_release(&dir);+ret=-1;+gotocleanup;+}+strbuf_release(&dir);+}++ret=finalize_object_file(tmp_file.buf,filename.buf);++cleanup:+strbuf_release(&tmp_file);+strbuf_release(&filename);+returnret;+}+inthash_object_file_literally(constvoid*buf,unsignedlonglen,constchar*type,structobject_id*oid,unsignedflags)
@@ -0,0 +1,92 @@+#!/bin/sh+#+# Copyright (c) 2021 Han Xin+#++test_description='Test unpack-objects when receive pack'++GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main+exportGIT_TEST_DEFAULT_INITIAL_BRANCH_NAME++../test-lib.sh++test_expect_success"create commit with big blobs (1.5 MB)"'+test-toolgenrandomfoo1500000>big-blob&&+test_commit--appendfoobig-blob&&+test-toolgenrandombar1500000>big-blob&&+test_commit--appendbarbig-blob&&+(+cd.git&&+findobjects/??-typef|sort+)>expect&&+gitrepack-ad+'++test_expect_success'setup GIT_ALLOC_LIMIT to 1MB''+GIT_ALLOC_LIMIT=1m&&+exportGIT_ALLOC_LIMIT+'++test_expect_success'prepare dest repository''+gitinit--baredest.git&&+git-Cdest.gitconfigcore.bigFileThreshold2m&&+git-Cdest.gitconfigreceive.unpacklimit100+'++test_expect_success'fail to push: cannot allocate''+test_must_failgitpushdest.gitHEAD2>err&&+test_i18ngrep"remote: fatal: attempting to allocate"err&&+(+cddest.git&&+findobjects/??-typef|sort+)>actual&&+!test_cmpexpectactual+'++test_expect_success'set a lower bigfile threshold''+git-Cdest.gitconfigcore.bigFileThreshold1m+'++test_expect_success'unpack big object in stream''+gitpushdest.gitHEAD&&+git-Cdest.gitfsck&&+(+cddest.git&&+findobjects/??-typef|sort+)>actual&&+test_cmpexpectactual+'++test_expect_success'setup for unpack-objects dry-run test''+PACK=$(echomain|gitpack-objects--progress--revstest)&&+unsetGIT_ALLOC_LIMIT&&+gitinit--bareunpack-test.git+'++test_expect_success'unpack-objects dry-run with large threshold''+(+cdunpack-test.git&&+gitconfigcore.bigFileThreshold2m&&+gitunpack-objects-n<../test-$PACK.pack+)&&+(+cdunpack-test.git&&+findobjects/-typef+)>actual&&+test_must_be_emptyactual+'++test_expect_success'unpack-objects dry-run with small threshold''+(+cdunpack-test.git&&+gitconfigcore.bigFileThreshold1m&&+gitunpack-objects-n<../test-$PACK.pack+)&&+(+cdunpack-test.git&&+findobjects/-typef+)>actual&&+test_must_be_emptyactual+'++test_done--
From: Philip Oakley <hidden> Date: 2021-10-20 14:43:03
On 09/10/2021 09:20, Han Xin wrote:
quoted hunk
From: Han Xin <redacted>
When calling "unpack_non_delta_entry()", will allocate full memory for
the whole size of the unpacked object and write the buffer to loose file
on disk. This may lead to OOM for the git-unpack-objects process when
unpacking a very large object.
In function "unpack_delta_entry()", will also allocate full memory to
buffer the whole delta, but since there will be no delta for an object
larger than "core.bigFileThreshold", this issue is moderate.
To resolve the OOM issue in "git-unpack-objects", we can unpack large
object to file in stream, and use the setting of "core.bigFileThreshold" as
the threshold for large object.
Reviewed-by: Jiang Xin <redacted>
Signed-off-by: Han Xin <redacted>
---
builtin/unpack-objects.c | 41 +++++++-
object-file.c | 149 +++++++++++++++++++++++++++---
object-store.h | 9 ++
t/t5590-receive-unpack-objects.sh | 92 ++++++++++++++++++
4 files changed, 279 insertions(+), 12 deletions(-)
create mode 100755 t/t5590-receive-unpack-objects.sh
Can we use size_t for the `size`, and possibly `nr`, to improve
compatibility with Windows systems where unsigned long is only 32 bits?
There has been some work in the past on providing large file support on
Windows, which requires numerous long -> size_t changes.
Philip
@@ -1913,6 +1913,28 @@ static int create_tmpfile(struct strbuf *tmp, const char *filename)returnfd;}+staticintwrite_object_buffer(structgit_zstream*stream,git_hash_ctx*c,+intfd,unsignedchar*compressed,+intcompressed_len,constvoid*buf,+size_tlen,intflush)+{+intret;++stream->next_in=(void*)buf;+stream->avail_in=len;+do{+unsignedchar*in0=stream->next_in;+ret=git_deflate(stream,flush);+the_hash_algo->update_fn(c,in0,stream->next_in-in0);+if(write_buffer(fd,compressed,stream->next_out-compressed)<0)+die(_("unable to write loose object file"));+stream->next_out=compressed;+stream->avail_out=compressed_len;+}while(ret==Z_OK);++returnret;+}+staticintwrite_loose_object(conststructobject_id*oid,char*hdr,inthdrlen,constvoid*buf,unsignedlonglen,time_tmtime)
@@ -1949,17 +1971,9 @@ static int write_loose_object(const struct object_id *oid, char *hdr,the_hash_algo->update_fn(&c,hdr,hdrlen);/* Then the data itself.. */-stream.next_in=(void*)buf;-stream.avail_in=len;-do{-unsignedchar*in0=stream.next_in;-ret=git_deflate(&stream,Z_FINISH);-the_hash_algo->update_fn(&c,in0,stream.next_in-in0);-if(write_buffer(fd,compressed,stream.next_out-compressed)<0)-die(_("unable to write loose object file"));-stream.next_out=compressed;-stream.avail_out=sizeof(compressed);-}while(ret==Z_OK);+ret=write_object_buffer(&stream,&c,fd,compressed,+sizeof(compressed),buf,len,+Z_FINISH);if(ret!=Z_STREAM_END)die(_("unable to deflate new object %s (%d)"),oid_to_hex(oid),
@@ -2020,6 +2034,119 @@ int write_object_file(const void *buf, unsigned long len, const char *type,returnwrite_loose_object(oid,hdr,hdrlen,buf,len,0);}+intwrite_stream_object_file(structgit_zstream_reader*reader,+unsignedlonglen,constchar*type,+structobject_id*oid,+intdry_run)+{+git_zstreamistream,ostream;+unsignedcharbuf[8192],compressed[4096];+charhdr[MAX_HEADER_LEN];+intistatus,ostatus,fd=0,hdrlen,dirlen,flush=0;+intret=0;+git_hash_ctxc;+structstrbuftmp_file=STRBUF_INIT;+structstrbuffilename=STRBUF_INIT;++/* Write tmpfile in objects dir, because oid is unknown */+if(!dry_run){+strbuf_addstr(&filename,the_repository->objects->odb->path);+strbuf_addch(&filename,'/');+fd=create_tmpfile(&tmp_file,filename.buf);+if(fd<0){+if(errno==EACCES)+ret=error(_("insufficient permission for adding an object to repository database %s"),+get_object_directory());+else+ret=error_errno(_("unable to create temporary file"));+gotocleanup;+}+}++memset(&istream,0,sizeof(istream));+istream.next_out=buf;+istream.avail_out=sizeof(buf);+git_inflate_init(&istream);++if(!dry_run){+/* Set it up */+git_deflate_init(&ostream,zlib_compression_level);+ostream.next_out=compressed;+ostream.avail_out=sizeof(compressed);+the_hash_algo->init_fn(&c);++/* First header */+hdrlen=xsnprintf(hdr,sizeof(hdr),"%s %"PRIuMAX,type,+(uintmax_t)len)+1;+ostream.next_in=(unsignedchar*)hdr;+ostream.avail_in=hdrlen;+while(git_deflate(&ostream,0)==Z_OK)+;/* nothing */+the_hash_algo->update_fn(&c,hdr,hdrlen);+}++/* Then the data itself */+do{+unsignedchar*last_out=istream.next_out;+reader->fill(&istream);+istatus=git_inflate(&istream,0);+if(istatus==Z_STREAM_END)+flush=Z_FINISH;+reader->use(&istream);+if(!dry_run)+ostatus=write_object_buffer(&ostream,&c,fd,compressed,+sizeof(compressed),last_out,+istream.next_out-last_out,+flush);+istream.next_out=buf;+istream.avail_out=sizeof(buf);+}while(istatus==Z_OK);++if(istream.total_out!=len||istatus!=Z_STREAM_END)+die(_("inflate returned %d"),istatus);+git_inflate_end(&istream);++if(dry_run)+gotocleanup;++if(ostatus!=Z_STREAM_END)+die(_("unable to deflate new object (%d)"),ostatus);+ostatus=git_deflate_end_gently(&ostream);+if(ostatus!=Z_OK)+die(_("deflateEnd on object failed (%d)"),ostatus);+the_hash_algo->final_fn(oid->hash,&c);+close_loose_object(fd);++/* We get the oid now */+loose_object_path(the_repository,&filename,oid);++dirlen=directory_size(filename.buf);+if(dirlen){+structstrbufdir=STRBUF_INIT;+/*+*Makesurethedirectoryexists;notethatthecontents+*ofthebufferareundefinedaftermkstempreturnsan+*error,sowehavetorewritethewholebufferfrom+*scratch.+*/+strbuf_add(&dir,filename.buf,dirlen-1);+if(mkdir(dir.buf,0777)&&errno!=EEXIST){+unlink_or_warn(tmp_file.buf);+strbuf_release(&dir);+ret=-1;+gotocleanup;+}+strbuf_release(&dir);+}++ret=finalize_object_file(tmp_file.buf,filename.buf);++cleanup:+strbuf_release(&tmp_file);+strbuf_release(&filename);+returnret;+}+inthash_object_file_literally(constvoid*buf,unsignedlonglen,constchar*type,structobject_id*oid,unsignedflags)
@@ -0,0 +1,92 @@+#!/bin/sh+#+# Copyright (c) 2021 Han Xin+#++test_description='Test unpack-objects when receive pack'++GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main+exportGIT_TEST_DEFAULT_INITIAL_BRANCH_NAME++../test-lib.sh++test_expect_success"create commit with big blobs (1.5 MB)"'+test-toolgenrandomfoo1500000>big-blob&&+test_commit--appendfoobig-blob&&+test-toolgenrandombar1500000>big-blob&&+test_commit--appendbarbig-blob&&+(+cd.git&&+findobjects/??-typef|sort+)>expect&&+gitrepack-ad+'++test_expect_success'setup GIT_ALLOC_LIMIT to 1MB''+GIT_ALLOC_LIMIT=1m&&+exportGIT_ALLOC_LIMIT+'++test_expect_success'prepare dest repository''+gitinit--baredest.git&&+git-Cdest.gitconfigcore.bigFileThreshold2m&&+git-Cdest.gitconfigreceive.unpacklimit100+'++test_expect_success'fail to push: cannot allocate''+test_must_failgitpushdest.gitHEAD2>err&&+test_i18ngrep"remote: fatal: attempting to allocate"err&&+(+cddest.git&&+findobjects/??-typef|sort+)>actual&&+!test_cmpexpectactual+'++test_expect_success'set a lower bigfile threshold''+git-Cdest.gitconfigcore.bigFileThreshold1m+'++test_expect_success'unpack big object in stream''+gitpushdest.gitHEAD&&+git-Cdest.gitfsck&&+(+cddest.git&&+findobjects/??-typef|sort+)>actual&&+test_cmpexpectactual+'++test_expect_success'setup for unpack-objects dry-run test''+PACK=$(echomain|gitpack-objects--progress--revstest)&&+unsetGIT_ALLOC_LIMIT&&+gitinit--bareunpack-test.git+'++test_expect_success'unpack-objects dry-run with large threshold''+(+cdunpack-test.git&&+gitconfigcore.bigFileThreshold2m&&+gitunpack-objects-n<../test-$PACK.pack+)&&+(+cdunpack-test.git&&+findobjects/-typef+)>actual&&+test_must_be_emptyactual+'++test_expect_success'unpack-objects dry-run with small threshold''+(+cdunpack-test.git&&+gitconfigcore.bigFileThreshold1m&&+gitunpack-objects-n<../test-$PACK.pack+)&&+(+cdunpack-test.git&&+findobjects/-typef+)>actual&&+test_must_be_emptyactual+'++test_done
Philip Oakley [off-list ref] 于2021年10月20日周三 下午10:43写道：
On 09/10/2021 09:20, Han Xin wrote:
quoted
From: Han Xin <redacted>
When calling "unpack_non_delta_entry()", will allocate full memory for
the whole size of the unpacked object and write the buffer to loose file
on disk. This may lead to OOM for the git-unpack-objects process when
unpacking a very large object.
In function "unpack_delta_entry()", will also allocate full memory to
buffer the whole delta, but since there will be no delta for an object
larger than "core.bigFileThreshold", this issue is moderate.
To resolve the OOM issue in "git-unpack-objects", we can unpack large
object to file in stream, and use the setting of "core.bigFileThreshold" as
the threshold for large object.
Reviewed-by: Jiang Xin <redacted>
Signed-off-by: Han Xin <redacted>
---
builtin/unpack-objects.c | 41 +++++++-
object-file.c | 149 +++++++++++++++++++++++++++---
object-store.h | 9 ++
t/t5590-receive-unpack-objects.sh | 92 ++++++++++++++++++
4 files changed, 279 insertions(+), 12 deletions(-)
create mode 100755 t/t5590-receive-unpack-objects.sh
Can we use size_t for the `size`, and possibly `nr`, to improve
compatibility with Windows systems where unsigned long is only 32 bits?
There has been some work in the past on providing large file support on
Windows, which requires numerous long -> size_t changes.
Philip
Thanks for your review. I'm not sure if I should do this change in this patch,
it will also change the type defined in `unpack_one()`,`unpack_non_delta_entry`,
`write_object()` and many others.
@@ -1913,6 +1913,28 @@ static int create_tmpfile(struct strbuf *tmp, const char *filename)returnfd;}+staticintwrite_object_buffer(structgit_zstream*stream,git_hash_ctx*c,+intfd,unsignedchar*compressed,+intcompressed_len,constvoid*buf,+size_tlen,intflush)+{+intret;++stream->next_in=(void*)buf;+stream->avail_in=len;+do{+unsignedchar*in0=stream->next_in;+ret=git_deflate(stream,flush);+the_hash_algo->update_fn(c,in0,stream->next_in-in0);+if(write_buffer(fd,compressed,stream->next_out-compressed)<0)+die(_("unable to write loose object file"));+stream->next_out=compressed;+stream->avail_out=compressed_len;+}while(ret==Z_OK);++returnret;+}+staticintwrite_loose_object(conststructobject_id*oid,char*hdr,inthdrlen,constvoid*buf,unsignedlonglen,time_tmtime)
@@ -1949,17 +1971,9 @@ static int write_loose_object(const struct object_id *oid, char *hdr,the_hash_algo->update_fn(&c,hdr,hdrlen);/* Then the data itself.. */-stream.next_in=(void*)buf;-stream.avail_in=len;-do{-unsignedchar*in0=stream.next_in;-ret=git_deflate(&stream,Z_FINISH);-the_hash_algo->update_fn(&c,in0,stream.next_in-in0);-if(write_buffer(fd,compressed,stream.next_out-compressed)<0)-die(_("unable to write loose object file"));-stream.next_out=compressed;-stream.avail_out=sizeof(compressed);-}while(ret==Z_OK);+ret=write_object_buffer(&stream,&c,fd,compressed,+sizeof(compressed),buf,len,+Z_FINISH);if(ret!=Z_STREAM_END)die(_("unable to deflate new object %s (%d)"),oid_to_hex(oid),
@@ -2020,6 +2034,119 @@ int write_object_file(const void *buf, unsigned long len, const char *type,returnwrite_loose_object(oid,hdr,hdrlen,buf,len,0);}+intwrite_stream_object_file(structgit_zstream_reader*reader,+unsignedlonglen,constchar*type,+structobject_id*oid,+intdry_run)+{+git_zstreamistream,ostream;+unsignedcharbuf[8192],compressed[4096];+charhdr[MAX_HEADER_LEN];+intistatus,ostatus,fd=0,hdrlen,dirlen,flush=0;+intret=0;+git_hash_ctxc;+structstrbuftmp_file=STRBUF_INIT;+structstrbuffilename=STRBUF_INIT;++/* Write tmpfile in objects dir, because oid is unknown */+if(!dry_run){+strbuf_addstr(&filename,the_repository->objects->odb->path);+strbuf_addch(&filename,'/');+fd=create_tmpfile(&tmp_file,filename.buf);+if(fd<0){+if(errno==EACCES)+ret=error(_("insufficient permission for adding an object to repository database %s"),+get_object_directory());+else+ret=error_errno(_("unable to create temporary file"));+gotocleanup;+}+}++memset(&istream,0,sizeof(istream));+istream.next_out=buf;+istream.avail_out=sizeof(buf);+git_inflate_init(&istream);++if(!dry_run){+/* Set it up */+git_deflate_init(&ostream,zlib_compression_level);+ostream.next_out=compressed;+ostream.avail_out=sizeof(compressed);+the_hash_algo->init_fn(&c);++/* First header */+hdrlen=xsnprintf(hdr,sizeof(hdr),"%s %"PRIuMAX,type,+(uintmax_t)len)+1;+ostream.next_in=(unsignedchar*)hdr;+ostream.avail_in=hdrlen;+while(git_deflate(&ostream,0)==Z_OK)+;/* nothing */+the_hash_algo->update_fn(&c,hdr,hdrlen);+}++/* Then the data itself */+do{+unsignedchar*last_out=istream.next_out;+reader->fill(&istream);+istatus=git_inflate(&istream,0);+if(istatus==Z_STREAM_END)+flush=Z_FINISH;+reader->use(&istream);+if(!dry_run)+ostatus=write_object_buffer(&ostream,&c,fd,compressed,+sizeof(compressed),last_out,+istream.next_out-last_out,+flush);+istream.next_out=buf;+istream.avail_out=sizeof(buf);+}while(istatus==Z_OK);++if(istream.total_out!=len||istatus!=Z_STREAM_END)+die(_("inflate returned %d"),istatus);+git_inflate_end(&istream);++if(dry_run)+gotocleanup;++if(ostatus!=Z_STREAM_END)+die(_("unable to deflate new object (%d)"),ostatus);+ostatus=git_deflate_end_gently(&ostream);+if(ostatus!=Z_OK)+die(_("deflateEnd on object failed (%d)"),ostatus);+the_hash_algo->final_fn(oid->hash,&c);+close_loose_object(fd);++/* We get the oid now */+loose_object_path(the_repository,&filename,oid);++dirlen=directory_size(filename.buf);+if(dirlen){+structstrbufdir=STRBUF_INIT;+/*+*Makesurethedirectoryexists;notethatthecontents+*ofthebufferareundefinedaftermkstempreturnsan+*error,sowehavetorewritethewholebufferfrom+*scratch.+*/+strbuf_add(&dir,filename.buf,dirlen-1);+if(mkdir(dir.buf,0777)&&errno!=EEXIST){+unlink_or_warn(tmp_file.buf);+strbuf_release(&dir);+ret=-1;+gotocleanup;+}+strbuf_release(&dir);+}++ret=finalize_object_file(tmp_file.buf,filename.buf);++cleanup:+strbuf_release(&tmp_file);+strbuf_release(&filename);+returnret;+}+inthash_object_file_literally(constvoid*buf,unsignedlonglen,constchar*type,structobject_id*oid,unsignedflags)
@@ -0,0 +1,92 @@+#!/bin/sh+#+# Copyright (c) 2021 Han Xin+#++test_description='Test unpack-objects when receive pack'++GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main+exportGIT_TEST_DEFAULT_INITIAL_BRANCH_NAME++../test-lib.sh++test_expect_success"create commit with big blobs (1.5 MB)"'+test-toolgenrandomfoo1500000>big-blob&&+test_commit--appendfoobig-blob&&+test-toolgenrandombar1500000>big-blob&&+test_commit--appendbarbig-blob&&+(+cd.git&&+findobjects/??-typef|sort+)>expect&&+gitrepack-ad+'++test_expect_success'setup GIT_ALLOC_LIMIT to 1MB''+GIT_ALLOC_LIMIT=1m&&+exportGIT_ALLOC_LIMIT+'++test_expect_success'prepare dest repository''+gitinit--baredest.git&&+git-Cdest.gitconfigcore.bigFileThreshold2m&&+git-Cdest.gitconfigreceive.unpacklimit100+'++test_expect_success'fail to push: cannot allocate''+test_must_failgitpushdest.gitHEAD2>err&&+test_i18ngrep"remote: fatal: attempting to allocate"err&&+(+cddest.git&&+findobjects/??-typef|sort+)>actual&&+!test_cmpexpectactual+'++test_expect_success'set a lower bigfile threshold''+git-Cdest.gitconfigcore.bigFileThreshold1m+'++test_expect_success'unpack big object in stream''+gitpushdest.gitHEAD&&+git-Cdest.gitfsck&&+(+cddest.git&&+findobjects/??-typef|sort+)>actual&&+test_cmpexpectactual+'++test_expect_success'setup for unpack-objects dry-run test''+PACK=$(echomain|gitpack-objects--progress--revstest)&&+unsetGIT_ALLOC_LIMIT&&+gitinit--bareunpack-test.git+'++test_expect_success'unpack-objects dry-run with large threshold''+(+cdunpack-test.git&&+gitconfigcore.bigFileThreshold2m&&+gitunpack-objects-n<../test-$PACK.pack+)&&+(+cdunpack-test.git&&+findobjects/-typef+)>actual&&+test_must_be_emptyactual+'++test_expect_success'unpack-objects dry-run with small threshold''+(+cdunpack-test.git&&+gitconfigcore.bigFileThreshold1m&&+gitunpack-objects-n<../test-$PACK.pack+)&&+(+cdunpack-test.git&&+findobjects/-typef+)>actual&&+test_must_be_emptyactual+'++test_done
From: Philip Oakley <hidden> Date: 2021-10-21 22:47:17
On 21/10/2021 04:42, Han Xin wrote:
quoted
quoted
+static void write_stream_blob(unsigned nr, unsigned long size)
Can we use size_t for the `size`, and possibly `nr`, to improve
compatibility with Windows systems where unsigned long is only 32 bits?
There has been some work in the past on providing large file support on
Windows, which requires numerous long -> size_t changes.
Philip
Thanks for your review. I'm not sure if I should do this change in this patch,
it will also change the type defined in `unpack_one()`,`unpack_non_delta_entry`,
`write_object()` and many others.
I was mainly raising the issue regarding the 4GB (sometime 2GB)
limitations on Windows which has been a problem for many years.
I had been thinking of not changing the `nr` (number of objects limit)
as 2G objects is hopefully already sufficient, even for thargest of
repos (though IIUC their index file size did break the 32bit size limit).
Staying with the existing types won't make the situation any worse, so
from that perspective the change isn't needed.
--
Philip
Any more suggestions?
Han Xin [off-list ref] 于2021年10月9日周六 下午4:21写道：
quoted hunk
From: Han Xin <redacted>
When calling "unpack_non_delta_entry()", will allocate full memory for
the whole size of the unpacked object and write the buffer to loose file
on disk. This may lead to OOM for the git-unpack-objects process when
unpacking a very large object.
In function "unpack_delta_entry()", will also allocate full memory to
buffer the whole delta, but since there will be no delta for an object
larger than "core.bigFileThreshold", this issue is moderate.
To resolve the OOM issue in "git-unpack-objects", we can unpack large
object to file in stream, and use the setting of "core.bigFileThreshold" as
the threshold for large object.
Reviewed-by: Jiang Xin <redacted>
Signed-off-by: Han Xin <redacted>
---
builtin/unpack-objects.c | 41 +++++++-
object-file.c | 149 +++++++++++++++++++++++++++---
object-store.h | 9 ++
t/t5590-receive-unpack-objects.sh | 92 ++++++++++++++++++
4 files changed, 279 insertions(+), 12 deletions(-)
create mode 100755 t/t5590-receive-unpack-objects.sh
@@ -320,11 +320,50 @@ static void added_object(unsigned nr, enum object_type type,}}+staticvoidfill_stream(structgit_zstream*stream)+{+stream->next_in=fill(1);+stream->avail_in=len;+}++staticvoiduse_stream(structgit_zstream*stream)+{+use(len-stream->avail_in);+}++staticvoidwrite_stream_blob(unsignednr,unsignedlongsize)+{+structgit_zstream_readerreader;+structobject_id*oid=&obj_list[nr].oid;++reader.fill=&fill_stream;+reader.use=&use_stream;++if(write_stream_object_file(&reader,size,type_name(OBJ_BLOB),+oid,dry_run))+die("failed to write object in stream");+if(strict&&!dry_run){+structblob*blob=lookup_blob(the_repository,oid);+if(blob)+blob->object.flags|=FLAG_WRITTEN;+else+die("invalid blob object from stream");+}+obj_list[nr].obj=NULL;+}+staticvoidunpack_non_delta_entry(enumobject_typetype,unsignedlongsize,unsignednr){-void*buf=get_data(size);+void*buf;++/* Write large blob in stream without allocating full buffer. */+if(type==OBJ_BLOB&&size>big_file_threshold){+write_stream_blob(nr,size);+return;+}+buf=get_data(size);if(!dry_run&&buf)write_object(nr,type,buf,size);else
@@ -1913,6 +1913,28 @@ static int create_tmpfile(struct strbuf *tmp, const char *filename)returnfd;}+staticintwrite_object_buffer(structgit_zstream*stream,git_hash_ctx*c,+intfd,unsignedchar*compressed,+intcompressed_len,constvoid*buf,+size_tlen,intflush)+{+intret;++stream->next_in=(void*)buf;+stream->avail_in=len;+do{+unsignedchar*in0=stream->next_in;+ret=git_deflate(stream,flush);+the_hash_algo->update_fn(c,in0,stream->next_in-in0);+if(write_buffer(fd,compressed,stream->next_out-compressed)<0)+die(_("unable to write loose object file"));+stream->next_out=compressed;+stream->avail_out=compressed_len;+}while(ret==Z_OK);++returnret;+}+staticintwrite_loose_object(conststructobject_id*oid,char*hdr,inthdrlen,constvoid*buf,unsignedlonglen,time_tmtime)
@@ -1949,17 +1971,9 @@ static int write_loose_object(const struct object_id *oid, char *hdr,the_hash_algo->update_fn(&c,hdr,hdrlen);/* Then the data itself.. */-stream.next_in=(void*)buf;-stream.avail_in=len;-do{-unsignedchar*in0=stream.next_in;-ret=git_deflate(&stream,Z_FINISH);-the_hash_algo->update_fn(&c,in0,stream.next_in-in0);-if(write_buffer(fd,compressed,stream.next_out-compressed)<0)-die(_("unable to write loose object file"));-stream.next_out=compressed;-stream.avail_out=sizeof(compressed);-}while(ret==Z_OK);+ret=write_object_buffer(&stream,&c,fd,compressed,+sizeof(compressed),buf,len,+Z_FINISH);if(ret!=Z_STREAM_END)die(_("unable to deflate new object %s (%d)"),oid_to_hex(oid),
@@ -2020,6 +2034,119 @@ int write_object_file(const void *buf, unsigned long len, const char *type,returnwrite_loose_object(oid,hdr,hdrlen,buf,len,0);}+intwrite_stream_object_file(structgit_zstream_reader*reader,+unsignedlonglen,constchar*type,+structobject_id*oid,+intdry_run)+{+git_zstreamistream,ostream;+unsignedcharbuf[8192],compressed[4096];+charhdr[MAX_HEADER_LEN];+intistatus,ostatus,fd=0,hdrlen,dirlen,flush=0;+intret=0;+git_hash_ctxc;+structstrbuftmp_file=STRBUF_INIT;+structstrbuffilename=STRBUF_INIT;++/* Write tmpfile in objects dir, because oid is unknown */+if(!dry_run){+strbuf_addstr(&filename,the_repository->objects->odb->path);+strbuf_addch(&filename,'/');+fd=create_tmpfile(&tmp_file,filename.buf);+if(fd<0){+if(errno==EACCES)+ret=error(_("insufficient permission for adding an object to repository database %s"),+get_object_directory());+else+ret=error_errno(_("unable to create temporary file"));+gotocleanup;+}+}++memset(&istream,0,sizeof(istream));+istream.next_out=buf;+istream.avail_out=sizeof(buf);+git_inflate_init(&istream);++if(!dry_run){+/* Set it up */+git_deflate_init(&ostream,zlib_compression_level);+ostream.next_out=compressed;+ostream.avail_out=sizeof(compressed);+the_hash_algo->init_fn(&c);++/* First header */+hdrlen=xsnprintf(hdr,sizeof(hdr),"%s %"PRIuMAX,type,+(uintmax_t)len)+1;+ostream.next_in=(unsignedchar*)hdr;+ostream.avail_in=hdrlen;+while(git_deflate(&ostream,0)==Z_OK)+;/* nothing */+the_hash_algo->update_fn(&c,hdr,hdrlen);+}++/* Then the data itself */+do{+unsignedchar*last_out=istream.next_out;+reader->fill(&istream);+istatus=git_inflate(&istream,0);+if(istatus==Z_STREAM_END)+flush=Z_FINISH;+reader->use(&istream);+if(!dry_run)+ostatus=write_object_buffer(&ostream,&c,fd,compressed,+sizeof(compressed),last_out,+istream.next_out-last_out,+flush);+istream.next_out=buf;+istream.avail_out=sizeof(buf);+}while(istatus==Z_OK);++if(istream.total_out!=len||istatus!=Z_STREAM_END)+die(_("inflate returned %d"),istatus);+git_inflate_end(&istream);++if(dry_run)+gotocleanup;++if(ostatus!=Z_STREAM_END)+die(_("unable to deflate new object (%d)"),ostatus);+ostatus=git_deflate_end_gently(&ostream);+if(ostatus!=Z_OK)+die(_("deflateEnd on object failed (%d)"),ostatus);+the_hash_algo->final_fn(oid->hash,&c);+close_loose_object(fd);++/* We get the oid now */+loose_object_path(the_repository,&filename,oid);++dirlen=directory_size(filename.buf);+if(dirlen){+structstrbufdir=STRBUF_INIT;+/*+*Makesurethedirectoryexists;notethatthecontents+*ofthebufferareundefinedaftermkstempreturnsan+*error,sowehavetorewritethewholebufferfrom+*scratch.+*/+strbuf_add(&dir,filename.buf,dirlen-1);+if(mkdir(dir.buf,0777)&&errno!=EEXIST){+unlink_or_warn(tmp_file.buf);+strbuf_release(&dir);+ret=-1;+gotocleanup;+}+strbuf_release(&dir);+}++ret=finalize_object_file(tmp_file.buf,filename.buf);++cleanup:+strbuf_release(&tmp_file);+strbuf_release(&filename);+returnret;+}+inthash_object_file_literally(constvoid*buf,unsignedlonglen,constchar*type,structobject_id*oid,unsignedflags)
@@ -0,0 +1,92 @@+#!/bin/sh+#+# Copyright (c) 2021 Han Xin+#++test_description='Test unpack-objects when receive pack'++GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main+exportGIT_TEST_DEFAULT_INITIAL_BRANCH_NAME++../test-lib.sh++test_expect_success"create commit with big blobs (1.5 MB)"'+test-toolgenrandomfoo1500000>big-blob&&+test_commit--appendfoobig-blob&&+test-toolgenrandombar1500000>big-blob&&+test_commit--appendbarbig-blob&&+(+cd.git&&+findobjects/??-typef|sort+)>expect&&+gitrepack-ad+'++test_expect_success'setup GIT_ALLOC_LIMIT to 1MB''+GIT_ALLOC_LIMIT=1m&&+exportGIT_ALLOC_LIMIT+'++test_expect_success'prepare dest repository''+gitinit--baredest.git&&+git-Cdest.gitconfigcore.bigFileThreshold2m&&+git-Cdest.gitconfigreceive.unpacklimit100+'++test_expect_success'fail to push: cannot allocate''+test_must_failgitpushdest.gitHEAD2>err&&+test_i18ngrep"remote: fatal: attempting to allocate"err&&+(+cddest.git&&+findobjects/??-typef|sort+)>actual&&+!test_cmpexpectactual+'++test_expect_success'set a lower bigfile threshold''+git-Cdest.gitconfigcore.bigFileThreshold1m+'++test_expect_success'unpack big object in stream''+gitpushdest.gitHEAD&&+git-Cdest.gitfsck&&+(+cddest.git&&+findobjects/??-typef|sort+)>actual&&+test_cmpexpectactual+'++test_expect_success'setup for unpack-objects dry-run test''+PACK=$(echomain|gitpack-objects--progress--revstest)&&+unsetGIT_ALLOC_LIMIT&&+gitinit--bareunpack-test.git+'++test_expect_success'unpack-objects dry-run with large threshold''+(+cdunpack-test.git&&+gitconfigcore.bigFileThreshold2m&&+gitunpack-objects-n<../test-$PACK.pack+)&&+(+cdunpack-test.git&&+findobjects/-typef+)>actual&&+test_must_be_emptyactual+'++test_expect_success'unpack-objects dry-run with small threshold''+(+cdunpack-test.git&&+gitconfigcore.bigFileThreshold1m&&+gitunpack-objects-n<../test-$PACK.pack+)&&+(+cdunpack-test.git&&+findobjects/-typef+)>actual&&+test_must_be_emptyactual+'++test_done--
From: Philip Oakley <hidden> Date: 2021-11-03 10:07:43
(replies to the alibaba-inc.com aren't getting through for me)
On 03/11/2021 01:48, Han Xin wrote:
Any more suggestions?
Han Xin [off-list ref] 于2021年10月9日周六 下午4:21写道：
quoted
From: Han Xin <redacted>
When calling "unpack_non_delta_entry()", will allocate full memory for
the whole size of the unpacked object and write the buffer to loose file
on disk. This may lead to OOM for the git-unpack-objects process when
unpacking a very large object.
Is it possible to split the patch into smaller pieces, taking each item
separately?
For large files (as above), it should be possible to stream the
unpacking direct to disk, in the same way that the zlib reading is
chunked. However having the same 'code' in two places would need to be
addressed (the DRY principle).
At the moment on LLP64 systems (Windows) there is already a long (32bit)
vs size_t (64bit) problem there (zlib stream), and the size_t problem
then permeates the wider codebase.
The normal Git file operations does tend to memory map whole files, but
here it looks like you can bypass that.
quoted
In function "unpack_delta_entry()", will also allocate full memory to
buffer the whole delta, but since there will be no delta for an object
larger than "core.bigFileThreshold", this issue is moderate.
What does 'moderate' mean here? Does it mean there is a simple test that
allows you to side step the whole problem?
quoted
To resolve the OOM issue in "git-unpack-objects", we can unpack large
object to file in stream, and use the setting of "core.bigFileThreshold" as
the threshold for large object.
Is this "core.bigFileThreshold" the core element? If so, it is too far
down the commit message. The readers have already (potentially) misread
the message and reacted too soon. Perhaps: "use `core.bigFileThreshold`
to avoid mmap OOM limits when unpacking".
--
Philip
@@ -320,11 +320,50 @@ static void added_object(unsigned nr, enum object_type type,}}+staticvoidfill_stream(structgit_zstream*stream)+{+stream->next_in=fill(1);+stream->avail_in=len;+}++staticvoiduse_stream(structgit_zstream*stream)+{+use(len-stream->avail_in);+}++staticvoidwrite_stream_blob(unsignednr,unsignedlongsize)+{+structgit_zstream_readerreader;+structobject_id*oid=&obj_list[nr].oid;++reader.fill=&fill_stream;+reader.use=&use_stream;++if(write_stream_object_file(&reader,size,type_name(OBJ_BLOB),+oid,dry_run))+die("failed to write object in stream");+if(strict&&!dry_run){+structblob*blob=lookup_blob(the_repository,oid);+if(blob)+blob->object.flags|=FLAG_WRITTEN;+else+die("invalid blob object from stream");+}+obj_list[nr].obj=NULL;+}+staticvoidunpack_non_delta_entry(enumobject_typetype,unsignedlongsize,unsignednr){-void*buf=get_data(size);+void*buf;++/* Write large blob in stream without allocating full buffer. */+if(type==OBJ_BLOB&&size>big_file_threshold){+write_stream_blob(nr,size);+return;+}+buf=get_data(size);if(!dry_run&&buf)write_object(nr,type,buf,size);else
@@ -1913,6 +1913,28 @@ static int create_tmpfile(struct strbuf *tmp, const char *filename)returnfd;}+staticintwrite_object_buffer(structgit_zstream*stream,git_hash_ctx*c,+intfd,unsignedchar*compressed,+intcompressed_len,constvoid*buf,+size_tlen,intflush)+{+intret;++stream->next_in=(void*)buf;+stream->avail_in=len;+do{+unsignedchar*in0=stream->next_in;+ret=git_deflate(stream,flush);+the_hash_algo->update_fn(c,in0,stream->next_in-in0);+if(write_buffer(fd,compressed,stream->next_out-compressed)<0)+die(_("unable to write loose object file"));+stream->next_out=compressed;+stream->avail_out=compressed_len;+}while(ret==Z_OK);++returnret;+}+staticintwrite_loose_object(conststructobject_id*oid,char*hdr,inthdrlen,constvoid*buf,unsignedlonglen,time_tmtime)
@@ -1949,17 +1971,9 @@ static int write_loose_object(const struct object_id *oid, char *hdr,the_hash_algo->update_fn(&c,hdr,hdrlen);/* Then the data itself.. */-stream.next_in=(void*)buf;-stream.avail_in=len;-do{-unsignedchar*in0=stream.next_in;-ret=git_deflate(&stream,Z_FINISH);-the_hash_algo->update_fn(&c,in0,stream.next_in-in0);-if(write_buffer(fd,compressed,stream.next_out-compressed)<0)-die(_("unable to write loose object file"));-stream.next_out=compressed;-stream.avail_out=sizeof(compressed);-}while(ret==Z_OK);+ret=write_object_buffer(&stream,&c,fd,compressed,+sizeof(compressed),buf,len,+Z_FINISH);if(ret!=Z_STREAM_END)die(_("unable to deflate new object %s (%d)"),oid_to_hex(oid),
@@ -2020,6 +2034,119 @@ int write_object_file(const void *buf, unsigned long len, const char *type,returnwrite_loose_object(oid,hdr,hdrlen,buf,len,0);}+intwrite_stream_object_file(structgit_zstream_reader*reader,+unsignedlonglen,constchar*type,+structobject_id*oid,+intdry_run)+{+git_zstreamistream,ostream;+unsignedcharbuf[8192],compressed[4096];+charhdr[MAX_HEADER_LEN];+intistatus,ostatus,fd=0,hdrlen,dirlen,flush=0;+intret=0;+git_hash_ctxc;+structstrbuftmp_file=STRBUF_INIT;+structstrbuffilename=STRBUF_INIT;++/* Write tmpfile in objects dir, because oid is unknown */+if(!dry_run){+strbuf_addstr(&filename,the_repository->objects->odb->path);+strbuf_addch(&filename,'/');+fd=create_tmpfile(&tmp_file,filename.buf);+if(fd<0){+if(errno==EACCES)+ret=error(_("insufficient permission for adding an object to repository database %s"),+get_object_directory());+else+ret=error_errno(_("unable to create temporary file"));+gotocleanup;+}+}++memset(&istream,0,sizeof(istream));+istream.next_out=buf;+istream.avail_out=sizeof(buf);+git_inflate_init(&istream);++if(!dry_run){+/* Set it up */+git_deflate_init(&ostream,zlib_compression_level);+ostream.next_out=compressed;+ostream.avail_out=sizeof(compressed);+the_hash_algo->init_fn(&c);++/* First header */+hdrlen=xsnprintf(hdr,sizeof(hdr),"%s %"PRIuMAX,type,+(uintmax_t)len)+1;+ostream.next_in=(unsignedchar*)hdr;+ostream.avail_in=hdrlen;+while(git_deflate(&ostream,0)==Z_OK)+;/* nothing */+the_hash_algo->update_fn(&c,hdr,hdrlen);+}++/* Then the data itself */+do{+unsignedchar*last_out=istream.next_out;+reader->fill(&istream);+istatus=git_inflate(&istream,0);+if(istatus==Z_STREAM_END)+flush=Z_FINISH;+reader->use(&istream);+if(!dry_run)+ostatus=write_object_buffer(&ostream,&c,fd,compressed,+sizeof(compressed),last_out,+istream.next_out-last_out,+flush);+istream.next_out=buf;+istream.avail_out=sizeof(buf);+}while(istatus==Z_OK);++if(istream.total_out!=len||istatus!=Z_STREAM_END)+die(_("inflate returned %d"),istatus);+git_inflate_end(&istream);++if(dry_run)+gotocleanup;++if(ostatus!=Z_STREAM_END)+die(_("unable to deflate new object (%d)"),ostatus);+ostatus=git_deflate_end_gently(&ostream);+if(ostatus!=Z_OK)+die(_("deflateEnd on object failed (%d)"),ostatus);+the_hash_algo->final_fn(oid->hash,&c);+close_loose_object(fd);++/* We get the oid now */+loose_object_path(the_repository,&filename,oid);++dirlen=directory_size(filename.buf);+if(dirlen){+structstrbufdir=STRBUF_INIT;+/*+*Makesurethedirectoryexists;notethatthecontents+*ofthebufferareundefinedaftermkstempreturnsan+*error,sowehavetorewritethewholebufferfrom+*scratch.+*/+strbuf_add(&dir,filename.buf,dirlen-1);+if(mkdir(dir.buf,0777)&&errno!=EEXIST){+unlink_or_warn(tmp_file.buf);+strbuf_release(&dir);+ret=-1;+gotocleanup;+}+strbuf_release(&dir);+}++ret=finalize_object_file(tmp_file.buf,filename.buf);++cleanup:+strbuf_release(&tmp_file);+strbuf_release(&filename);+returnret;+}+inthash_object_file_literally(constvoid*buf,unsignedlonglen,constchar*type,structobject_id*oid,unsignedflags)
@@ -0,0 +1,92 @@+#!/bin/sh+#+# Copyright (c) 2021 Han Xin+#++test_description='Test unpack-objects when receive pack'++GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main+exportGIT_TEST_DEFAULT_INITIAL_BRANCH_NAME++../test-lib.sh++test_expect_success"create commit with big blobs (1.5 MB)"'+test-toolgenrandomfoo1500000>big-blob&&+test_commit--appendfoobig-blob&&+test-toolgenrandombar1500000>big-blob&&+test_commit--appendbarbig-blob&&+(+cd.git&&+findobjects/??-typef|sort+)>expect&&+gitrepack-ad+'++test_expect_success'setup GIT_ALLOC_LIMIT to 1MB''+GIT_ALLOC_LIMIT=1m&&+exportGIT_ALLOC_LIMIT+'++test_expect_success'prepare dest repository''+gitinit--baredest.git&&+git-Cdest.gitconfigcore.bigFileThreshold2m&&+git-Cdest.gitconfigreceive.unpacklimit100+'++test_expect_success'fail to push: cannot allocate''+test_must_failgitpushdest.gitHEAD2>err&&+test_i18ngrep"remote: fatal: attempting to allocate"err&&+(+cddest.git&&+findobjects/??-typef|sort+)>actual&&+!test_cmpexpectactual+'++test_expect_success'set a lower bigfile threshold''+git-Cdest.gitconfigcore.bigFileThreshold1m+'++test_expect_success'unpack big object in stream''+gitpushdest.gitHEAD&&+git-Cdest.gitfsck&&+(+cddest.git&&+findobjects/??-typef|sort+)>actual&&+test_cmpexpectactual+'++test_expect_success'setup for unpack-objects dry-run test''+PACK=$(echomain|gitpack-objects--progress--revstest)&&+unsetGIT_ALLOC_LIMIT&&+gitinit--bareunpack-test.git+'++test_expect_success'unpack-objects dry-run with large threshold''+(+cdunpack-test.git&&+gitconfigcore.bigFileThreshold2m&&+gitunpack-objects-n<../test-$PACK.pack+)&&+(+cdunpack-test.git&&+findobjects/-typef+)>actual&&+test_must_be_emptyactual+'++test_expect_success'unpack-objects dry-run with small threshold''+(+cdunpack-test.git&&+gitconfigcore.bigFileThreshold1m&&+gitunpack-objects-n<../test-$PACK.pack+)&&+(+cdunpack-test.git&&+findobjects/-typef+)>actual&&+test_must_be_emptyactual+'++test_done--
From: Han Xin <redacted>
Refactor write_loose_object() to support inputstream, in the same way
that zlib reading is chunked.
Using "in_stream" instead of "void *buf", we needn't to allocate enough
memory in advance, and only part of the contents will be read when
called "in_stream.read()".
Helped-by: Jiang Xin [off-list ref]
Signed-off-by: Han Xin <redacted>
---
object-file.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++----
object-store.h | 5 +++++
2 files changed, 51 insertions(+), 4 deletions(-)
@@ -1898,6 +1918,7 @@ static int write_loose_object(const struct object_id *oid, char *hdr,the_hash_algo->update_fn(&c,hdr,hdrlen);/* Then the data itself.. */+buf=in_stream->read(in_stream->data,&len);stream.next_in=(void*)buf;stream.avail_in=len;do{
@@ -1960,6 +1981,13 @@ int write_object_file_flags(const void *buf, unsigned long len,{charhdr[MAX_HEADER_LEN];inthdrlen=sizeof(hdr);+structinput_streamin_stream={+.read=read_input_stream_from_buffer,+.data=(void*)&(structinput_data_from_buffer){+.buf=buf,+.len=len,+},+};/* Normally if we have it in the pack then we do not bother writing*itoutinto.git/objects/??/?{38}file.
@@ -1968,7 +1996,7 @@ int write_object_file_flags(const void *buf, unsigned long len,&hdrlen);if(freshen_packed_object(oid)||freshen_loose_object(oid))return0;-returnwrite_loose_object(oid,hdr,hdrlen,buf,len,0,flags);+returnwrite_loose_object(oid,hdr,hdrlen,&in_stream,0,flags);}inthash_object_file_literally(constvoid*buf,unsignedlonglen,
@@ -1977,6 +2005,13 @@ int hash_object_file_literally(const void *buf, unsigned long len,{char*header;inthdrlen,status=0;+structinput_streamin_stream={+.read=read_input_stream_from_buffer,+.data=(void*)&(structinput_data_from_buffer){+.buf=buf,+.len=len,+},+};/* type string, SP, %lu of the length plus NUL must fit this */hdrlen=strlen(type)+MAX_HEADER_LEN;
@@ -1988,7 +2023,7 @@ int hash_object_file_literally(const void *buf, unsigned long len,gotocleanup;if(freshen_packed_object(oid)||freshen_loose_object(oid))gotocleanup;-status=write_loose_object(oid,header,hdrlen,buf,len,0,0);+status=write_loose_object(oid,header,hdrlen,&in_stream,0,0);cleanup:free(header);
@@ -2003,14 +2038,21 @@ int force_object_loose(const struct object_id *oid, time_t mtime)charhdr[MAX_HEADER_LEN];inthdrlen;intret;+structinput_data_from_bufferdata;+structinput_streamin_stream={+.read=read_input_stream_from_buffer,+.data=&data,+};if(has_loose_object(oid))return0;buf=read_object(the_repository,oid,&type,&len);if(!buf)returnerror(_("cannot read object for %s"),oid_to_hex(oid));+data.buf=buf;+data.len=len;hdrlen=xsnprintf(hdr,sizeof(hdr),"%s %"PRIuMAX,type_name(type),(uintmax_t)len)+1;-ret=write_loose_object(oid,hdr,hdrlen,buf,len,mtime,0);+ret=write_loose_object(oid,hdr,hdrlen,&in_stream,mtime,0);free(buf);returnret;
From: Han Xin <redacted>
We will use "write_loose_object()" later to handle large blob object,
which needs to work in dry_run mode.
Helped-by: Jiang Xin [off-list ref]
Signed-off-by: Han Xin <redacted>
---
object-file.c | 32 +++++++++++++++++++-------------
1 file changed, 19 insertions(+), 13 deletions(-)
@@ -1894,14 +1895,16 @@ static int write_loose_object(const struct object_id *oid, char *hdr,loose_object_path(the_repository,&filename,oid);-fd=create_tmpfile(&tmp_file,filename.buf);-if(fd<0){-if(flags&HASH_SILENT)-return-1;-elseif(errno==EACCES)-returnerror(_("insufficient permission for adding an object to repository database %s"),get_object_directory());-else-returnerror_errno(_("unable to create temporary file"));+if(!dry_run){+fd=create_tmpfile(&tmp_file,filename.buf);+if(fd<0){+if(flags&HASH_SILENT)+return-1;+elseif(errno==EACCES)+returnerror(_("insufficient permission for adding an object to repository database %s"),get_object_directory());+else+returnerror_errno(_("unable to create temporary file"));+}}/* Set it up */
@@ -1925,7 +1928,7 @@ static int write_loose_object(const struct object_id *oid, char *hdr,unsignedchar*in0=stream.next_in;ret=git_deflate(&stream,Z_FINISH);the_hash_algo->update_fn(&c,in0,stream.next_in-in0);-if(write_buffer(fd,compressed,stream.next_out-compressed)<0)+if(!dry_run&&write_buffer(fd,compressed,stream.next_out-compressed)<0)die(_("unable to write loose object file"));stream.next_out=compressed;stream.avail_out=sizeof(compressed);
@@ -1943,6 +1946,9 @@ static int write_loose_object(const struct object_id *oid, char *hdr,die(_("confused by unstable object source data for %s"),oid_to_hex(oid));+if(dry_run)+return0;+close_loose_object(fd);if(mtime){
@@ -1996,7 +2002,7 @@ int write_object_file_flags(const void *buf, unsigned long len,&hdrlen);if(freshen_packed_object(oid)||freshen_loose_object(oid))return0;-returnwrite_loose_object(oid,hdr,hdrlen,&in_stream,0,flags);+returnwrite_loose_object(oid,hdr,hdrlen,&in_stream,0,0,flags);}inthash_object_file_literally(constvoid*buf,unsignedlonglen,
@@ -2023,7 +2029,7 @@ int hash_object_file_literally(const void *buf, unsigned long len,gotocleanup;if(freshen_packed_object(oid)||freshen_loose_object(oid))gotocleanup;-status=write_loose_object(oid,header,hdrlen,&in_stream,0,0);+status=write_loose_object(oid,header,hdrlen,&in_stream,0,0,0);cleanup:free(header);
From: Han Xin <redacted>
When read input stream, oid can't get before reading all, and it will be
filled after reading.
Helped-by: Jiang Xin [off-list ref]
Signed-off-by: Han Xin <redacted>
---
object-file.c | 34 ++++++++++++++++++++++++++++++++--
1 file changed, 32 insertions(+), 2 deletions(-)
@@ -1893,7 +1893,13 @@ static int write_loose_object(const struct object_id *oid, char *hdr,constchar*buf;unsignedlonglen;-loose_object_path(the_repository,&filename,oid);+if(is_null_oid(oid)){+/* When oid is not determined, save tmp file to odb path. */+strbuf_reset(&filename);+strbuf_addstr(&filename,the_repository->objects->odb->path);+strbuf_addch(&filename,'/');+}else+loose_object_path(the_repository,&filename,oid);if(!dry_run){fd=create_tmpfile(&tmp_file,filename.buf);
@@ -1942,7 +1948,7 @@ static int write_loose_object(const struct object_id *oid, char *hdr,die(_("deflateEnd on object %s failed (%d)"),oid_to_hex(oid),ret);the_hash_algo->final_oid_fn(¶no_oid,&c);-if(!oideq(oid,¶no_oid))+if(!is_null_oid(oid)&&!oideq(oid,¶no_oid))die(_("confused by unstable object source data for %s"),oid_to_hex(oid));
@@ -1951,6 +1957,30 @@ static int write_loose_object(const struct object_id *oid, char *hdr,close_loose_object(fd);+if(is_null_oid(oid)){+intdirlen;++/* copy oid */+oidcpy((structobject_id*)oid,¶no_oid);+/* We get the oid now */+loose_object_path(the_repository,&filename,oid);++dirlen=directory_size(filename.buf);+if(dirlen){+structstrbufdir=STRBUF_INIT;+/*+*Makesurethedirectoryexists;notethatthe+*contentsofthebufferareundefinedaftermkstemp+*returnsanerror,sowehavetorewritethewhole+*bufferfromscratch.+*/+strbuf_reset(&dir);+strbuf_add(&dir,filename.buf,dirlen-1);+if(mkdir(dir.buf,0777)&&errno!=EEXIST)+return-1;+}+}+if(mtime){structutimbufutb;utb.actime=mtime;
From: Han Xin <redacted>
Read input stream repeatedly in write_loose_object() unless reach the
end, so that we can divide the large blob write into many small blocks.
Signed-off-by: Han Xin <redacted>
---
object-file.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
@@ -1891,7 +1891,7 @@ static int write_loose_object(const struct object_id *oid, char *hdr,staticstructstrbuftmp_file=STRBUF_INIT;staticstructstrbuffilename=STRBUF_INIT;constchar*buf;-unsignedlonglen;+intflush=0;if(is_null_oid(oid)){/* When oid is not determined, save tmp file to odb path. */
@@ -1927,12 +1927,16 @@ static int write_loose_object(const struct object_id *oid, char *hdr,the_hash_algo->update_fn(&c,hdr,hdrlen);/* Then the data itself.. */-buf=in_stream->read(in_stream->data,&len);-stream.next_in=(void*)buf;-stream.avail_in=len;do{unsignedchar*in0=stream.next_in;-ret=git_deflate(&stream,Z_FINISH);+if(!stream.avail_in){+if((buf=in_stream->read(in_stream->data,&stream.avail_in))){+stream.next_in=(void*)buf;+in0=(unsignedchar*)buf;+}else+flush=Z_FINISH;+}+ret=git_deflate(&stream,flush);the_hash_algo->update_fn(&c,in0,stream.next_in-in0);if(!dry_run&&write_buffer(fd,compressed,stream.next_out-compressed)<0)die(_("unable to write loose object file"));
From: Han Xin <redacted>
For large loose object files, that should be possible to stream it
direct to disk with "write_loose_object()".
Unlike "write_object_file()", you need to implement an "input_stream"
instead of giving void *buf.
Signed-off-by: Han Xin <redacted>
---
object-file.c | 8 ++++----
object-store.h | 5 +++++
2 files changed, 9 insertions(+), 4 deletions(-)
From: Han Xin <redacted>
When calling "unpack_non_delta_entry()", will allocate full memory for
the whole size of the unpacked object and write the buffer to loose file
on disk. This may lead to OOM for the git-unpack-objects process when
unpacking a very large object.
In function "unpack_delta_entry()", will also allocate full memory to
buffer the whole delta, but since there will be no delta for an object
larger than "core.bigFileThreshold", this issue is moderate.
To resolve the OOM issue in "git-unpack-objects", we can unpack large
object to file in stream, and use "core.bigFileThreshold" to avoid OOM
limits when called "get_data()".
Signed-off-by: Han Xin <redacted>
---
builtin/unpack-objects.c | 76 ++++++++++++++++++++++++-
t/t5590-receive-unpack-objects.sh | 92 +++++++++++++++++++++++++++++++
2 files changed, 167 insertions(+), 1 deletion(-)
create mode 100755 t/t5590-receive-unpack-objects.sh
@@ -320,11 +320,85 @@ static void added_object(unsigned nr, enum object_type type,}}+structinput_data_from_zstream{+git_zstream*zstream;+unsignedcharbuf[4096];+intstatus;+};++staticconstchar*read_inflate_in_stream(void*data,unsignedlong*readlen)+{+structinput_data_from_zstream*input=data;+git_zstream*zstream=input->zstream;+void*in=fill(1);++if(!len||input->status==Z_STREAM_END){+*readlen=0;+returnNULL;+}++zstream->next_out=input->buf;+zstream->avail_out=sizeof(input->buf);+zstream->next_in=in;+zstream->avail_in=len;++input->status=git_inflate(zstream,0);+use(len-zstream->avail_in);+*readlen=sizeof(input->buf)-zstream->avail_out;++return(constchar*)input->buf;+}++staticvoidwrite_stream_blob(unsignednr,unsignedlongsize)+{+charhdr[32];+inthdrlen;+git_zstreamzstream;+structinput_data_from_zstreamdata;+structinput_streamin_stream={+.read=read_inflate_in_stream,+.data=&data,+};+structobject_id*oid=&obj_list[nr].oid;+intret;++memset(&zstream,0,sizeof(zstream));+memset(&data,0,sizeof(data));+data.zstream=&zstream;+git_inflate_init(&zstream);++/* Generate the header */+hdrlen=xsnprintf(hdr,sizeof(hdr),"%s %"PRIuMAX,type_name(OBJ_BLOB),(uintmax_t)size)+1;++if((ret=write_loose_object(oid,hdr,hdrlen,&in_stream,dry_run,0,0)))+die(_("failed to write object in stream %d"),ret);++if(zstream.total_out!=size||data.status!=Z_STREAM_END)+die(_("inflate returned %d"),data.status);+git_inflate_end(&zstream);++if(strict&&!dry_run){+structblob*blob=lookup_blob(the_repository,oid);+if(blob)+blob->object.flags|=FLAG_WRITTEN;+else+die("invalid blob object from stream");+}+obj_list[nr].obj=NULL;+}+staticvoidunpack_non_delta_entry(enumobject_typetype,unsignedlongsize,unsignednr){-void*buf=get_data(size);+void*buf;++/* Write large blob in stream without allocating full buffer. */+if(type==OBJ_BLOB&&size>big_file_threshold){+write_stream_blob(nr,size);+return;+}+buf=get_data(size);if(!dry_run&&buf)write_object(nr,type,buf,size);else
@@ -0,0 +1,92 @@+#!/bin/sh+#+# Copyright (c) 2021 Han Xin+#++test_description='Test unpack-objects when receive pack'++GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main+exportGIT_TEST_DEFAULT_INITIAL_BRANCH_NAME++../test-lib.sh++test_expect_success"create commit with big blobs (1.5 MB)"'+test-toolgenrandomfoo1500000>big-blob&&+test_commit--appendfoobig-blob&&+test-toolgenrandombar1500000>big-blob&&+test_commit--appendbarbig-blob&&+(+cd.git&&+findobjects/??-typef|sort+)>expect&&+gitrepack-ad+'++test_expect_success'setup GIT_ALLOC_LIMIT to 1MB''+GIT_ALLOC_LIMIT=1m&&+exportGIT_ALLOC_LIMIT+'++test_expect_success'prepare dest repository''+gitinit--baredest.git&&+git-Cdest.gitconfigcore.bigFileThreshold2m&&+git-Cdest.gitconfigreceive.unpacklimit100+'++test_expect_success'fail to push: cannot allocate''+test_must_failgitpushdest.gitHEAD2>err&&+test_i18ngrep"remote: fatal: attempting to allocate"err&&+(+cddest.git&&+findobjects/??-typef|sort+)>actual&&+!test_cmpexpectactual+'++test_expect_success'set a lower bigfile threshold''+git-Cdest.gitconfigcore.bigFileThreshold1m+'++test_expect_success'unpack big object in stream''+gitpushdest.gitHEAD&&+git-Cdest.gitfsck&&+(+cddest.git&&+findobjects/??-typef|sort+)>actual&&+test_cmpexpectactual+'++test_expect_success'setup for unpack-objects dry-run test''+PACK=$(echomain|gitpack-objects--progress--revstest)&&+unsetGIT_ALLOC_LIMIT&&+gitinit--bareunpack-test.git+'++test_expect_success'unpack-objects dry-run with large threshold''+(+cdunpack-test.git&&+gitconfigcore.bigFileThreshold2m&&+gitunpack-objects-n<../test-$PACK.pack+)&&+(+cdunpack-test.git&&+findobjects/-typef+)>actual&&+test_must_be_emptyactual+'++test_expect_success'unpack-objects dry-run with small threshold''+(+cdunpack-test.git&&+gitconfigcore.bigFileThreshold1m&&+gitunpack-objects-n<../test-$PACK.pack+)&&+(+cdunpack-test.git&&+findobjects/-typef+)>actual&&+test_must_be_emptyactual+'++test_done
On Fri, Nov 12, 2021 at 5:43 PM Han Xin [off-list ref] wrote:
From: Han Xin <redacted>
It would be better to provide a cover letter describing changes in v2, such as:
* Make "write_loose_object()" a public method, so we can
reuse it in "unpack_non_delta_entry()".
(But I doubt we can use "write_object_file_flags()" public
function, without make this change.)
* Add an new interface "input_stream" as an argument for
"write_loose_object()", so that we can feed data to
"write_loose_object()" from buffer or from zlib stream.
Refactor write_loose_object() to support inputstream, in the same way
that zlib reading is chunked.
In the beginning of your commit log, you should describe the problem, such as:
We used to read the full content of a blob into buffer in
"unpack_non_delta_entry()" by calling:
void *buf = get_data(size);
This will consume lots of memory for a very big blob object.
quoted hunk
Using "in_stream" instead of "void *buf", we needn't to allocate enough
memory in advance, and only part of the contents will be read when
called "in_stream.read()".
Helped-by: Jiang Xin [off-list ref]
Signed-off-by: Han Xin <redacted>
---
object-file.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++----
object-store.h | 5 +++++
2 files changed, 51 insertions(+), 4 deletions(-)
Can we use the same prototype as the original: "const void *buf" ?
quoted hunk
+ unsigned long len;
loose_object_path(the_repository, &filename, oid);
@@ -1898,6 +1918,7 @@ static int write_loose_object(const struct object_id *oid, char *hdr, the_hash_algo->update_fn(&c, hdr, hdrlen); /* Then the data itself.. */+ buf = in_stream->read(in_stream->data, &len); stream.next_in = (void *)buf; stream.avail_in = len; do {
@@ -1960,6 +1981,13 @@ int write_object_file_flags(const void *buf, unsigned long len, { char hdr[MAX_HEADER_LEN]; int hdrlen = sizeof(hdr);+ struct input_stream in_stream = {+ .read = read_input_stream_from_buffer,+ .data = (void *)&(struct input_data_from_buffer) {+ .buf = buf,+ .len = len,+ },+ }; /* Normally if we have it in the pack then we do not bother writing * it out into .git/objects/??/?{38} file.
@@ -1968,7 +1996,7 @@ int write_object_file_flags(const void *buf, unsigned long len, &hdrlen); if (freshen_packed_object(oid) || freshen_loose_object(oid)) return 0;- return write_loose_object(oid, hdr, hdrlen, buf, len, 0, flags);+ return write_loose_object(oid, hdr, hdrlen, &in_stream, 0, flags); } int hash_object_file_literally(const void *buf, unsigned long len,
@@ -1977,6 +2005,13 @@ int hash_object_file_literally(const void *buf, unsigned long len, { char *header; int hdrlen, status = 0;+ struct input_stream in_stream = {+ .read = read_input_stream_from_buffer,+ .data = (void *)&(struct input_data_from_buffer) {+ .buf = buf,+ .len = len,+ },+ }; /* type string, SP, %lu of the length plus NUL must fit this */ hdrlen = strlen(type) + MAX_HEADER_LEN;
@@ -1988,7 +2023,7 @@ int hash_object_file_literally(const void *buf, unsigned long len, goto cleanup; if (freshen_packed_object(oid) || freshen_loose_object(oid)) goto cleanup;- status = write_loose_object(oid, header, hdrlen, buf, len, 0, 0);+ status = write_loose_object(oid, header, hdrlen, &in_stream, 0, 0); cleanup: free(header);
On Fri, Nov 12, 2021 at 5:42 PM Han Xin [off-list ref] wrote:
From: Han Xin <redacted>
We will use "write_loose_object()" later to handle large blob object,
which needs to work in dry_run mode.
The dry_run mode comes from "builtin/unpack-object.c", throw the
buffer read from "get_data()".
So why not add "dry_run" to "get_data()" instead?
If we have a dry_run version of get_data, such as "get_data(size,
dry_run)", we do not have to add dry_run mode for ”
write_loose_object()".
See: git grep -A5 get_data builtin/unpack-objects.c
builtin/unpack-objects.c: void *buf = get_data(size);
builtin/unpack-objects.c-
builtin/unpack-objects.c- if (!dry_run && buf)
builtin/unpack-objects.c- write_object(nr, type, buf, size);
builtin/unpack-objects.c- else
builtin/unpack-objects.c- free(buf);
--
builtin/unpack-objects.c: delta_data = get_data(delta_size);
builtin/unpack-objects.c- if (dry_run || !delta_data) {
builtin/unpack-objects.c- free(delta_data);
builtin/unpack-objects.c- return;
builtin/unpack-objects.c- }
--
builtin/unpack-objects.c: delta_data = get_data(delta_size);
builtin/unpack-objects.c- if (dry_run || !delta_data) {
builtin/unpack-objects.c- free(delta_data);
builtin/unpack-objects.c- return;
builtin/unpack-objects.c- }
@@ -1894,14 +1895,16 @@ static int write_loose_object(const struct object_id *oid, char *hdr,loose_object_path(the_repository,&filename,oid);-fd=create_tmpfile(&tmp_file,filename.buf);-if(fd<0){-if(flags&HASH_SILENT)-return-1;-elseif(errno==EACCES)-returnerror(_("insufficient permission for adding an object to repository database %s"),get_object_directory());-else-returnerror_errno(_("unable to create temporary file"));+if(!dry_run){+fd=create_tmpfile(&tmp_file,filename.buf);+if(fd<0){+if(flags&HASH_SILENT)+return-1;+elseif(errno==EACCES)+returnerror(_("insufficient permission for adding an object to repository database %s"),get_object_directory());+else+returnerror_errno(_("unable to create temporary file"));+}}/* Set it up */
@@ -1925,7 +1928,7 @@ static int write_loose_object(const struct object_id *oid, char *hdr,unsignedchar*in0=stream.next_in;ret=git_deflate(&stream,Z_FINISH);the_hash_algo->update_fn(&c,in0,stream.next_in-in0);-if(write_buffer(fd,compressed,stream.next_out-compressed)<0)+if(!dry_run&&write_buffer(fd,compressed,stream.next_out-compressed)<0)die(_("unable to write loose object file"));stream.next_out=compressed;stream.avail_out=sizeof(compressed);
@@ -1943,6 +1946,9 @@ static int write_loose_object(const struct object_id *oid, char *hdr,die(_("confused by unstable object source data for %s"),oid_to_hex(oid));+if(dry_run)+return0;+close_loose_object(fd);if(mtime){
@@ -1996,7 +2002,7 @@ int write_object_file_flags(const void *buf, unsigned long len,&hdrlen);if(freshen_packed_object(oid)||freshen_loose_object(oid))return0;-returnwrite_loose_object(oid,hdr,hdrlen,&in_stream,0,flags);+returnwrite_loose_object(oid,hdr,hdrlen,&in_stream,0,0,flags);}inthash_object_file_literally(constvoid*buf,unsignedlonglen,
@@ -2023,7 +2029,7 @@ int hash_object_file_literally(const void *buf, unsigned long len,gotocleanup;if(freshen_packed_object(oid)||freshen_loose_object(oid))gotocleanup;-status=write_loose_object(oid,header,hdrlen,&in_stream,0,0);+status=write_loose_object(oid,header,hdrlen,&in_stream,0,0,0);cleanup:free(header);
@@ -1893,7 +1893,13 @@ static int write_loose_object(const struct object_id *oid, char *hdr,constchar*buf;unsignedlonglen;-loose_object_path(the_repository,&filename,oid);+if(is_null_oid(oid)){+/* When oid is not determined, save tmp file to odb path. */+strbuf_reset(&filename);+strbuf_addstr(&filename,the_repository->objects->odb->path);+strbuf_addch(&filename,'/');+}else+loose_object_path(the_repository,&filename,oid);if(!dry_run){fd=create_tmpfile(&tmp_file,filename.buf);
@@ -1942,7 +1948,7 @@ static int write_loose_object(const struct object_id *oid, char *hdr,die(_("deflateEnd on object %s failed (%d)"),oid_to_hex(oid),ret);the_hash_algo->final_oid_fn(¶no_oid,&c);-if(!oideq(oid,¶no_oid))+if(!is_null_oid(oid)&&!oideq(oid,¶no_oid))die(_("confused by unstable object source data for %s"),oid_to_hex(oid));
@@ -1951,6 +1957,30 @@ static int write_loose_object(const struct object_id *oid, char *hdr,close_loose_object(fd);+if(is_null_oid(oid)){+intdirlen;++/* copy oid */+oidcpy((structobject_id*)oid,¶no_oid);+/* We get the oid now */+loose_object_path(the_repository,&filename,oid);++dirlen=directory_size(filename.buf);+if(dirlen){+structstrbufdir=STRBUF_INIT;+/*+*Makesurethedirectoryexists;notethatthe+*contentsofthebufferareundefinedaftermkstemp+*returnsanerror,sowehavetorewritethewhole+*bufferfromscratch.+*/+strbuf_reset(&dir);+strbuf_add(&dir,filename.buf,dirlen-1);+if(mkdir(dir.buf,0777)&&errno!=EEXIST)+return-1;+}+}+if(mtime){structutimbufutb;utb.actime=mtime;--
On Fri, Nov 12, 2021 at 5:43 PM Han Xin [off-list ref] wrote:
From: Han Xin <redacted>
Read input stream repeatedly in write_loose_object() unless reach the
end, so that we can divide the large blob write into many small blocks.
In order to prepare the stream version of "write_loose_object()", we need ...
@@ -1891,7 +1891,7 @@ static int write_loose_object(const struct object_id *oid, char *hdr,staticstructstrbuftmp_file=STRBUF_INIT;staticstructstrbuffilename=STRBUF_INIT;constchar*buf;-unsignedlonglen;+intflush=0;if(is_null_oid(oid)){/* When oid is not determined, save tmp file to odb path. */
@@ -1927,12 +1927,16 @@ static int write_loose_object(const struct object_id *oid, char *hdr,the_hash_algo->update_fn(&c,hdr,hdrlen);/* Then the data itself.. */-buf=in_stream->read(in_stream->data,&len);-stream.next_in=(void*)buf;-stream.avail_in=len;do{unsignedchar*in0=stream.next_in;-ret=git_deflate(&stream,Z_FINISH);+if(!stream.avail_in){+if((buf=in_stream->read(in_stream->data,&stream.avail_in))){
if ((buf = in_stream->read(in_stream->data, &stream.avail_in)) != NULL) {
Or split this long line into:
buf = in_stream->read(in_stream->data, &stream.avail_in);
if (buf) {
On Fri, Nov 12, 2021 at 5:42 PM Han Xin [off-list ref] wrote:
quoted hunk
From: Han Xin <redacted>
When calling "unpack_non_delta_entry()", will allocate full memory for
the whole size of the unpacked object and write the buffer to loose file
on disk. This may lead to OOM for the git-unpack-objects process when
unpacking a very large object.
In function "unpack_delta_entry()", will also allocate full memory to
buffer the whole delta, but since there will be no delta for an object
larger than "core.bigFileThreshold", this issue is moderate.
To resolve the OOM issue in "git-unpack-objects", we can unpack large
object to file in stream, and use "core.bigFileThreshold" to avoid OOM
limits when called "get_data()".
Signed-off-by: Han Xin <redacted>
---
builtin/unpack-objects.c | 76 ++++++++++++++++++++++++-
t/t5590-receive-unpack-objects.sh | 92 +++++++++++++++++++++++++++++++
2 files changed, 167 insertions(+), 1 deletion(-)
create mode 100755 t/t5590-receive-unpack-objects.sh
@@ -320,11 +320,85 @@ static void added_object(unsigned nr, enum object_type type,}}+structinput_data_from_zstream{+git_zstream*zstream;+unsignedcharbuf[4096];+intstatus;+};++staticconstchar*read_inflate_in_stream(void*data,unsignedlong*readlen)+{+structinput_data_from_zstream*input=data;+git_zstream*zstream=input->zstream;+void*in=fill(1);++if(!len||input->status==Z_STREAM_END){+*readlen=0;+returnNULL;+}++zstream->next_out=input->buf;+zstream->avail_out=sizeof(input->buf);+zstream->next_in=in;+zstream->avail_in=len;++input->status=git_inflate(zstream,0);+use(len-zstream->avail_in);+*readlen=sizeof(input->buf)-zstream->avail_out;++return(constchar*)input->buf;+}++staticvoidwrite_stream_blob(unsignednr,unsignedlongsize)+{+charhdr[32];+inthdrlen;+git_zstreamzstream;+structinput_data_from_zstreamdata;+structinput_streamin_stream={+.read=read_inflate_in_stream,+.data=&data,+};+structobject_id*oid=&obj_list[nr].oid;+intret;++memset(&zstream,0,sizeof(zstream));+memset(&data,0,sizeof(data));+data.zstream=&zstream;+git_inflate_init(&zstream);++/* Generate the header */+hdrlen=xsnprintf(hdr,sizeof(hdr),"%s %"PRIuMAX,type_name(OBJ_BLOB),(uintmax_t)size)+1;++if((ret=write_loose_object(oid,hdr,hdrlen,&in_stream,dry_run,0,0)))+die(_("failed to write object in stream %d"),ret);++if(zstream.total_out!=size||data.status!=Z_STREAM_END)+die(_("inflate returned %d"),data.status);+git_inflate_end(&zstream);++if(strict&&!dry_run){+structblob*blob=lookup_blob(the_repository,oid);+if(blob)+blob->object.flags|=FLAG_WRITTEN;+else+die("invalid blob object from stream");+}+obj_list[nr].obj=NULL;+}+staticvoidunpack_non_delta_entry(enumobject_typetype,unsignedlongsize,unsignednr){-void*buf=get_data(size);+void*buf;++/* Write large blob in stream without allocating full buffer. */+if(type==OBJ_BLOB&&size>big_file_threshold){
Default size of big_file_threshold is 512m. Can we use
"write_stream_blob" for all objects? Can we get a more suitable
threshold through some benchmark data?
From: Han Xin <redacted>
Although we do not recommend users push large binary files to the git repositories,
it's difficult to prevent them from doing so. Once, we found a problem with a surge
in memory usage on the server. The source of the problem is that a user submitted
a single object with a size of 15GB. Once someone initiates a git push, the git
process will immediately allocate 15G of memory, resulting in an OOM risk.
Through further analysis, we found that when we execute git unpack-objects, in
unpack_non_delta_entry(), "void *buf = get_data(size);" will directly allocate
memory equal to the size of the object. This is quite a scary thing, because the
pre-receive hook has not been executed at this time, and we cannot avoid this by hooks.
I got inspiration from the deflate process of zlib, maybe it would be a good idea
to change unpack-objects to stream deflate.
Changes since v2:
* Rewrite commit messages and make changes suggested by Jiang Xin.
* Remove the commit "object-file.c: add dry_run mode for write_loose_object()" and
use a new commit "unpack-objects.c: add dry_run mode for get_data()" instead.
Han Xin (5):
object-file: refactor write_loose_object() to read buffer from stream
object-file.c: handle undetermined oid in write_loose_object()
object-file.c: read stream in a loop in write_loose_object()
unpack-objects.c: add dry_run mode for get_data()
unpack-objects: unpack_non_delta_entry() read data in a stream
builtin/unpack-objects.c | 92 +++++++++++++++++++++++++--
object-file.c | 98 +++++++++++++++++++++++++----
object-store.h | 9 +++
t/t5590-unpack-non-delta-objects.sh | 76 ++++++++++++++++++++++
4 files changed, 257 insertions(+), 18 deletions(-)
create mode 100755 t/t5590-unpack-non-delta-objects.sh
Range-diff against v2:
1: 01672f50a0 ! 1: 8640b04f6d object-file: refactor write_loose_object() to support inputstream
@@ Metadata
Author: Han Xin [off-list ref]
## Commit message ##
- object-file: refactor write_loose_object() to support inputstream
+ object-file: refactor write_loose_object() to read buffer from stream
- Refactor write_loose_object() to support inputstream, in the same way
- that zlib reading is chunked.
+ 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.
- Using "in_stream" instead of "void *buf", we needn't to allocate enough
- memory in advance, and only part of the contents will be read when
- called "in_stream.read()".
+ This can be improved by feeding data to "write_loose_object()" in a
+ stream. The input stream is implemented as an interface. In the first
+ step, we make a simple implementation, feeding the entire buffer in the
+ "stream" to "write_loose_object()" as a refactor.
Helped-by: Jiang Xin [off-list ref]
Signed-off-by: Han Xin [off-list ref]
@@ object-file.c: static int create_tmpfile(struct strbuf *tmp, const char *filenam
return fd;
}
-+struct input_data_from_buffer {
-+ const char *buf;
++struct simple_input_stream_data {
++ const void *buf;
+ unsigned long len;
+};
+
-+static const char *read_input_stream_from_buffer(void *data, unsigned long *len)
++static const void *feed_simple_input_stream(struct input_stream *in_stream, unsigned long *len)
+{
-+ struct input_data_from_buffer *input = (struct input_data_from_buffer *)data;
++ struct simple_input_stream_data *data = in_stream->data;
+
-+ if (input->len == 0) {
++ if (data->len == 0) {
+ *len = 0;
+ return NULL;
+ }
-+ *len = input->len;
-+ input->len = 0;
-+ return input->buf;
++ *len = data->len;
++ data->len = 0;
++ return data->buf;
+}
+
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 *
struct object_id parano_oid;
static struct strbuf tmp_file = STRBUF_INIT;
static struct strbuf filename = STRBUF_INIT;
-+ const char *buf;
++ const void *buf;
+ unsigned long len;
loose_object_path(the_repository, &filename, oid);
@@ object-file.c: static int write_loose_object(const struct object_id *oid, char *
the_hash_algo->update_fn(&c, hdr, hdrlen);
/* Then the data itself.. */
-+ buf = in_stream->read(in_stream->data, &len);
++ buf = in_stream->read(in_stream, &len);
stream.next_in = (void *)buf;
stream.avail_in = len;
do {
@@ object-file.c: int write_object_file_flags(const void *buf, unsigned long len,
char hdr[MAX_HEADER_LEN];
int hdrlen = sizeof(hdr);
+ struct input_stream in_stream = {
-+ .read = read_input_stream_from_buffer,
-+ .data = (void *)&(struct input_data_from_buffer) {
++ .read = feed_simple_input_stream,
++ .data = (void *)&(struct simple_input_stream_data) {
+ .buf = buf,
+ .len = len,
+ },
@@ object-file.c: int hash_object_file_literally(const void *buf, unsigned long len
char *header;
int hdrlen, status = 0;
+ struct input_stream in_stream = {
-+ .read = read_input_stream_from_buffer,
-+ .data = (void *)&(struct input_data_from_buffer) {
++ .read = feed_simple_input_stream,
++ .data = (void *)&(struct simple_input_stream_data) {
+ .buf = buf,
+ .len = len,
+ },
@@ object-file.c: int force_object_loose(const struct object_id *oid, time_t mtime)
char hdr[MAX_HEADER_LEN];
int hdrlen;
int ret;
-+ struct input_data_from_buffer data;
++ struct simple_input_stream_data data;
+ struct input_stream in_stream = {
-+ .read = read_input_stream_from_buffer,
++ .read = feed_simple_input_stream,
+ .data = &data,
+ };
@@ object-store.h: struct object_directory {
};
+struct input_stream {
-+ const char *(*read)(void* data, unsigned long *len);
++ const void *(*read)(struct input_stream *, unsigned long *len);
+ void *data;
+};
+
2: a309b7e391 < -: ---------- object-file.c: add dry_run mode for write_loose_object()
3: b0a5b53710 ! 2: d4a2caf2bd object-file.c: handle nil oid in write_loose_object()
@@ Metadata
Author: Han Xin [off-list ref]
## Commit message ##
- object-file.c: handle nil oid in write_loose_object()
+ object-file.c: handle undetermined oid in write_loose_object()
- When read input stream, oid can't get before reading all, and it will be
- filled after reading.
+ When streaming a large blob object to "write_loose_object()", we have no
+ chance to run "write_object_file_prepare()" to calculate the oid in
+ advance. So we need to handle undetermined oid in function
+ "write_loose_object()".
+
+ In the original implementation, 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.
Helped-by: Jiang Xin [off-list ref]
Signed-off-by: Han Xin [off-list ref]
## object-file.c ##
@@ object-file.c: static int write_loose_object(const struct object_id *oid, char *hdr,
- const char *buf;
+ const void *buf;
unsigned long len;
- loose_object_path(the_repository, &filename, oid);
@@ object-file.c: static int write_loose_object(const struct object_id *oid, char *
+ strbuf_reset(&filename);
+ strbuf_addstr(&filename, the_repository->objects->odb->path);
+ strbuf_addch(&filename, '/');
-+ } else
++ } else {
+ loose_object_path(the_repository, &filename, oid);
++ }
- if (!dry_run) {
- fd = create_tmpfile(&tmp_file, filename.buf);
+ fd = create_tmpfile(&tmp_file, filename.buf);
+ if (fd < 0) {
@@ object-file.c: static int write_loose_object(const struct object_id *oid, char *hdr,
die(_("deflateEnd on object %s failed (%d)"), oid_to_hex(oid),
ret);
@@ object-file.c: static int write_loose_object(const struct object_id *oid, char *
die(_("confused by unstable object source data for %s"),
oid_to_hex(oid));
-@@ object-file.c: static int write_loose_object(const struct object_id *oid, char *hdr,
-
close_loose_object(fd);
+ if (is_null_oid(oid)) {
+ int dirlen;
+
-+ /* copy oid */
+ oidcpy((struct object_id *)oid, ¶no_oid);
-+ /* We get the oid now */
+ loose_object_path(the_repository, &filename, oid);
+
++ /* We finally know the object path, and create the missing dir. */
+ dirlen = directory_size(filename.buf);
+ if (dirlen) {
+ struct strbuf dir = STRBUF_INIT;
-+ /*
-+ * Make sure the directory exists; note that the
-+ * contents of the buffer are undefined after mkstemp
-+ * returns an error, so we have to rewrite the whole
-+ * buffer from scratch.
-+ */
-+ strbuf_reset(&dir);
+ strbuf_add(&dir, filename.buf, dirlen - 1);
+ if (mkdir(dir.buf, 0777) && errno != EEXIST)
+ return -1;
++ if (adjust_shared_perm(dir.buf))
++ return -1;
++ strbuf_release(&dir);
+ }
+ }
+
4: 09d438b692 ! 3: 2575900449 object-file.c: read input stream repeatedly in write_loose_object()
@@ Metadata
Author: Han Xin [off-list ref]
## Commit message ##
- object-file.c: read input stream repeatedly in write_loose_object()
+ object-file.c: read stream in a loop in write_loose_object()
- Read input stream repeatedly in write_loose_object() unless reach the
- end, so that we can divide the large blob write into many small blocks.
+ In order to prepare the stream version of "write_loose_object()", read
+ the input stream in a loop in "write_loose_object()", so that we can
+ feed the contents of large blob object to "write_loose_object()" using
+ a small fixed buffer.
+ Helped-by: Jiang Xin [off-list ref]
Signed-off-by: Han Xin [off-list ref]
## object-file.c ##
@@ object-file.c: static int write_loose_object(const struct object_id *oid, char *hdr,
static struct strbuf tmp_file = STRBUF_INIT;
static struct strbuf filename = STRBUF_INIT;
- const char *buf;
+ const void *buf;
- unsigned long len;
+ int flush = 0;
@@ object-file.c: static int write_loose_object(const struct object_id *oid, char *
the_hash_algo->update_fn(&c, hdr, hdrlen);
/* Then the data itself.. */
-- buf = in_stream->read(in_stream->data, &len);
+- buf = in_stream->read(in_stream, &len);
- stream.next_in = (void *)buf;
- stream.avail_in = len;
do {
unsigned char *in0 = stream.next_in;
- ret = git_deflate(&stream, Z_FINISH);
+ if (!stream.avail_in) {
-+ if ((buf = in_stream->read(in_stream->data, &stream.avail_in))) {
++ buf = in_stream->read(in_stream, &stream.avail_in);
++ if (buf) {
+ stream.next_in = (void *)buf;
+ in0 = (unsigned char *)buf;
-+ } else
++ } else {
+ flush = Z_FINISH;
++ }
+ }
+ ret = git_deflate(&stream, flush);
the_hash_algo->update_fn(&c, in0, stream.next_in - in0);
- if (!dry_run && write_buffer(fd, compressed, stream.next_out - compressed) < 0)
+ if (write_buffer(fd, compressed, stream.next_out - compressed) < 0)
die(_("unable to write loose object file"));
5: 9fb188d437 < -: ---------- object-store.h: add write_loose_object()
-: ---------- > 4: ca93ecc780 unpack-objects.c: add dry_run mode for get_data()
6: 80468a6fbc ! 5: 39a072ee2a unpack-objects: unpack large object in stream
@@ Metadata
Author: Han Xin [off-list ref]
## Commit message ##
- unpack-objects: unpack large object in stream
+ unpack-objects: unpack_non_delta_entry() read data in a stream
- When calling "unpack_non_delta_entry()", will allocate full memory for
- the whole size of the unpacked object and write the buffer to loose file
- on disk. This may lead to OOM for the git-unpack-objects process when
- unpacking a very large object.
+ 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.
- In function "unpack_delta_entry()", will also allocate full memory to
- buffer the whole delta, but since there will be no delta for an object
- larger than "core.bigFileThreshold", this issue is moderate.
+ By implementing a zstream version of input_stream interface, we can use
+ a small fixed buffer for "unpack_non_delta_entry()".
- To resolve the OOM issue in "git-unpack-objects", we can unpack large
- object to file in stream, and use "core.bigFileThreshold" to avoid OOM
- limits when called "get_data()".
+ However, unpack non-delta objects from a stream instead of from an entrie
+ buffer will have 10% performance penalty. Therefore, only unpack object
+ larger than the "big_file_threshold" in zstream. See the following
+ benchmarks:
+ $ hyperfine \
+ --prepare 'rm -rf dest.git && git init --bare dest.git' \
+ 'git -C dest.git unpack-objects <binary_320M.pack'
+ Benchmark 1: git -C dest.git unpack-objects <binary_320M.pack
+ Time (mean ± σ): 10.029 s ± 0.270 s [User: 8.265 s, System: 1.522 s]
+ Range (min … max): 9.786 s … 10.603 s 10 runs
+
+ $ hyperfine \
+ --prepare 'rm -rf dest.git && git init --bare dest.git' \
+ 'git -c core.bigFileThreshold=2m -C dest.git unpack-objects <binary_320M.pack'
+ Benchmark 1: git -c core.bigFileThreshold=2m -C dest.git unpack-objects <binary_320M.pack
+ Time (mean ± σ): 10.859 s ± 0.774 s [User: 8.813 s, System: 1.898 s]
+ Range (min … max): 9.884 s … 12.192 s 10 runs
+
+ $ hyperfine \
+ --prepare 'rm -rf dest.git && git init --bare dest.git' \
+ 'git -C dest.git unpack-objects <binary_96M.pack'
+ Benchmark 1: git -C dest.git unpack-objects <binary_96M.pack
+ Time (mean ± σ): 2.678 s ± 0.037 s [User: 2.205 s, System: 0.450 s]
+ Range (min … max): 2.639 s … 2.743 s 10 runs
+
+ $ hyperfine \
+ --prepare 'rm -rf dest.git && git init --bare dest.git' \
+ 'git -c core.bigFileThreshold=2m -C dest.git unpack-objects <binary_96M.pack'
+ Benchmark 1: git -c core.bigFileThreshold=2m -C dest.git unpack-objects <binary_96M.pack
+ Time (mean ± σ): 2.819 s ± 0.124 s [User: 2.216 s, System: 0.564 s]
+ Range (min … max): 2.679 s … 3.125 s 10 runs
+
+ Helped-by: Jiang Xin [off-list ref]
Signed-off-by: Han Xin [off-list ref]
## builtin/unpack-objects.c ##
@@ builtin/unpack-objects.c: static void added_object(unsigned nr, enum object_type
}
}
-+struct input_data_from_zstream {
++struct input_zstream_data {
+ git_zstream *zstream;
+ unsigned char buf[4096];
+ int status;
+};
+
-+static const char *read_inflate_in_stream(void *data, unsigned long *readlen)
++static const void *feed_input_zstream(struct input_stream *in_stream, unsigned long *readlen)
+{
-+ struct input_data_from_zstream *input = data;
-+ git_zstream *zstream = input->zstream;
++ struct input_zstream_data *data = in_stream->data;
++ git_zstream *zstream = data->zstream;
+ void *in = fill(1);
+
-+ if (!len || input->status == Z_STREAM_END) {
++ if (!len || data->status == Z_STREAM_END) {
+ *readlen = 0;
+ return NULL;
+ }
+
-+ zstream->next_out = input->buf;
-+ zstream->avail_out = sizeof(input->buf);
++ zstream->next_out = data->buf;
++ zstream->avail_out = sizeof(data->buf);
+ zstream->next_in = in;
+ zstream->avail_in = len;
+
-+ input->status = git_inflate(zstream, 0);
++ data->status = git_inflate(zstream, 0);
+ use(len - zstream->avail_in);
-+ *readlen = sizeof(input->buf) - zstream->avail_out;
++ *readlen = sizeof(data->buf) - zstream->avail_out;
+
-+ return (const char *)input->buf;
++ return data->buf;
+}
+
+static void write_stream_blob(unsigned nr, unsigned long size)
@@ builtin/unpack-objects.c: static void added_object(unsigned nr, enum object_type
+ char hdr[32];
+ int hdrlen;
+ git_zstream zstream;
-+ struct input_data_from_zstream data;
++ struct input_zstream_data data;
+ struct input_stream in_stream = {
-+ .read = read_inflate_in_stream,
++ .read = feed_input_zstream,
+ .data = &data,
+ };
+ struct object_id *oid = &obj_list[nr].oid;
@@ builtin/unpack-objects.c: static void added_object(unsigned nr, enum object_type
+ /* Generate the header */
+ hdrlen = xsnprintf(hdr, sizeof(hdr), "%s %"PRIuMAX, type_name(OBJ_BLOB), (uintmax_t)size) + 1;
+
-+ if ((ret = write_loose_object(oid, hdr, hdrlen, &in_stream, dry_run, 0, 0)))
++ if ((ret = write_loose_object(oid, hdr, hdrlen, &in_stream, 0, 0)))
+ die(_("failed to write object in stream %d"), ret);
+
+ if (zstream.total_out != size || data.status != Z_STREAM_END)
@@ builtin/unpack-objects.c: static void added_object(unsigned nr, enum object_type
static void unpack_non_delta_entry(enum object_type type, unsigned long size,
unsigned nr)
{
-- void *buf = get_data(size);
+- void *buf = get_data(size, dry_run);
+ void *buf;
+
+ /* Write large blob in stream without allocating full buffer. */
-+ if (type == OBJ_BLOB && size > big_file_threshold) {
++ if (!dry_run && type == OBJ_BLOB && size > big_file_threshold) {
+ write_stream_blob(nr, size);
+ return;
+ }
-+ buf = get_data(size);
++ buf = get_data(size, dry_run);
if (!dry_run && buf)
write_object(nr, type, buf, size);
else
- ## t/t5590-receive-unpack-objects.sh (new) ##
+ ## object-file.c ##
+@@ object-file.c: static const void *feed_simple_input_stream(struct input_stream *in_stream, unsi
+ return data->buf;
+ }
+
+-static int write_loose_object(const struct object_id *oid, char *hdr,
+- int hdrlen, struct input_stream *in_stream,
+- time_t mtime, unsigned flags)
++int write_loose_object(const struct object_id *oid, char *hdr,
++ int hdrlen, struct input_stream *in_stream,
++ time_t mtime, unsigned flags)
+ {
+ int fd, ret;
+ unsigned char compressed[4096];
+
+ ## object-store.h ##
+@@ object-store.h: int hash_object_file(const struct git_hash_algo *algo, const void *buf,
+ unsigned long len, const char *type,
+ struct object_id *oid);
+
++int write_loose_object(const struct object_id *oid, char *hdr,
++ int hdrlen, struct input_stream *in_stream,
++ time_t mtime, unsigned flags);
++
+ int write_object_file_flags(const void *buf, unsigned long len,
+ const char *type, struct object_id *oid,
+ unsigned flags);
+
+ ## t/t5590-unpack-non-delta-objects.sh (new) ##
@@
+#!/bin/sh
+#
@@ t/t5590-receive-unpack-objects.sh (new)
+ cd .git &&
+ find objects/?? -type f | sort
+ ) >expect &&
-+ git repack -ad
++ PACK=$(echo main | git pack-objects --progress --revs test)
+'
+
+test_expect_success 'setup GIT_ALLOC_LIMIT to 1MB' '
@@ t/t5590-receive-unpack-objects.sh (new)
+ git -C dest.git config receive.unpacklimit 100
+'
+
-+test_expect_success 'fail to push: cannot allocate' '
-+ test_must_fail git push dest.git HEAD 2>err &&
-+ test_i18ngrep "remote: fatal: attempting to allocate" err &&
++test_expect_success 'fail to unpack-objects: cannot allocate' '
++ test_must_fail git -C dest.git unpack-objects <test-$PACK.pack 2>err &&
++ test_i18ngrep "fatal: attempting to allocate" err &&
+ (
+ cd dest.git &&
+ find objects/?? -type f | sort
@@ t/t5590-receive-unpack-objects.sh (new)
+'
+
+test_expect_success 'unpack big object in stream' '
-+ git push dest.git HEAD &&
++ git -C dest.git unpack-objects <test-$PACK.pack &&
+ git -C dest.git fsck &&
+ (
+ cd dest.git &&
@@ t/t5590-receive-unpack-objects.sh (new)
+'
+
+test_expect_success 'setup for unpack-objects dry-run test' '
-+ PACK=$(echo main | git pack-objects --progress --revs test) &&
-+ unset GIT_ALLOC_LIMIT &&
+ git init --bare unpack-test.git
+'
+
-+test_expect_success 'unpack-objects dry-run with large threshold' '
-+ (
-+ cd unpack-test.git &&
-+ git config core.bigFileThreshold 2m &&
-+ git unpack-objects -n <../test-$PACK.pack
-+ ) &&
-+ (
-+ cd unpack-test.git &&
-+ find objects/ -type f
-+ ) >actual &&
-+ test_must_be_empty actual
-+'
-+
-+test_expect_success 'unpack-objects dry-run with small threshold' '
++test_expect_success 'unpack-objects dry-run' '
+ (
+ cd unpack-test.git &&
-+ git config core.bigFileThreshold 1m &&
+ git unpack-objects -n <../test-$PACK.pack
+ ) &&
+ (
--
2.34.0.6.g676eedc724
From: Han Xin <redacted>
When streaming a large blob object to "write_loose_object()", we have no
chance to run "write_object_file_prepare()" to calculate the oid in
advance. So we need to handle undetermined oid in function
"write_loose_object()".
In the original implementation, 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.
Helped-by: Jiang Xin [off-list ref]
Signed-off-by: Han Xin <redacted>
---
object-file.c | 30 ++++++++++++++++++++++++++++--
1 file changed, 28 insertions(+), 2 deletions(-)
@@ -1892,7 +1892,14 @@ static int write_loose_object(const struct object_id *oid, char *hdr,constvoid*buf;unsignedlonglen;-loose_object_path(the_repository,&filename,oid);+if(is_null_oid(oid)){+/* When oid is not determined, save tmp file to odb path. */+strbuf_reset(&filename);+strbuf_addstr(&filename,the_repository->objects->odb->path);+strbuf_addch(&filename,'/');+}else{+loose_object_path(the_repository,&filename,oid);+}fd=create_tmpfile(&tmp_file,filename.buf);if(fd<0){
@@ -1939,12 +1946,31 @@ static int write_loose_object(const struct object_id *oid, char *hdr,die(_("deflateEnd on object %s failed (%d)"),oid_to_hex(oid),ret);the_hash_algo->final_oid_fn(¶no_oid,&c);-if(!oideq(oid,¶no_oid))+if(!is_null_oid(oid)&&!oideq(oid,¶no_oid))die(_("confused by unstable object source data for %s"),oid_to_hex(oid));close_loose_object(fd);+if(is_null_oid(oid)){+intdirlen;++oidcpy((structobject_id*)oid,¶no_oid);+loose_object_path(the_repository,&filename,oid);++/* We finally know the object path, and create the missing dir. */+dirlen=directory_size(filename.buf);+if(dirlen){+structstrbufdir=STRBUF_INIT;+strbuf_add(&dir,filename.buf,dirlen-1);+if(mkdir(dir.buf,0777)&&errno!=EEXIST)+return-1;+if(adjust_shared_perm(dir.buf))+return-1;+strbuf_release(&dir);+}+}+if(mtime){structutimbufutb;utb.actime=mtime;
From: Han Xin <redacted>
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.
This can be improved by feeding data to "write_loose_object()" in a
stream. The input stream is implemented as an interface. In the first
step, we make a simple implementation, feeding the entire buffer in the
"stream" to "write_loose_object()" as a refactor.
Helped-by: Jiang Xin [off-list ref]
Signed-off-by: Han Xin <redacted>
---
object-file.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++----
object-store.h | 5 +++++
2 files changed, 51 insertions(+), 4 deletions(-)
@@ -1898,6 +1918,7 @@ static int write_loose_object(const struct object_id *oid, char *hdr,the_hash_algo->update_fn(&c,hdr,hdrlen);/* Then the data itself.. */+buf=in_stream->read(in_stream,&len);stream.next_in=(void*)buf;stream.avail_in=len;do{
@@ -1960,6 +1981,13 @@ int write_object_file_flags(const void *buf, unsigned long len,{charhdr[MAX_HEADER_LEN];inthdrlen=sizeof(hdr);+structinput_streamin_stream={+.read=feed_simple_input_stream,+.data=(void*)&(structsimple_input_stream_data){+.buf=buf,+.len=len,+},+};/* Normally if we have it in the pack then we do not bother writing*itoutinto.git/objects/??/?{38}file.
@@ -1968,7 +1996,7 @@ int write_object_file_flags(const void *buf, unsigned long len,&hdrlen);if(freshen_packed_object(oid)||freshen_loose_object(oid))return0;-returnwrite_loose_object(oid,hdr,hdrlen,buf,len,0,flags);+returnwrite_loose_object(oid,hdr,hdrlen,&in_stream,0,flags);}inthash_object_file_literally(constvoid*buf,unsignedlonglen,
@@ -1977,6 +2005,13 @@ int hash_object_file_literally(const void *buf, unsigned long len,{char*header;inthdrlen,status=0;+structinput_streamin_stream={+.read=feed_simple_input_stream,+.data=(void*)&(structsimple_input_stream_data){+.buf=buf,+.len=len,+},+};/* type string, SP, %lu of the length plus NUL must fit this */hdrlen=strlen(type)+MAX_HEADER_LEN;
@@ -1988,7 +2023,7 @@ int hash_object_file_literally(const void *buf, unsigned long len,gotocleanup;if(freshen_packed_object(oid)||freshen_loose_object(oid))gotocleanup;-status=write_loose_object(oid,header,hdrlen,buf,len,0,0);+status=write_loose_object(oid,header,hdrlen,&in_stream,0,0);cleanup:free(header);
@@ -2003,14 +2038,21 @@ int force_object_loose(const struct object_id *oid, time_t mtime)charhdr[MAX_HEADER_LEN];inthdrlen;intret;+structsimple_input_stream_datadata;+structinput_streamin_stream={+.read=feed_simple_input_stream,+.data=&data,+};if(has_loose_object(oid))return0;buf=read_object(the_repository,oid,&type,&len);if(!buf)returnerror(_("cannot read object for %s"),oid_to_hex(oid));+data.buf=buf;+data.len=len;hdrlen=xsnprintf(hdr,sizeof(hdr),"%s %"PRIuMAX,type_name(type),(uintmax_t)len)+1;-ret=write_loose_object(oid,hdr,hdrlen,buf,len,mtime,0);+ret=write_loose_object(oid,hdr,hdrlen,&in_stream,mtime,0);free(buf);returnret;
From: Han Xin <redacted>
In order to prepare the stream version of "write_loose_object()", read
the input stream in a loop in "write_loose_object()", so that we can
feed the contents of large blob object to "write_loose_object()" using
a small fixed buffer.
Helped-by: Jiang Xin [off-list ref]
Signed-off-by: Han Xin <redacted>
---
object-file.c | 16 +++++++++++-----
1 file changed, 11 insertions(+), 5 deletions(-)
@@ -1890,7 +1890,7 @@ static int write_loose_object(const struct object_id *oid, char *hdr,staticstructstrbuftmp_file=STRBUF_INIT;staticstructstrbuffilename=STRBUF_INIT;constvoid*buf;-unsignedlonglen;+intflush=0;if(is_null_oid(oid)){/* When oid is not determined, save tmp file to odb path. */
@@ -1925,12 +1925,18 @@ static int write_loose_object(const struct object_id *oid, char *hdr,the_hash_algo->update_fn(&c,hdr,hdrlen);/* Then the data itself.. */-buf=in_stream->read(in_stream,&len);-stream.next_in=(void*)buf;-stream.avail_in=len;do{unsignedchar*in0=stream.next_in;-ret=git_deflate(&stream,Z_FINISH);+if(!stream.avail_in){+buf=in_stream->read(in_stream,&stream.avail_in);+if(buf){+stream.next_in=(void*)buf;+in0=(unsignedchar*)buf;+}else{+flush=Z_FINISH;+}+}+ret=git_deflate(&stream,flush);the_hash_algo->update_fn(&c,in0,stream.next_in-in0);if(write_buffer(fd,compressed,stream.next_out-compressed)<0)die(_("unable to write loose object file"));
From: Han Xin <redacted>
In dry_run mode, "get_data()" is used to verify the inflation of data,
and the returned buffer will not be used at all and will be freed
immediately. Even in dry_run mode, it is dangerous to allocate a
full-size buffer for a large blob object. Therefore, only allocate a
low memory footprint when calling "get_data()" in dry_run mode.
Suggested-by: Jiang Xin <redacted>
Signed-off-by: Han Xin <redacted>
---
builtin/unpack-objects.c | 18 ++++++++++++------
1 file changed, 12 insertions(+), 6 deletions(-)
@@ -396,7 +402,7 @@ static void unpack_delta_entry(enum object_type type, unsigned long delta_size,if(base_offset<=0||base_offset>=obj_list[nr].offset)die("offset value out of bound for delta base object");-delta_data=get_data(delta_size);+delta_data=get_data(delta_size,dry_run);if(dry_run||!delta_data){free(delta_data);return;
From: Han Xin <redacted>
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.
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. Therefore, only unpack object
larger than the "big_file_threshold" in zstream. See the following
benchmarks:
$ hyperfine \
--prepare 'rm -rf dest.git && git init --bare dest.git' \
'git -C dest.git unpack-objects <binary_320M.pack'
Benchmark 1: git -C dest.git unpack-objects <binary_320M.pack
Time (mean ± σ): 10.029 s ± 0.270 s [User: 8.265 s, System: 1.522 s]
Range (min … max): 9.786 s … 10.603 s 10 runs
$ hyperfine \
--prepare 'rm -rf dest.git && git init --bare dest.git' \
'git -c core.bigFileThreshold=2m -C dest.git unpack-objects <binary_320M.pack'
Benchmark 1: git -c core.bigFileThreshold=2m -C dest.git unpack-objects <binary_320M.pack
Time (mean ± σ): 10.859 s ± 0.774 s [User: 8.813 s, System: 1.898 s]
Range (min … max): 9.884 s … 12.192 s 10 runs
$ hyperfine \
--prepare 'rm -rf dest.git && git init --bare dest.git' \
'git -C dest.git unpack-objects <binary_96M.pack'
Benchmark 1: git -C dest.git unpack-objects <binary_96M.pack
Time (mean ± σ): 2.678 s ± 0.037 s [User: 2.205 s, System: 0.450 s]
Range (min … max): 2.639 s … 2.743 s 10 runs
$ hyperfine \
--prepare 'rm -rf dest.git && git init --bare dest.git' \
'git -c core.bigFileThreshold=2m -C dest.git unpack-objects <binary_96M.pack'
Benchmark 1: git -c core.bigFileThreshold=2m -C dest.git unpack-objects <binary_96M.pack
Time (mean ± σ): 2.819 s ± 0.124 s [User: 2.216 s, System: 0.564 s]
Range (min … max): 2.679 s … 3.125 s 10 runs
Helped-by: Jiang Xin [off-list ref]
Signed-off-by: Han Xin <redacted>
---
builtin/unpack-objects.c | 76 ++++++++++++++++++++++++++++-
object-file.c | 6 +--
object-store.h | 4 ++
t/t5590-unpack-non-delta-objects.sh | 76 +++++++++++++++++++++++++++++
4 files changed, 158 insertions(+), 4 deletions(-)
create mode 100755 t/t5590-unpack-non-delta-objects.sh
@@ -326,11 +326,85 @@ static void added_object(unsigned nr, enum object_type type,}}+structinput_zstream_data{+git_zstream*zstream;+unsignedcharbuf[4096];+intstatus;+};++staticconstvoid*feed_input_zstream(structinput_stream*in_stream,unsignedlong*readlen)+{+structinput_zstream_data*data=in_stream->data;+git_zstream*zstream=data->zstream;+void*in=fill(1);++if(!len||data->status==Z_STREAM_END){+*readlen=0;+returnNULL;+}++zstream->next_out=data->buf;+zstream->avail_out=sizeof(data->buf);+zstream->next_in=in;+zstream->avail_in=len;++data->status=git_inflate(zstream,0);+use(len-zstream->avail_in);+*readlen=sizeof(data->buf)-zstream->avail_out;++returndata->buf;+}++staticvoidwrite_stream_blob(unsignednr,unsignedlongsize)+{+charhdr[32];+inthdrlen;+git_zstreamzstream;+structinput_zstream_datadata;+structinput_streamin_stream={+.read=feed_input_zstream,+.data=&data,+};+structobject_id*oid=&obj_list[nr].oid;+intret;++memset(&zstream,0,sizeof(zstream));+memset(&data,0,sizeof(data));+data.zstream=&zstream;+git_inflate_init(&zstream);++/* Generate the header */+hdrlen=xsnprintf(hdr,sizeof(hdr),"%s %"PRIuMAX,type_name(OBJ_BLOB),(uintmax_t)size)+1;++if((ret=write_loose_object(oid,hdr,hdrlen,&in_stream,0,0)))+die(_("failed to write object in stream %d"),ret);++if(zstream.total_out!=size||data.status!=Z_STREAM_END)+die(_("inflate returned %d"),data.status);+git_inflate_end(&zstream);++if(strict&&!dry_run){+structblob*blob=lookup_blob(the_repository,oid);+if(blob)+blob->object.flags|=FLAG_WRITTEN;+else+die("invalid blob object from stream");+}+obj_list[nr].obj=NULL;+}+staticvoidunpack_non_delta_entry(enumobject_typetype,unsignedlongsize,unsignednr){-void*buf=get_data(size,dry_run);+void*buf;++/* Write large blob in stream without allocating full buffer. */+if(!dry_run&&type==OBJ_BLOB&&size>big_file_threshold){+write_stream_blob(nr,size);+return;+}+buf=get_data(size,dry_run);if(!dry_run&&buf)write_object(nr,type,buf,size);else
@@ -0,0 +1,76 @@+#!/bin/sh+#+# Copyright (c) 2021 Han Xin+#++test_description='Test unpack-objects when receive pack'++GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main+exportGIT_TEST_DEFAULT_INITIAL_BRANCH_NAME++../test-lib.sh++test_expect_success"create commit with big blobs (1.5 MB)"'+test-toolgenrandomfoo1500000>big-blob&&+test_commit--appendfoobig-blob&&+test-toolgenrandombar1500000>big-blob&&+test_commit--appendbarbig-blob&&+(+cd.git&&+findobjects/??-typef|sort+)>expect&&+PACK=$(echomain|gitpack-objects--progress--revstest)+'++test_expect_success'setup GIT_ALLOC_LIMIT to 1MB''+GIT_ALLOC_LIMIT=1m&&+exportGIT_ALLOC_LIMIT+'++test_expect_success'prepare dest repository''+gitinit--baredest.git&&+git-Cdest.gitconfigcore.bigFileThreshold2m&&+git-Cdest.gitconfigreceive.unpacklimit100+'++test_expect_success'fail to unpack-objects: cannot allocate''+test_must_failgit-Cdest.gitunpack-objects<test-$PACK.pack2>err&&+test_i18ngrep"fatal: attempting to allocate"err&&+(+cddest.git&&+findobjects/??-typef|sort+)>actual&&+!test_cmpexpectactual+'++test_expect_success'set a lower bigfile threshold''+git-Cdest.gitconfigcore.bigFileThreshold1m+'++test_expect_success'unpack big object in stream''+git-Cdest.gitunpack-objects<test-$PACK.pack&&+git-Cdest.gitfsck&&+(+cddest.git&&+findobjects/??-typef|sort+)>actual&&+test_cmpexpectactual+'++test_expect_success'setup for unpack-objects dry-run test''+gitinit--bareunpack-test.git+'++test_expect_success'unpack-objects dry-run''+(+cdunpack-test.git&&+gitunpack-objects-n<../test-$PACK.pack+)&&+(+cdunpack-test.git&&+findobjects/-typef+)>actual&&+test_must_be_emptyactual+'++test_done
From: Han Xin <redacted>
Although we do not recommend users push large binary files to the git repositories,
it's difficult to prevent them from doing so. Once, we found a problem with a surge
in memory usage on the server. The source of the problem is that a user submitted
a single object with a size of 15GB. Once someone initiates a git push, the git
process will immediately allocate 15G of memory, resulting in an OOM risk.
Through further analysis, we found that when we execute git unpack-objects, in
unpack_non_delta_entry(), "void *buf = get_data(size);" will directly allocate
memory equal to the size of the object. This is quite a scary thing, because the
pre-receive hook has not been executed at this time, and we cannot avoid this by hooks.
I got inspiration from the deflate process of zlib, maybe it would be a good idea
to change unpack-objects to stream deflate.
Hi, Jeff.
I hope you can share with me how Github solves this problem.
As you said in your reply at：
https://lore.kernel.org/git/YVaw6agcPNclhws8@coredump.intra.peff.net/
"we don't have a match in unpack-objects, but we always run index-pack
on incoming packs".
In the original implementation of "index-pack", for objects larger than
big_file_threshold, "fixed_buf" with a size of 8192 will be used to
complete the calculation of "oid".
I tried the implementation in jk/no-more-unpack-objects, as you noted:
/* XXX This will expand too-large objects! */
if (!data)
data = new_data = get_data_from_pack(obj_entry);
If the conditions of --unpack are given, there will be risks here.
When I create an object larger than 1GB and execute index-pack, the
result is as follows:
$GIT_ALLOC_LIMIT=1024m git index-pack --unpack --stdin <large.pack
fatal: attempting to allocate 1228800001 over limit 1073741824
Looking forward to your reply.
From: Han Xin <redacted>
When streaming a large blob object to "write_loose_object()", we have no
chance to run "write_object_file_prepare()" to calculate the oid in
advance. So we need to handle undetermined oid in function
"write_loose_object()".
In the original implementation, 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.
My first reaction is to not write into .git/objects/ directly, but
instead make a .git/objects/tmp/ directory and write within that
directory. The idea is to prevent leaving stale files in the
.git/objects/ directory if the process terminates strangely (say,
a power outage or segfault).
If this was an interesting idea to pursue, it does leave a question:
should we clean up the tmp/ directory when it is empty? That would
require adding a check in finalize_object_file() that is probably
best left unchecked (the lstat() would add a cost per loose object
write that is probably too costly). I would rather leave an empty
tmp/ directory than add that cost per loose object write.
I suppose another way to do it would be to register the check as
an event at the end of the process, so we only check once, and
that only happens if we created a loose object with this streaming
method.
With all of these complications in mind, I think cleaning up the
stale tmp/ directory could (at the very least) be delayed to another
commit or patch series. Hopefully adding the directory is not too
much complication to add here.
- loose_object_path(the_repository, &filename, oid);
+ if (is_null_oid(oid)) {
+ /* When oid is not determined, save tmp file to odb path. */
+ strbuf_reset(&filename);
+ strbuf_addstr(&filename, the_repository->objects->odb->path);
+ strbuf_addch(&filename, '/');
Here, you could instead of the strbuf_addch() do
strbuf_add(&filename, "/tmp/", 5);
if (safe_create_leading_directories(filename.buf)) {
error(_("failed to create '%s'"));
strbuf_release(&filename);
return -1;
}
From: Jeff King <hidden> Date: 2021-11-29 19:14:25
On Mon, Nov 29, 2021 at 03:01:47PM +0800, Han Xin wrote:
Han Xin [off-list ref] writes:
quoted
From: Han Xin <redacted>
Although we do not recommend users push large binary files to the git repositories,
it's difficult to prevent them from doing so. Once, we found a problem with a surge
in memory usage on the server. The source of the problem is that a user submitted
a single object with a size of 15GB. Once someone initiates a git push, the git
process will immediately allocate 15G of memory, resulting in an OOM risk.
Through further analysis, we found that when we execute git unpack-objects, in
unpack_non_delta_entry(), "void *buf = get_data(size);" will directly allocate
memory equal to the size of the object. This is quite a scary thing, because the
pre-receive hook has not been executed at this time, and we cannot avoid this by hooks.
I got inspiration from the deflate process of zlib, maybe it would be a good idea
to change unpack-objects to stream deflate.
Hi, Jeff.
I hope you can share with me how Github solves this problem.
As you said in your reply at：
https://lore.kernel.org/git/YVaw6agcPNclhws8@coredump.intra.peff.net/
"we don't have a match in unpack-objects, but we always run index-pack
on incoming packs".
In the original implementation of "index-pack", for objects larger than
big_file_threshold, "fixed_buf" with a size of 8192 will be used to
complete the calculation of "oid".
We set transfer.unpackLimit to "1", so we never run unpack-objects at
all. We always run index-pack, and every push, no matter how small,
results in a pack.
We also set GIT_ALLOC_LIMIT to limit any single allocation. We also have
custom code in index-pack to detect large objects (where our definition
of "large" is 100MB by default):
- for large blobs, we do index it as normal, writing the oid out to a
file which is then processed by a pre-receive hook (since people
often push up large files accidentally, the hook generates a nice
error message, including finding the path at which the blob is
referenced)
- for other large objects, we die immediately (with an error message).
100MB commit messages aren't a common user error, and it closes off
a whole set of possible integer-overflow parsing attacks (e.g.,
index-pack in strict-mode will run every tree through fsck_tree(),
so there's otherwise nothing stopping you from having a 4GB filename
in a tree).
I tried the implementation in jk/no-more-unpack-objects, as you noted:
/* XXX This will expand too-large objects! */
if (!data)
data = new_data = get_data_from_pack(obj_entry);
If the conditions of --unpack are given, there will be risks here.
When I create an object larger than 1GB and execute index-pack, the
result is as follows:
$GIT_ALLOC_LIMIT=1024m git index-pack --unpack --stdin <large.pack
fatal: attempting to allocate 1228800001 over limit 1073741824
Yeah, that issue was one of the reasons I never sent the "index-pack
--unpack" code to the list. We don't actually use those patches at
GitHub. It was something I was working on for upstream but never
finished.
-Peff
From: Han Xin <redacted>
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.
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. Therefore, only unpack object
larger than the "big_file_threshold" in zstream. See the following
benchmarks:
$ hyperfine \
--prepare 'rm -rf dest.git && git init --bare dest.git' \
'git -C dest.git unpack-objects <binary_320M.pack'
Benchmark 1: git -C dest.git unpack-objects <binary_320M.pack
Time (mean ± σ): 10.029 s ± 0.270 s [User: 8.265 s, System: 1.522 s]
Range (min … max): 9.786 s … 10.603 s 10 runs
$ hyperfine \
--prepare 'rm -rf dest.git && git init --bare dest.git' \
'git -c core.bigFileThreshold=2m -C dest.git unpack-objects <binary_320M.pack'
Benchmark 1: git -c core.bigFileThreshold=2m -C dest.git unpack-objects <binary_320M.pack
Time (mean ± σ): 10.859 s ± 0.774 s [User: 8.813 s, System: 1.898 s]
Range (min … max): 9.884 s … 12.192 s 10 runs
It seems that you want us to compare this pair of results, and
hyperfine can assist with that by including multiple benchmarks
(with labels, using '-n') as follows:
$ hyperfine \
--prepare 'rm -rf dest.git && git init --bare dest.git' \
-n 'old' '~/_git/git-upstream/git -C dest.git unpack-objects <big.pack' \
-n 'new' '~/_git/git/git -C dest.git unpack-objects <big.pack' \
-n 'new (small threshold)' '~/_git/git/git -c core.bigfilethreshold=64k -C dest.git unpack-objects <big.pack'
Benchmark 1: old
Time (mean ± σ): 20.835 s ± 0.058 s [User: 14.510 s, System: 6.284 s]
Range (min … max): 20.741 s … 20.909 s 10 runs
Benchmark 2: new
Time (mean ± σ): 26.515 s ± 0.072 s [User: 19.783 s, System: 6.696 s]
Range (min … max): 26.419 s … 26.611 s 10 runs
Benchmark 3: new (small threshold)
Time (mean ± σ): 26.523 s ± 0.101 s [User: 19.805 s, System: 6.680 s]
Range (min … max): 26.416 s … 26.739 s 10 runs
Summary
'old' ran
1.27 ± 0.00 times faster than 'new'
1.27 ± 0.01 times faster than 'new (small threshold)'
(Here, 'old' is testing a compiled version of the latest 'master'
branch, while 'new' has your patches applied on top.)
Notice from this example I had a pack with many small objects (mostly
commits and trees) and I see that this change introduces significant
overhead to this case.
It would be nice to understand this overhead and fix it before taking
this change any further.
Thanks,
-Stolee
On Tue, Nov 30, 2021 at 3:12 AM Jeff King [off-list ref] wrote:
We set transfer.unpackLimit to "1", so we never run unpack-objects at
all. We always run index-pack, and every push, no matter how small,
results in a pack.
We also set GIT_ALLOC_LIMIT to limit any single allocation. We also have
custom code in index-pack to detect large objects (where our definition
of "large" is 100MB by default):
- for large blobs, we do index it as normal, writing the oid out to a
file which is then processed by a pre-receive hook (since people
often push up large files accidentally, the hook generates a nice
error message, including finding the path at which the blob is
referenced)
- for other large objects, we die immediately (with an error message).
100MB commit messages aren't a common user error, and it closes off
a whole set of possible integer-overflow parsing attacks (e.g.,
index-pack in strict-mode will run every tree through fsck_tree(),
so there's otherwise nothing stopping you from having a 4GB filename
in a tree).
Thank you very much for sharing.
The way Github handles it reminds me of what Shawn Pearce introduced in
"Scaling up JGit". I guess "mulit-pack-index" and "bitmap" must play an
important role in this.
I will seriously consider this solution, thanks a lot.
On Tue, Nov 30, 2021 at 1:37 AM Derrick Stolee [off-list ref] wrote:
On 11/21/2021 10:32 PM, Han Xin wrote:
quoted
From: Han Xin <redacted>
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.
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. Therefore, only unpack object
larger than the "big_file_threshold" in zstream. See the following
benchmarks:
$ hyperfine \
--prepare 'rm -rf dest.git && git init --bare dest.git' \
'git -C dest.git unpack-objects <binary_320M.pack'
Benchmark 1: git -C dest.git unpack-objects <binary_320M.pack
Time (mean ± σ): 10.029 s ± 0.270 s [User: 8.265 s, System: 1.522 s]
Range (min … max): 9.786 s … 10.603 s 10 runs
$ hyperfine \
--prepare 'rm -rf dest.git && git init --bare dest.git' \
'git -c core.bigFileThreshold=2m -C dest.git unpack-objects <binary_320M.pack'
Benchmark 1: git -c core.bigFileThreshold=2m -C dest.git unpack-objects <binary_320M.pack
Time (mean ± σ): 10.859 s ± 0.774 s [User: 8.813 s, System: 1.898 s]
Range (min … max): 9.884 s … 12.192 s 10 runs
It seems that you want us to compare this pair of results, and
hyperfine can assist with that by including multiple benchmarks
(with labels, using '-n') as follows:
$ hyperfine \
--prepare 'rm -rf dest.git && git init --bare dest.git' \
-n 'old' '~/_git/git-upstream/git -C dest.git unpack-objects <big.pack' \
-n 'new' '~/_git/git/git -C dest.git unpack-objects <big.pack' \
-n 'new (small threshold)' '~/_git/git/git -c core.bigfilethreshold=64k -C dest.git unpack-objects <big.pack'
Benchmark 1: old
Time (mean ± σ): 20.835 s ± 0.058 s [User: 14.510 s, System: 6.284 s]
Range (min … max): 20.741 s … 20.909 s 10 runs
Benchmark 2: new
Time (mean ± σ): 26.515 s ± 0.072 s [User: 19.783 s, System: 6.696 s]
Range (min … max): 26.419 s … 26.611 s 10 runs
Benchmark 3: new (small threshold)
Time (mean ± σ): 26.523 s ± 0.101 s [User: 19.805 s, System: 6.680 s]
Range (min … max): 26.416 s … 26.739 s 10 runs
Summary
'old' ran
1.27 ± 0.00 times faster than 'new'
1.27 ± 0.01 times faster than 'new (small threshold)'
(Here, 'old' is testing a compiled version of the latest 'master'
branch, while 'new' has your patches applied on top.)
Notice from this example I had a pack with many small objects (mostly
commits and trees) and I see that this change introduces significant
overhead to this case.
It would be nice to understand this overhead and fix it before taking
this change any further.
Thanks,
-Stolee
Can you show me the specific information of the repository you
tested, so that I can analyze it further.
I test this repository, but did not meet the problem:
Unpacking objects: 100% (18345/18345), 43.15 MiB
hyperfine \
--prepare 'rm -rf dest.git && git init --bare dest.git' \
-n 'old' 'git -C dest.git unpack-objects <big.pack' \
-n 'new' 'new/git -C dest.git unpack-objects <big.pack' \
-n 'new (small threshold)' 'new/git -c
core.bigfilethreshold=64k -C dest.git unpack-objects <big.pack'
Benchmark 1: old
Time (mean ± σ): 17.403 s ± 0.880 s [User: 4.996 s, System: 11.803 s]
Range (min … max): 15.911 s … 19.368 s 10 runs
Benchmark 2: new
Time (mean ± σ): 17.788 s ± 0.199 s [User: 5.054 s, System: 12.257 s]
Range (min … max): 17.420 s … 18.195 s 10 runs
Benchmark 3: new (small threshold)
Time (mean ± σ): 18.433 s ± 0.711 s [User: 4.982 s, System: 12.338 s]
Range (min … max): 17.518 s … 19.775 s 10 runs
Summary
'old' ran
1.02 ± 0.05 times faster than 'new'
1.06 ± 0.07 times faster than 'new (small threshold)'
Thanks,
- Han Xin
On Tue, Nov 30, 2021 at 1:37 AM Derrick Stolee [off-list ref] wrote:
quoted
$ hyperfine \
--prepare 'rm -rf dest.git && git init --bare dest.git' \
-n 'old' '~/_git/git-upstream/git -C dest.git unpack-objects <big.pack' \
-n 'new' '~/_git/git/git -C dest.git unpack-objects <big.pack' \
-n 'new (small threshold)' '~/_git/git/git -c core.bigfilethreshold=64k -C dest.git unpack-objects <big.pack'
Benchmark 1: old
Time (mean ± σ): 20.835 s ± 0.058 s [User: 14.510 s, System: 6.284 s]
Range (min … max): 20.741 s … 20.909 s 10 runs
Benchmark 2: new
Time (mean ± σ): 26.515 s ± 0.072 s [User: 19.783 s, System: 6.696 s]
Range (min … max): 26.419 s … 26.611 s 10 runs
Benchmark 3: new (small threshold)
Time (mean ± σ): 26.523 s ± 0.101 s [User: 19.805 s, System: 6.680 s]
Range (min … max): 26.416 s … 26.739 s 10 runs
Summary
'old' ran
1.27 ± 0.00 times faster than 'new'
1.27 ± 0.01 times faster than 'new (small threshold)'
(Here, 'old' is testing a compiled version of the latest 'master'
branch, while 'new' has your patches applied on top.)
Notice from this example I had a pack with many small objects (mostly
commits and trees) and I see that this change introduces significant
overhead to this case.
It would be nice to understand this overhead and fix it before taking
this change any further.
Thanks,
-Stolee
Can you show me the specific information of the repository you
tested, so that I can analyze it further.
I used a pack-file from an internal repo. It happened to be using
partial clone, so here is a repro with the git/git repository
after cloning this way:
$ git clone --no-checkout --filter=blob:none https://github.com/git/git
(copy the large .pack from git/.git/objects/pack/ to big.pack)
$ hyperfine \
--prepare 'rm -rf dest.git && git init --bare dest.git' \
-n 'old' '~/_git/git-upstream/git -C dest.git unpack-objects <big.pack' \
-n 'new' '~/_git/git/git -C dest.git unpack-objects <big.pack' \
-n 'new (small threshold)' '~/_git/git/git -c core.bigfilethreshold=64k -C dest.git unpack-objects <big.pack'
Benchmark 1: old
Time (mean ± σ): 82.748 s ± 0.445 s [User: 50.512 s, System: 32.049 s]
Range (min … max): 82.042 s … 83.587 s 10 runs
Benchmark 2: new
Time (mean ± σ): 101.644 s ± 0.524 s [User: 67.470 s, System: 34.047 s]
Range (min … max): 100.866 s … 102.633 s 10 runs
Benchmark 3: new (small threshold)
Time (mean ± σ): 101.093 s ± 0.269 s [User: 67.404 s, System: 33.559 s]
Range (min … max): 100.639 s … 101.375 s 10 runs
Summary
'old' ran
1.22 ± 0.01 times faster than 'new (small threshold)'
1.23 ± 0.01 times faster than 'new'
I'm also able to repro this with a smaller repo (microsoft/scalar)
so the tests complete much faster:
$ hyperfine \
--prepare 'rm -rf dest.git && git init --bare dest.git' \
-n 'old' '~/_git/git-upstream/git -C dest.git unpack-objects <small.pack' \
-n 'new' '~/_git/git/git -C dest.git unpack-objects <small.pack' \
-n 'new (small threshold)' '~/_git/git/git -c core.bigfilethreshold=64k -C dest.git unpack-objects <small.pack'
Benchmark 1: old
Time (mean ± σ): 3.295 s ± 0.023 s [User: 1.063 s, System: 2.228 s]
Range (min … max): 3.269 s … 3.351 s 10 runs
Benchmark 2: new
Time (mean ± σ): 3.592 s ± 0.105 s [User: 1.261 s, System: 2.328 s]
Range (min … max): 3.378 s … 3.679 s 10 runs
Benchmark 3: new (small threshold)
Time (mean ± σ): 3.584 s ± 0.144 s [User: 1.241 s, System: 2.339 s]
Range (min … max): 3.359 s … 3.747 s 10 runs
Summary
'old' ran
1.09 ± 0.04 times faster than 'new (small threshold)'
1.09 ± 0.03 times faster than 'new'
It's not the same relative overhead, but still significant.
These pack-files contain (mostly) small objects, no large blobs.
I know that's not the target of your efforts, but it would be
good to avoid a regression here.
Thanks,
-Stolee
I hadn't sent a shameless plug for my "git hyperfine" script to the
list, perhaps this is a good time. It's just a thin shellscript wrapper
around "hyperfine" that I wrote the other day, which...
On Tue, Nov 30 2021, Derrick Stolee wrote:
[...]
I used a pack-file from an internal repo. It happened to be using
partial clone, so here is a repro with the git/git repository
after cloning this way:
$ git clone --no-checkout --filter=blob:none https://github.com/git/git
(copy the large .pack from git/.git/objects/pack/ to big.pack)
$ hyperfine \
--prepare 'rm -rf dest.git && git init --bare dest.git' \
-n 'old' '~/_git/git-upstream/git -C dest.git unpack-objects <big.pack' \
-n 'new' '~/_git/git/git -C dest.git unpack-objects <big.pack' \
-n 'new (small threshold)' '~/_git/git/git -c core.bigfilethreshold=64k -C dest.git unpack-objects <big.pack'
Benchmark 1: old
Time (mean ± σ): 82.748 s ± 0.445 s [User: 50.512 s, System: 32.049 s]
Range (min … max): 82.042 s … 83.587 s 10 runs
Benchmark 2: new
Time (mean ± σ): 101.644 s ± 0.524 s [User: 67.470 s, System: 34.047 s]
Range (min … max): 100.866 s … 102.633 s 10 runs
Benchmark 3: new (small threshold)
Time (mean ± σ): 101.093 s ± 0.269 s [User: 67.404 s, System: 33.559 s]
Range (min … max): 100.639 s … 101.375 s 10 runs
Summary
'old' ran
1.22 ± 0.01 times faster than 'new (small threshold)'
1.23 ± 0.01 times faster than 'new'
...adds enough sugar around "hyperfine" itself to do this as e.g. (the
"-s" is a feature I submitted to hyperfine itself, it's not in a release
yet[1], but in this case you could also use "-p"):
git hyperfine -L rev v2.20.0,origin/master \
-s 'if ! test -d redis.git; then git clone --bare --filter=blob:none https://github.com/redis/redis; fi && make' \
-p 'rm -rf dest.git; git init --bare dest.git' \
'./git -C dest.git unpack-objects <$(echo redis.git/objects/pack/*.pack)'
The sugar being that for each named "rev" parameter it'll set up "git
worktree" for you, so under the hood each of those is chdir-ing to the
respective revision of:
$ git worktree list
[...]
/run/user/1001/git-hyperfine/origin/master abe6bb39053 (detached HEAD)
/run/user/1001/git-hyperfine/v2.33.0 225bc32a989 (detached HEAD)
That they're named revisions and not git-rev-parse'd is intentional,
since you'll benefit from faster incremental "make" (even if using
"ccache"). I'm typically benchmarking HEAD~1,HEAD~0.
The output will then use those "rev" parameters, and be e.g.:
Benchmark 1: ./git -C dest.git unpack-objects <$(echo redis.git/objects/pack/*.pack)' in 'v2.20.0
Time (mean ± σ): 6.678 s ± 0.046 s [User: 4.525 s, System: 2.117 s]
Range (min … max): 6.619 s … 6.765 s 10 runs
Benchmark 2: ./git -C dest.git unpack-objects <$(echo redis.git/objects/pack/*.pack)' in 'origin/master
Time (mean ± σ): 6.756 s ± 0.074 s [User: 4.586 s, System: 2.134 s]
Range (min … max): 6.691 s … 6.941 s 10 runs
Summary
'./git -C dest.git unpack-objects <$(echo redis.git/objects/pack/*.pack)' in 'v2.20.0' ran
1.01 ± 0.01 times faster than './git -C dest.git unpack-objects <$(echo redis.git/objects/pack/*.pack)' in 'origin/master'
I think if you're routinely benchmarking N different git versions you'll
find it handy, it also has configurable hook support (using git config),
so e.g. it's easy to copy your config.mak in-place in the
worktrees. E.g. my config is:
$ git -P config --get-regexp '^hyperfine'
hyperfine.run-dir $XDG_RUNTIME_DIR/git-hyperfine
hyperfine.xargs-options -r
hyperfine.hook.setup ~/g/git.meta/config.mak.sh
It's hosted at https://github.com/avar/git-hyperfine/ and
https://gitlab.com/avar/git-hyperfine/; It's implemented in (portable)
POSIX shell script.
There's surely some bugs in it, one known one is that unlike hyperfine
it doesn't accept there being spaces in the parameters to -L, because
I'm screwing up some quoting-within-quoting in the (shellscript)
implementation (suggestions for that particular one most welcome).
I hacked it up after this suggestion from Jeff King[2] of moving t/perf
over to it.
I haven't done any of that legwork, but I think a wrapper like
"git-hyperfine" that prepares worktrees for the N revisions we're
benchmarking is a good direction to go in.
We don't use git-worktrees in t/perf, but probably could for most/all
tests. In any case it would be easy to have the script setup the revs to
be benchmarked in some hookable custom manner to have it do exactly what
t/perf/run is doing now.
1. https://github.com/sharkdp/hyperfine/commit/017d55a
2. https://lore.kernel.org/git/YV+zFqi4VmBVJYex@coredump.intra.peff.net/
On Wed, Dec 1, 2021 at 2:38 AM Derrick Stolee [off-list ref] wrote:
I used a pack-file from an internal repo. It happened to be using
partial clone, so here is a repro with the git/git repository
after cloning this way:
$ git clone --no-checkout --filter=blob:none https://github.com/git/git
(copy the large .pack from git/.git/objects/pack/ to big.pack)
$ hyperfine \
--prepare 'rm -rf dest.git && git init --bare dest.git' \
-n 'old' '~/_git/git-upstream/git -C dest.git unpack-objects <big.pack' \
-n 'new' '~/_git/git/git -C dest.git unpack-objects <big.pack' \
-n 'new (small threshold)' '~/_git/git/git -c core.bigfilethreshold=64k -C dest.git unpack-objects <big.pack'
Benchmark 1: old
Time (mean ± σ): 82.748 s ± 0.445 s [User: 50.512 s, System: 32.049 s]
Range (min … max): 82.042 s … 83.587 s 10 runs
Benchmark 2: new
Time (mean ± σ): 101.644 s ± 0.524 s [User: 67.470 s, System: 34.047 s]
Range (min … max): 100.866 s … 102.633 s 10 runs
Benchmark 3: new (small threshold)
Time (mean ± σ): 101.093 s ± 0.269 s [User: 67.404 s, System: 33.559 s]
Range (min … max): 100.639 s … 101.375 s 10 runs
Summary
'old' ran
1.22 ± 0.01 times faster than 'new (small threshold)'
1.23 ± 0.01 times faster than 'new'
I'm also able to repro this with a smaller repo (microsoft/scalar)
so the tests complete much faster:
$ hyperfine \
--prepare 'rm -rf dest.git && git init --bare dest.git' \
-n 'old' '~/_git/git-upstream/git -C dest.git unpack-objects <small.pack' \
-n 'new' '~/_git/git/git -C dest.git unpack-objects <small.pack' \
-n 'new (small threshold)' '~/_git/git/git -c core.bigfilethreshold=64k -C dest.git unpack-objects <small.pack'
Benchmark 1: old
Time (mean ± σ): 3.295 s ± 0.023 s [User: 1.063 s, System: 2.228 s]
Range (min … max): 3.269 s … 3.351 s 10 runs
Benchmark 2: new
Time (mean ± σ): 3.592 s ± 0.105 s [User: 1.261 s, System: 2.328 s]
Range (min … max): 3.378 s … 3.679 s 10 runs
Benchmark 3: new (small threshold)
Time (mean ± σ): 3.584 s ± 0.144 s [User: 1.241 s, System: 2.339 s]
Range (min … max): 3.359 s … 3.747 s 10 runs
Summary
'old' ran
1.09 ± 0.04 times faster than 'new (small threshold)'
1.09 ± 0.03 times faster than 'new'
It's not the same relative overhead, but still significant.
These pack-files contain (mostly) small objects, no large blobs.
I know that's not the target of your efforts, but it would be
good to avoid a regression here.
Thanks,
-Stolee
With your help, I did catch this performance problem, which was
introduced in this patch:
https://lore.kernel.org/git/20211122033220.32883-4-chiyutianyi@gmail.com/
This patch changes the original data reading ino to stream reading, but
its problem is that even for the original reading of the whole object data,
it still generates an additional git_deflate() and subsequent transfer.
I will fix it in a follow-up patch.
Thanks,
-Han Xin
On Wed, Dec 1, 2021 at 2:38 AM Derrick Stolee [off-list ref] wrote:
quoted
These pack-files contain (mostly) small objects, no large blobs.
I know that's not the target of your efforts, but it would be
good to avoid a regression here.
Thanks,
-Stolee
With your help, I did catch this performance problem, which was
introduced in this patch:
https://lore.kernel.org/git/20211122033220.32883-4-chiyutianyi@gmail.com/
This patch changes the original data reading ino to stream reading, but
its problem is that even for the original reading of the whole object data,
it still generates an additional git_deflate() and subsequent transfer.
From: Han Xin <redacted>
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.
This can be improved by feeding data to "write_loose_object()" in a
stream. The input stream is implemented as an interface. In the first
step, we make a simple implementation, feeding the entire buffer in the
"stream" to "write_loose_object()" as a refactor.
Helped-by: Jiang Xin [off-list ref]
Signed-off-by: Han Xin <redacted>
---
object-file.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++----
object-store.h | 6 ++++++
2 files changed, 55 insertions(+), 4 deletions(-)
@@ -1898,6 +1918,7 @@ static int write_loose_object(const struct object_id *oid, char *hdr,the_hash_algo->update_fn(&c,hdr,hdrlen);/* Then the data itself.. */+buf=in_stream->read(in_stream,&len);stream.next_in=(void*)buf;stream.avail_in=len;do{
@@ -1960,6 +1981,14 @@ int write_object_file_flags(const void *buf, unsigned long len,{charhdr[MAX_HEADER_LEN];inthdrlen=sizeof(hdr);+structinput_streamin_stream={+.read=feed_simple_input_stream,+.data=(void*)&(structsimple_input_stream_data){+.buf=buf,+.len=len,+},+.size=len,+};/* Normally if we have it in the pack then we do not bother writing*itoutinto.git/objects/??/?{38}file.
@@ -1968,7 +1997,7 @@ int write_object_file_flags(const void *buf, unsigned long len,&hdrlen);if(freshen_packed_object(oid)||freshen_loose_object(oid))return0;-returnwrite_loose_object(oid,hdr,hdrlen,buf,len,0,flags);+returnwrite_loose_object(oid,hdr,hdrlen,&in_stream,0,flags);}inthash_object_file_literally(constvoid*buf,unsignedlonglen,
@@ -1977,6 +2006,14 @@ int hash_object_file_literally(const void *buf, unsigned long len,{char*header;inthdrlen,status=0;+structinput_streamin_stream={+.read=feed_simple_input_stream,+.data=(void*)&(structsimple_input_stream_data){+.buf=buf,+.len=len,+},+.size=len,+};/* type string, SP, %lu of the length plus NUL must fit this */hdrlen=strlen(type)+MAX_HEADER_LEN;
@@ -1988,7 +2025,7 @@ int hash_object_file_literally(const void *buf, unsigned long len,gotocleanup;if(freshen_packed_object(oid)||freshen_loose_object(oid))gotocleanup;-status=write_loose_object(oid,header,hdrlen,buf,len,0,0);+status=write_loose_object(oid,header,hdrlen,&in_stream,0,0);cleanup:free(header);
@@ -2003,14 +2040,22 @@ int force_object_loose(const struct object_id *oid, time_t mtime)charhdr[MAX_HEADER_LEN];inthdrlen;intret;+structsimple_input_stream_datadata;+structinput_streamin_stream={+.read=feed_simple_input_stream,+.data=&data,+};if(has_loose_object(oid))return0;buf=read_object(the_repository,oid,&type,&len);+in_stream.size=len;if(!buf)returnerror(_("cannot read object for %s"),oid_to_hex(oid));+data.buf=buf;+data.len=len;hdrlen=xsnprintf(hdr,sizeof(hdr),"%s %"PRIuMAX,type_name(type),(uintmax_t)len)+1;-ret=write_loose_object(oid,hdr,hdrlen,buf,len,mtime,0);+ret=write_loose_object(oid,hdr,hdrlen,&in_stream,mtime,0);free(buf);returnret;
From: Han Xin <redacted>
When streaming a large blob object to "write_loose_object()", we have no
chance to run "write_object_file_prepare()" to calculate the oid in
advance. So we need to handle undetermined oid in function
"write_loose_object()".
In the original implementation, 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.
Helped-by: Jiang Xin [off-list ref]
Signed-off-by: Han Xin <redacted>
---
object-file.c | 30 ++++++++++++++++++++++++++++--
1 file changed, 28 insertions(+), 2 deletions(-)
@@ -1892,7 +1892,14 @@ static int write_loose_object(const struct object_id *oid, char *hdr,constvoid*buf;unsignedlonglen;-loose_object_path(the_repository,&filename,oid);+if(is_null_oid(oid)){+/* When oid is not determined, save tmp file to odb path. */+strbuf_reset(&filename);+strbuf_addstr(&filename,the_repository->objects->odb->path);+strbuf_addch(&filename,'/');+}else{+loose_object_path(the_repository,&filename,oid);+}fd=create_tmpfile(&tmp_file,filename.buf);if(fd<0){
@@ -1939,12 +1946,31 @@ static int write_loose_object(const struct object_id *oid, char *hdr,die(_("deflateEnd on object %s failed (%d)"),oid_to_hex(oid),ret);the_hash_algo->final_oid_fn(¶no_oid,&c);-if(!oideq(oid,¶no_oid))+if(!is_null_oid(oid)&&!oideq(oid,¶no_oid))die(_("confused by unstable object source data for %s"),oid_to_hex(oid));close_loose_object(fd);+if(is_null_oid(oid)){+intdirlen;++oidcpy((structobject_id*)oid,¶no_oid);+loose_object_path(the_repository,&filename,oid);++/* We finally know the object path, and create the missing dir. */+dirlen=directory_size(filename.buf);+if(dirlen){+structstrbufdir=STRBUF_INIT;+strbuf_add(&dir,filename.buf,dirlen-1);+if(mkdir(dir.buf,0777)&&errno!=EEXIST)+return-1;+if(adjust_shared_perm(dir.buf))+return-1;+strbuf_release(&dir);+}+}+if(mtime){structutimbufutb;utb.actime=mtime;
From: Han Xin <redacted>
In order to prepare the stream version of "write_loose_object()", read
the input stream in a loop in "write_loose_object()", so that we can
feed the contents of large blob object to "write_loose_object()" using
a small fixed buffer.
Helped-by: Jiang Xin [off-list ref]
Signed-off-by: Han Xin <redacted>
---
object-file.c | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)
@@ -1890,7 +1890,7 @@ static int write_loose_object(const struct object_id *oid, char *hdr,staticstructstrbuftmp_file=STRBUF_INIT;staticstructstrbuffilename=STRBUF_INIT;constvoid*buf;-unsignedlonglen;+intflush=0;if(is_null_oid(oid)){/* When oid is not determined, save tmp file to odb path. */
@@ -1925,18 +1925,23 @@ static int write_loose_object(const struct object_id *oid, char *hdr,the_hash_algo->update_fn(&c,hdr,hdrlen);/* Then the data itself.. */-buf=in_stream->read(in_stream,&len);-stream.next_in=(void*)buf;-stream.avail_in=len;do{unsignedchar*in0=stream.next_in;-ret=git_deflate(&stream,Z_FINISH);+if(!stream.avail_in){+buf=in_stream->read(in_stream,&stream.avail_in);+stream.next_in=(void*)buf;+in0=(unsignedchar*)buf;+/* All data has been read. */+if(in_stream->size+hdrlen==stream.total_in+stream.avail_in)+flush=Z_FINISH;+}+ret=git_deflate(&stream,flush);the_hash_algo->update_fn(&c,in0,stream.next_in-in0);if(write_buffer(fd,compressed,stream.next_out-compressed)<0)die(_("unable to write loose object file"));stream.next_out=compressed;stream.avail_out=sizeof(compressed);-}while(ret==Z_OK);+}while(ret==Z_OK||ret==Z_BUF_ERROR);if(ret!=Z_STREAM_END)die(_("unable to deflate new object %s (%d)"),oid_to_hex(oid),
From: Han Xin <redacted>
In dry_run mode, "get_data()" is used to verify the inflation of data,
and the returned buffer will not be used at all and will be freed
immediately. Even in dry_run mode, it is dangerous to allocate a
full-size buffer for a large blob object. Therefore, only allocate a
low memory footprint when calling "get_data()" in dry_run mode.
Suggested-by: Jiang Xin <redacted>
Signed-off-by: Han Xin <redacted>
---
builtin/unpack-objects.c | 18 ++++++++++++------
1 file changed, 12 insertions(+), 6 deletions(-)
@@ -396,7 +402,7 @@ static void unpack_delta_entry(enum object_type type, unsigned long delta_size,if(base_offset<=0||base_offset>=obj_list[nr].offset)die("offset value out of bound for delta base object");-delta_data=get_data(delta_size);+delta_data=get_data(delta_size,dry_run);if(dry_run||!delta_data){free(delta_data);return;
From: Han Xin <redacted>
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.
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. Therefore, only unpack object
larger than the "big_file_threshold" in zstream. See the following
benchmarks:
hyperfine \
--setup \
'if ! test -d scalar.git; then git clone --bare https://github.com/microsoft/scalar.git; cp scalar.git/objects/pack/*.pack small.pack; fi' \
--prepare 'rm -rf dest.git && git init --bare dest.git' \
-n 'old' 'git -C dest.git unpack-objects <small.pack' \
-n 'new' 'new/git -C dest.git unpack-objects <small.pack' \
-n 'new (small threshold)' \
'new/git -c core.bigfilethreshold=16k -C dest.git unpack-objects <small.pack'
Benchmark 1: old
Time (mean ± σ): 6.075 s ± 0.069 s [User: 5.047 s, System: 0.991 s]
Range (min … max): 6.018 s … 6.189 s 10 runs
Benchmark 2: new
Time (mean ± σ): 6.090 s ± 0.033 s [User: 5.075 s, System: 0.976 s]
Range (min … max): 6.030 s … 6.142 s 10 runs
Benchmark 3: new (small threshold)
Time (mean ± σ): 6.755 s ± 0.029 s [User: 5.150 s, System: 1.560 s]
Range (min … max): 6.711 s … 6.809 s 10 runs
Summary
'old' ran
1.00 ± 0.01 times faster than 'new'
1.11 ± 0.01 times faster than 'new (small threshold)'
Helped-by: Derrick Stolee [off-list ref]
Helped-by: Jiang Xin [off-list ref]
Signed-off-by: Han Xin <redacted>
---
builtin/unpack-objects.c | 77 ++++++++++++++++++++++++++++-
object-file.c | 6 +--
object-store.h | 4 ++
t/t5590-unpack-non-delta-objects.sh | 76 ++++++++++++++++++++++++++++
4 files changed, 159 insertions(+), 4 deletions(-)
create mode 100755 t/t5590-unpack-non-delta-objects.sh
@@ -326,11 +326,86 @@ static void added_object(unsigned nr, enum object_type type,}}+structinput_zstream_data{+git_zstream*zstream;+unsignedcharbuf[8192];+intstatus;+};++staticconstvoid*feed_input_zstream(structinput_stream*in_stream,unsignedlong*readlen)+{+structinput_zstream_data*data=in_stream->data;+git_zstream*zstream=data->zstream;+void*in=fill(1);++if(!len||data->status==Z_STREAM_END){+*readlen=0;+returnNULL;+}++zstream->next_out=data->buf;+zstream->avail_out=sizeof(data->buf);+zstream->next_in=in;+zstream->avail_in=len;++data->status=git_inflate(zstream,0);+use(len-zstream->avail_in);+*readlen=sizeof(data->buf)-zstream->avail_out;++returndata->buf;+}++staticvoidwrite_stream_blob(unsignednr,unsignedlongsize)+{+charhdr[32];+inthdrlen;+git_zstreamzstream;+structinput_zstream_datadata;+structinput_streamin_stream={+.read=feed_input_zstream,+.data=&data,+.size=size,+};+structobject_id*oid=&obj_list[nr].oid;+intret;++memset(&zstream,0,sizeof(zstream));+memset(&data,0,sizeof(data));+data.zstream=&zstream;+git_inflate_init(&zstream);++/* Generate the header */+hdrlen=xsnprintf(hdr,sizeof(hdr),"%s %"PRIuMAX,type_name(OBJ_BLOB),(uintmax_t)size)+1;++if((ret=write_loose_object(oid,hdr,hdrlen,&in_stream,0,0)))+die(_("failed to write object in stream %d"),ret);++if(zstream.total_out!=size||data.status!=Z_STREAM_END)+die(_("inflate returned %d"),data.status);+git_inflate_end(&zstream);++if(strict&&!dry_run){+structblob*blob=lookup_blob(the_repository,oid);+if(blob)+blob->object.flags|=FLAG_WRITTEN;+else+die("invalid blob object from stream");+}+obj_list[nr].obj=NULL;+}+staticvoidunpack_non_delta_entry(enumobject_typetype,unsignedlongsize,unsignednr){-void*buf=get_data(size,dry_run);+void*buf;++/* Write large blob in stream without allocating full buffer. */+if(!dry_run&&type==OBJ_BLOB&&size>big_file_threshold){+write_stream_blob(nr,size);+return;+}+buf=get_data(size,dry_run);if(!dry_run&&buf)write_object(nr,type,buf,size);else
@@ -0,0 +1,76 @@+#!/bin/sh+#+# Copyright (c) 2021 Han Xin+#++test_description='Test unpack-objects when receive pack'++GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main+exportGIT_TEST_DEFAULT_INITIAL_BRANCH_NAME++../test-lib.sh++test_expect_success"create commit with big blobs (1.5 MB)"'+test-toolgenrandomfoo1500000>big-blob&&+test_commit--appendfoobig-blob&&+test-toolgenrandombar1500000>big-blob&&+test_commit--appendbarbig-blob&&+(+cd.git&&+findobjects/??-typef|sort+)>expect&&+PACK=$(echomain|gitpack-objects--progress--revstest)+'++test_expect_success'setup GIT_ALLOC_LIMIT to 1MB''+GIT_ALLOC_LIMIT=1m&&+exportGIT_ALLOC_LIMIT+'++test_expect_success'prepare dest repository''+gitinit--baredest.git&&+git-Cdest.gitconfigcore.bigFileThreshold2m&&+git-Cdest.gitconfigreceive.unpacklimit100+'++test_expect_success'fail to unpack-objects: cannot allocate''+test_must_failgit-Cdest.gitunpack-objects<test-$PACK.pack2>err&&+test_i18ngrep"fatal: attempting to allocate"err&&+(+cddest.git&&+findobjects/??-typef|sort+)>actual&&+!test_cmpexpectactual+'++test_expect_success'set a lower bigfile threshold''+git-Cdest.gitconfigcore.bigFileThreshold1m+'++test_expect_success'unpack big object in stream''+git-Cdest.gitunpack-objects<test-$PACK.pack&&+git-Cdest.gitfsck&&+(+cddest.git&&+findobjects/??-typef|sort+)>actual&&+test_cmpexpectactual+'++test_expect_success'setup for unpack-objects dry-run test''+gitinit--bareunpack-test.git+'++test_expect_success'unpack-objects dry-run''+(+cdunpack-test.git&&+gitunpack-objects-n<../test-$PACK.pack+)&&+(+cdunpack-test.git&&+findobjects/-typef+)>actual&&+test_must_be_emptyactual+'++test_done
From: Han Xin <redacted>
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.
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. Therefore, only unpack object
larger than the "big_file_threshold" in zstream. See the following
benchmarks:
hyperfine \
--setup \
'if ! test -d scalar.git; then git clone --bare https://github.com/microsoft/scalar.git; cp scalar.git/objects/pack/*.pack small.pack; fi' \
--prepare 'rm -rf dest.git && git init --bare dest.git' \
-n 'old' 'git -C dest.git unpack-objects <small.pack' \
-n 'new' 'new/git -C dest.git unpack-objects <small.pack' \
-n 'new (small threshold)' \
'new/git -c core.bigfilethreshold=16k -C dest.git unpack-objects <small.pack'
Benchmark 1: old
Time (mean ± σ): 6.075 s ± 0.069 s [User: 5.047 s, System: 0.991 s]
Range (min … max): 6.018 s … 6.189 s 10 runs
Benchmark 2: new
Time (mean ± σ): 6.090 s ± 0.033 s [User: 5.075 s, System: 0.976 s]
Range (min … max): 6.030 s … 6.142 s 10 runs
Benchmark 3: new (small threshold)
Time (mean ± σ): 6.755 s ± 0.029 s [User: 5.150 s, System: 1.560 s]
Range (min … max): 6.711 s … 6.809 s 10 runs
Summary
'old' ran
1.00 ± 0.01 times faster than 'new'
1.11 ± 0.01 times faster than 'new (small threshold)'
So before we wrote used core.bigfilethreshold for two things (or more?):
Whether we show a diff for it (we mark it "binary") and whether it's
split into a loose object.
Now it's three things, we've added a "this is a threshold when we'll
stream the object" to that.
Might it make sense to squash something like this in, so we can have our
cake & eat it too?
With this I get, where HEAD~0 is this change:
Summary
'./git -C dest.git -c core.bigfilethreshold=512m unpack-objects <small.pack' in 'HEAD~0' ran
1.00 ± 0.01 times faster than './git -C dest.git -c core.bigfilethreshold=512m unpack-objects <small.pack' in 'HEAD~1'
1.00 ± 0.01 times faster than './git -C dest.git -c core.bigfilethreshold=512m unpack-objects <small.pack' in 'origin/master'
1.01 ± 0.01 times faster than './git -C dest.git -c core.bigfilethreshold=16k unpack-objects <small.pack' in 'HEAD~0'
1.06 ± 0.14 times faster than './git -C dest.git -c core.bigfilethreshold=16k unpack-objects <small.pack' in 'origin/master'
1.20 ± 0.01 times faster than './git -C dest.git -c core.bigfilethreshold=16k unpack-objects <small.pack' in 'HEAD~1'
I.e. it's 5% slower, not 20% (haven't looked into why), but we'll not
stream out 16k..128MB objects (maybe the repo has even bigger ones?)
@@ -424,6 +424,17 @@ be delta compressed, but larger binary media files won't be. + Common unit suffixes of 'k', 'm', or 'g' are supported.+core.bigFileStreamingThreshold::+ Files larger than this will be streamed out to a temporary+ object file while being hashed, which will when be renamed+ in-place to a loose object, particularly if the+ `core.bigFileThreshold' setting dictates that they're always+ written out as loose objects.+++Default is 128 MiB on all platforms.+++Common unit suffixes of 'k', 'm', or 'g' are supported.+ core.excludesFile:: Specifies the pathname to the file that contains patterns to describe paths that are not meant to be tracked, in addition
@@ -400,7 +400,7 @@ static void unpack_non_delta_entry(enum object_type type, unsigned long size,void*buf;/* Write large blob in stream without allocating full buffer. */-if(!dry_run&&type==OBJ_BLOB&&size>big_file_threshold){+if(!dry_run&&type==OBJ_BLOB&&size>big_file_streaming_threshold){write_stream_blob(nr,size);return;}
From: Han Xin <redacted>
When streaming a large blob object to "write_loose_object()", we have no
chance to run "write_object_file_prepare()" to calculate the oid in
advance. So we need to handle undetermined oid in function
"write_loose_object()".
In the original implementation, 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.
Helped-by: Jiang Xin [off-list ref]
Signed-off-by: Han Xin <redacted>
---
object-file.c | 30 ++++++++++++++++++++++++++++--
1 file changed, 28 insertions(+), 2 deletions(-)
@@ -1892,7 +1892,14 @@ static int write_loose_object(const struct object_id *oid, char *hdr,constvoid*buf;unsignedlonglen;-loose_object_path(the_repository,&filename,oid);+if(is_null_oid(oid)){+/* When oid is not determined, save tmp file to odb path. */+strbuf_reset(&filename);+strbuf_addstr(&filename,the_repository->objects->odb->path);+strbuf_addch(&filename,'/');+}else{+loose_object_path(the_repository,&filename,oid);+}fd=create_tmpfile(&tmp_file,filename.buf);if(fd<0){
@@ -1939,12 +1946,31 @@ static int write_loose_object(const struct object_id *oid, char *hdr,die(_("deflateEnd on object %s failed (%d)"),oid_to_hex(oid),ret);the_hash_algo->final_oid_fn(¶no_oid,&c);-if(!oideq(oid,¶no_oid))+if(!is_null_oid(oid)&&!oideq(oid,¶no_oid))die(_("confused by unstable object source data for %s"),oid_to_hex(oid));close_loose_object(fd);+if(is_null_oid(oid)){+intdirlen;++oidcpy((structobject_id*)oid,¶no_oid);+loose_object_path(the_repository,&filename,oid);
Why are we breaking the promise that "oid" is constant here? I tested
locally with the below on top, and it seems to work (at least no tests
broke). Isn't it preferrable to the cast & the caller having its "oid"
changed?
@@ -1958,10 +1958,11 @@ int write_loose_object(const struct object_id *oid, char *hdr,close_loose_object(fd);if(is_null_oid(oid)){+structobject_idoid2;intdirlen;-oidcpy((structobject_id*)oid,¶no_oid);-loose_object_path(the_repository,&filename,oid);+oidcpy(&oid2,¶no_oid);+loose_object_path(the_repository,&filename,&oid2);/* We finally know the object path, and create the missing dir. */dirlen=directory_size(filename.buf);
From: Han Xin <redacted>
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.
This can be improved by feeding data to "write_loose_object()" in a
stream. The input stream is implemented as an interface. In the first
step, we make a simple implementation, feeding the entire buffer in the
"stream" to "write_loose_object()" as a refactor.
Helped-by: Jiang Xin [off-list ref]
Signed-off-by: Han Xin <redacted>
---
object-file.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++----
object-store.h | 6 ++++++
2 files changed, 55 insertions(+), 4 deletions(-)
I see why you picked "const void *buf" here, over say const char *, it's
what "struct input_stream" uses.
But why not use size_t for the length, as input_stream does?
But isn't the body of this functin the same as:
*len = data->len;
if (!len)
return NULL;
data->len = 0;
return data->buf;
I.e. you don't need the condition for setting "*len" if it's 0, then
data->len is also 0. You just want to return NULL afterwards, and not
set (harmless, but no need) data->len to 0)< or return data->buf.
Maybe it's that I'm unused to it, but I find this a bit more readable:
@@ -2013,12 +2011,13 @@ int write_object_file_flags(const void *buf, unsigned long len,
{
char hdr[MAX_HEADER_LEN];
int hdrlen = sizeof(hdr);
+ struct simple_input_stream_data tmp = {
+ .buf = buf,
+ .len = len,
+ };
struct input_stream in_stream = {
.read = feed_simple_input_stream,
- .data = (void *)&(struct simple_input_stream_data) {
- .buf = buf,
- .len = len,
- },
+ .data = (void *)&tmp,
.size = len,
};
Yes there's a temporary variable, but no denser inline casting. Also
easier to strep through in a debugger (which will have the type
information on "tmp".
quoted hunk
int hash_object_file_literally(const void *buf, unsigned long len,
From: Han Xin <redacted>
When streaming a large blob object to "write_loose_object()", we have no
chance to run "write_object_file_prepare()" to calculate the oid in
advance. So we need to handle undetermined oid in function
"write_loose_object()".
In the original implementation, 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.
Helped-by: Jiang Xin [off-list ref]
Signed-off-by: Han Xin <redacted>
---
object-file.c | 30 ++++++++++++++++++++++++++++--
1 file changed, 28 insertions(+), 2 deletions(-)
@@ -1892,7 +1892,14 @@ static int write_loose_object(const struct object_id *oid, char *hdr,constvoid*buf;unsignedlonglen;-loose_object_path(the_repository,&filename,oid);+if(is_null_oid(oid)){+/* When oid is not determined, save tmp file to odb path. */+strbuf_reset(&filename);
Why re-use this & leak memory? An existing strbuf use in this function
doesn't leak in the same way. Just release it as in the below patch on
top (the ret v.s. err variable naming is a bit confused, maybe could do
with a prep cleanup step.).
And once we do that this could just become:
strbuf_addf($filename, "%s/", ...)
Is there's existing uses of this pattern, so mayb e not worth it, but it
allows you to remove the braces on the if/else.
@@ -1892,7 +1892,6 @@ int write_loose_object(const struct object_id *oid, char *hdr,if(is_null_oid(oid)){/* When oid is not determined, save tmp file to odb path. */-strbuf_reset(&filename);strbuf_addstr(&filename,the_repository->objects->odb->path);strbuf_addch(&filename,'/');}else{
@@ -1902,11 +1901,12 @@ int write_loose_object(const struct object_id *oid, char *hdr,fd=create_tmpfile(&tmp_file,filename.buf);if(fd<0){if(flags&HASH_SILENT)-return-1;+err=-1;elseif(errno==EACCES)-returnerror(_("insufficient permission for adding an object to repository database %s"),get_object_directory());+err=error(_("insufficient permission for adding an object to repository database %s"),get_object_directory());else-returnerror_errno(_("unable to create temporary file"));+err=error_errno(_("unable to create temporary file"));+gotocleanup;}/* Set it up */
@@ -0,0 +1,76 @@+#!/bin/sh+#+# Copyright (c) 2021 Han Xin+#++test_description='Test unpack-objects when receive pack'++GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main+exportGIT_TEST_DEFAULT_INITIAL_BRANCH_NAME++../test-lib.sh++test_expect_success"create commit with big blobs (1.5 MB)"'+test-toolgenrandomfoo1500000>big-blob&&+test_commit--appendfoobig-blob&&+test-toolgenrandombar1500000>big-blob&&+test_commit--appendbarbig-blob&&+(+cd.git&&+findobjects/??-typef|sort
I think it would be better to just (could roll this into a function):
test_when_finished "rm -rf dest.git" &&
git init dest.git &&
git -C dest.git config ...
Then you can use it with e.g. --run=3-4 and not have it error out
because of skipped setup.
A lot of our tests fail like that, but in this case fixing it seems
trivial.
It's probably nothing, but in your CL you note that you changed another
hardcoding from 4k to 8k, should this one still be 4k?
It's probably fine, just wondering...
Just a side-note, I think (but am not 100% sure) that these existing
occurances aren't needed due to our use of CALLOC_ARRAY():
diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c
index 4a9466295ba..00b349412c5 100644
--- a/builtin/unpack-objects.c
+++ b/builtin/unpack-objects.c
@@ -248,7 +248,6 @@ static void write_object(unsigned nr, enum object_type type,
die("failed to write object");
added_object(nr, type, buf, size);
free(buf);
- obj_list[nr].obj = NULL;
} else if (type == OBJ_BLOB) {
struct blob *blob;
if (write_object_file(buf, size, type_name(type),
@@ -262,7 +261,6 @@ static void write_object(unsigned nr, enum object_type type,
blob->object.flags |= FLAG_WRITTEN;
else
die("invalid blob object");
- obj_list[nr].obj = NULL;
} else {
struct object *obj;
int eaten;
The reason I'm noting it is that the same seems to be true of your new
addition here. I.e. are these assignments to NULL needed?
Anyway, the reason I started poking at this it tha this
write_stream_blob() seems to duplicate much of write_object(). AFAICT
only the writing part is really different, the part where we
lookup_blob() after, set FLAG_WRITTEN etc. is all the same.
Why can't we call write_object() here?
The obvious answer seems to be that the call to write_object_file()
isn't prepared to do the sort of streaming that you want, so instead
you're bypassing it and calling write_loose_object() directly.
I haven't tried this myself, but isn't a better and cleaner approach
here to not add another meaning to what is_null_oid() means, but to just
add a HASH_STREAM flag that'll get passed down as "unsigned flags" to
write_loose_object()? See FLAG_BITS in object.h.
Then the "obj_list[nr].obj" here could also become
"obj_list[nr].obj.flags |= (1u<<12)" or whatever (but that wouldn't
strictly be needed I think.
But by adding the "HASH_STREAM" flag you could I think stop duplicating
the "Generate the header" etc. here and call write_object_file_flags().
I don't so much care about how it's done within unpack-objects.c, but
not having another meaning to is_null_oid() in play would be really
nice, and it this case it seems entirely avoidable.
On Fri, Dec 3, 2021 at 9:41 PM Ævar Arnfjörð Bjarmason [off-list ref] wrote:
On Fri, Dec 03 2021, Han Xin wrote:
quoted
From: Han Xin <redacted>
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.
This can be improved by feeding data to "write_loose_object()" in a
stream. The input stream is implemented as an interface. In the first
step, we make a simple implementation, feeding the entire buffer in the
"stream" to "write_loose_object()" as a refactor.
Helped-by: Jiang Xin [off-list ref]
Signed-off-by: Han Xin <redacted>
---
object-file.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++----
object-store.h | 6 ++++++
2 files changed, 55 insertions(+), 4 deletions(-)
I see why you picked "const void *buf" here, over say const char *, it's
what "struct input_stream" uses.
But why not use size_t for the length, as input_stream does?
But isn't the body of this functin the same as:
*len = data->len;
if (!len)
return NULL;
data->len = 0;
return data->buf;
I.e. you don't need the condition for setting "*len" if it's 0, then
data->len is also 0. You just want to return NULL afterwards, and not
set (harmless, but no need) data->len to 0)< or return data->buf.
Maybe it's that I'm unused to it, but I find this a bit more readable:
@@ -2013,12 +2011,13 @@ int write_object_file_flags(const void *buf, unsigned long len,
{
char hdr[MAX_HEADER_LEN];
int hdrlen = sizeof(hdr);
+ struct simple_input_stream_data tmp = {
+ .buf = buf,
+ .len = len,
+ };
struct input_stream in_stream = {
.read = feed_simple_input_stream,
- .data = (void *)&(struct simple_input_stream_data) {
- .buf = buf,
- .len = len,
- },
+ .data = (void *)&tmp,
.size = len,
};
Yes there's a temporary variable, but no denser inline casting. Also
easier to strep through in a debugger (which will have the type
information on "tmp".
Will apply.
quoted
int hash_object_file_literally(const void *buf, unsigned long len,
On Fri, Dec 3, 2021 at 9:27 PM Ævar Arnfjörð Bjarmason [off-list ref] wrote:
quoted hunk
On Fri, Dec 03 2021, Han Xin wrote:
quoted
From: Han Xin <redacted>
When streaming a large blob object to "write_loose_object()", we have no
chance to run "write_object_file_prepare()" to calculate the oid in
advance. So we need to handle undetermined oid in function
"write_loose_object()".
In the original implementation, 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.
Helped-by: Jiang Xin [off-list ref]
Signed-off-by: Han Xin <redacted>
---
object-file.c | 30 ++++++++++++++++++++++++++++--
1 file changed, 28 insertions(+), 2 deletions(-)
@@ -1892,7 +1892,14 @@ static int write_loose_object(const struct object_id *oid, char *hdr,constvoid*buf;unsignedlonglen;-loose_object_path(the_repository,&filename,oid);+if(is_null_oid(oid)){+/* When oid is not determined, save tmp file to odb path. */+strbuf_reset(&filename);+strbuf_addstr(&filename,the_repository->objects->odb->path);+strbuf_addch(&filename,'/');+}else{+loose_object_path(the_repository,&filename,oid);+}fd=create_tmpfile(&tmp_file,filename.buf);if(fd<0){
@@ -1939,12 +1946,31 @@ static int write_loose_object(const struct object_id *oid, char *hdr,die(_("deflateEnd on object %s failed (%d)"),oid_to_hex(oid),ret);the_hash_algo->final_oid_fn(¶no_oid,&c);-if(!oideq(oid,¶no_oid))+if(!is_null_oid(oid)&&!oideq(oid,¶no_oid))die(_("confused by unstable object source data for %s"),oid_to_hex(oid));close_loose_object(fd);+if(is_null_oid(oid)){+intdirlen;++oidcpy((structobject_id*)oid,¶no_oid);+loose_object_path(the_repository,&filename,oid);
Why are we breaking the promise that "oid" is constant here? I tested
locally with the below on top, and it seems to work (at least no tests
broke). Isn't it preferrable to the cast & the caller having its "oid"
changed?
@@ -1958,10 +1958,11 @@ int write_loose_object(const struct object_id *oid, char *hdr,close_loose_object(fd);if(is_null_oid(oid)){+structobject_idoid2;intdirlen;-oidcpy((structobject_id*)oid,¶no_oid);-loose_object_path(the_repository,&filename,oid);+oidcpy(&oid2,¶no_oid);+loose_object_path(the_repository,&filename,&oid2);/* We finally know the object path, and create the missing dir. */dirlen=directory_size(filename.buf);
Maybe I should change the promise that "oid" is constant in
"write_loose_object()".
The original write_object_file_flags() defines a variable "oid", and
completes the calculation of the "oid" in
"write_object_file_prepare()" which will be passed to
"write_loose_object()".
If a null oid is maintained after calling "write_loose_object()",
"--strict" will become meaningless, although it does not break existing
test cases.
On Fri, Dec 3, 2021 at 9:54 PM Ævar Arnfjörð Bjarmason [off-list ref] wrote:
quoted hunk
On Fri, Dec 03 2021, Han Xin wrote:
quoted
From: Han Xin <redacted>
When streaming a large blob object to "write_loose_object()", we have no
chance to run "write_object_file_prepare()" to calculate the oid in
advance. So we need to handle undetermined oid in function
"write_loose_object()".
In the original implementation, 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.
Helped-by: Jiang Xin [off-list ref]
Signed-off-by: Han Xin <redacted>
---
object-file.c | 30 ++++++++++++++++++++++++++++--
1 file changed, 28 insertions(+), 2 deletions(-)
@@ -1892,7 +1892,14 @@ static int write_loose_object(const struct object_id *oid, char *hdr,constvoid*buf;unsignedlonglen;-loose_object_path(the_repository,&filename,oid);+if(is_null_oid(oid)){+/* When oid is not determined, save tmp file to odb path. */+strbuf_reset(&filename);
Why re-use this & leak memory? An existing strbuf use in this function
doesn't leak in the same way. Just release it as in the below patch on
top (the ret v.s. err variable naming is a bit confused, maybe could do
with a prep cleanup step.).
And once we do that this could just become:
strbuf_addf($filename, "%s/", ...)
Is there's existing uses of this pattern, so mayb e not worth it, but it
allows you to remove the braces on the if/else.
@@ -1892,7 +1892,6 @@ int write_loose_object(const struct object_id *oid, char *hdr,if(is_null_oid(oid)){/* When oid is not determined, save tmp file to odb path. */-strbuf_reset(&filename);strbuf_addstr(&filename,the_repository->objects->odb->path);strbuf_addch(&filename,'/');}else{
@@ -1902,11 +1901,12 @@ int write_loose_object(const struct object_id *oid, char *hdr,fd=create_tmpfile(&tmp_file,filename.buf);if(fd<0){if(flags&HASH_SILENT)-return-1;+err=-1;elseif(errno==EACCES)-returnerror(_("insufficient permission for adding an object to repository database %s"),get_object_directory());+err=error(_("insufficient permission for adding an object to repository database %s"),get_object_directory());else-returnerror_errno(_("unable to create temporary file"));+err=error_errno(_("unable to create temporary file"));+gotocleanup;}/* Set it up */
It's probably nothing, but in your CL you note that you changed another
hardcoding from 4k to 8k, should this one still be 4k?
It's probably fine, just wondering...
@@ -0,0 +1,76 @@+#!/bin/sh+#+# Copyright (c) 2021 Han Xin+#++test_description='Test unpack-objects when receive pack'++GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main+exportGIT_TEST_DEFAULT_INITIAL_BRANCH_NAME++../test-lib.sh++test_expect_success"create commit with big blobs (1.5 MB)"'+test-toolgenrandomfoo1500000>big-blob&&+test_commit--appendfoobig-blob&&+test-toolgenrandombar1500000>big-blob&&+test_commit--appendbarbig-blob&&+(+cd.git&&+findobjects/??-typef|sort
I think it would be better to just (could roll this into a function):
test_when_finished "rm -rf dest.git" &&
git init dest.git &&
git -C dest.git config ...
Then you can use it with e.g. --run=3-4 and not have it error out
because of skipped setup.
A lot of our tests fail like that, but in this case fixing it seems
trivial.
+ (
+ cd dest.git &&
+ find objects/?? -type f | sort
..."find" needed over just globbing?:
obj=$(echo objects/*/*)
?
I tried to use "echo" instead of "find". It works well on my personal
computer, but fails due to the "info/commit-graph" generated when CI on
Github.
So it seems that ".git/objects/??" will be more rigorous?
On Fri, Dec 3, 2021 at 9:19 PM Ævar Arnfjörð Bjarmason [off-list ref] wrote:
quoted hunk
On Fri, Dec 03 2021, Han Xin wrote:
quoted
From: Han Xin <redacted>
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.
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. Therefore, only unpack object
larger than the "big_file_threshold" in zstream. See the following
benchmarks:
hyperfine \
--setup \
'if ! test -d scalar.git; then git clone --bare https://github.com/microsoft/scalar.git; cp scalar.git/objects/pack/*.pack small.pack; fi' \
--prepare 'rm -rf dest.git && git init --bare dest.git' \
-n 'old' 'git -C dest.git unpack-objects <small.pack' \
-n 'new' 'new/git -C dest.git unpack-objects <small.pack' \
-n 'new (small threshold)' \
'new/git -c core.bigfilethreshold=16k -C dest.git unpack-objects <small.pack'
Benchmark 1: old
Time (mean ± σ): 6.075 s ± 0.069 s [User: 5.047 s, System: 0.991 s]
Range (min … max): 6.018 s … 6.189 s 10 runs
Benchmark 2: new
Time (mean ± σ): 6.090 s ± 0.033 s [User: 5.075 s, System: 0.976 s]
Range (min … max): 6.030 s … 6.142 s 10 runs
Benchmark 3: new (small threshold)
Time (mean ± σ): 6.755 s ± 0.029 s [User: 5.150 s, System: 1.560 s]
Range (min … max): 6.711 s … 6.809 s 10 runs
Summary
'old' ran
1.00 ± 0.01 times faster than 'new'
1.11 ± 0.01 times faster than 'new (small threshold)'
So before we wrote used core.bigfilethreshold for two things (or more?):
Whether we show a diff for it (we mark it "binary") and whether it's
split into a loose object.
Now it's three things, we've added a "this is a threshold when we'll
stream the object" to that.
Might it make sense to squash something like this in, so we can have our
cake & eat it too?
With this I get, where HEAD~0 is this change:
Summary
'./git -C dest.git -c core.bigfilethreshold=512m unpack-objects <small.pack' in 'HEAD~0' ran
1.00 ± 0.01 times faster than './git -C dest.git -c core.bigfilethreshold=512m unpack-objects <small.pack' in 'HEAD~1'
1.00 ± 0.01 times faster than './git -C dest.git -c core.bigfilethreshold=512m unpack-objects <small.pack' in 'origin/master'
1.01 ± 0.01 times faster than './git -C dest.git -c core.bigfilethreshold=16k unpack-objects <small.pack' in 'HEAD~0'
1.06 ± 0.14 times faster than './git -C dest.git -c core.bigfilethreshold=16k unpack-objects <small.pack' in 'origin/master'
1.20 ± 0.01 times faster than './git -C dest.git -c core.bigfilethreshold=16k unpack-objects <small.pack' in 'HEAD~1'
I.e. it's 5% slower, not 20% (haven't looked into why), but we'll not
stream out 16k..128MB objects (maybe the repo has even bigger ones?)
@@ -424,6 +424,17 @@ be delta compressed, but larger binary media files won't be. + Common unit suffixes of 'k', 'm', or 'g' are supported.+core.bigFileStreamingThreshold::+ Files larger than this will be streamed out to a temporary+ object file while being hashed, which will when be renamed+ in-place to a loose object, particularly if the+ `core.bigFileThreshold' setting dictates that they're always+ written out as loose objects.+++Default is 128 MiB on all platforms.+++Common unit suffixes of 'k', 'm', or 'g' are supported.+ core.excludesFile:: Specifies the pathname to the file that contains patterns to describe paths that are not meant to be tracked, in addition
@@ -400,7 +400,7 @@ static void unpack_non_delta_entry(enum object_type type, unsigned long size,void*buf;/* Write large blob in stream without allocating full buffer. */-if(!dry_run&&type==OBJ_BLOB&&size>big_file_threshold){+if(!dry_run&&type==OBJ_BLOB&&size>big_file_streaming_threshold){write_stream_blob(nr,size);return;}
I'm not sure if we need an additional "core.bigFileStreamingThreshold"
here, because "core.bigFileThreshold" has been widely used in
"index-pack", "read_object" and so on.
In the test case which uses "core.bigFileStreamingThreshold" instead of
"core.bigFileThreshold", I found the test case execution failed because
of "fsck", who tried to allocate 15MB of memory.
In the process of "fsck_loose()", "read_loose_object()" will be called,
which contains the following content:
if (*oi->typep == OBJ_BLOB && *size> big_file_threshold) {
if (check_stream_oid(&stream, hdr, *size, path, expected_oid) <0)
goto out;
} else {
/* this will allocate 15MB of memory */
*contents = unpack_loose_rest(&stream, hdr, *size, expected_oid);
...
}
The same case can be found in "unpack_entry_data()":
static char fixed_buf[8192];
...
if (type == OBJ_BLOB && size > big_file_threshold)
buf = fixed_buf;
else
buf = xmallocz(size);
...
Although I know that setting a "core.bigfilethreshold" smaller than the
default value on the server side does not help me prevent users from
creating large delta objects on the client side, it can still
effectively help me reduce the Memory allocation in "receive-pack".
If this is not the correct way to use "core.bigfilethreshold", maybe
you can share some better solutions to me, if you want.
Thanks.
-Han Xin
Just a side-note, I think (but am not 100% sure) that these existing
occurances aren't needed due to our use of CALLOC_ARRAY():
diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c
index 4a9466295ba..00b349412c5 100644
--- a/builtin/unpack-objects.c
+++ b/builtin/unpack-objects.c
@@ -248,7 +248,6 @@ static void write_object(unsigned nr, enum object_type type,
die("failed to write object");
added_object(nr, type, buf, size);
free(buf);
- obj_list[nr].obj = NULL;
} else if (type == OBJ_BLOB) {
struct blob *blob;
if (write_object_file(buf, size, type_name(type),
@@ -262,7 +261,6 @@ static void write_object(unsigned nr, enum object_type type,
blob->object.flags |= FLAG_WRITTEN;
else
die("invalid blob object");
- obj_list[nr].obj = NULL;
} else {
struct object *obj;
int eaten;
The reason I'm noting it is that the same seems to be true of your new
addition here. I.e. are these assignments to NULL needed?
Anyway, the reason I started poking at this it tha this
write_stream_blob() seems to duplicate much of write_object(). AFAICT
only the writing part is really different, the part where we
lookup_blob() after, set FLAG_WRITTEN etc. is all the same.
Why can't we call write_object() here?
The obvious answer seems to be that the call to write_object_file()
isn't prepared to do the sort of streaming that you want, so instead
you're bypassing it and calling write_loose_object() directly.
I haven't tried this myself, but isn't a better and cleaner approach
here to not add another meaning to what is_null_oid() means, but to just
add a HASH_STREAM flag that'll get passed down as "unsigned flags" to
write_loose_object()? See FLAG_BITS in object.h.
Then the "obj_list[nr].obj" here could also become
"obj_list[nr].obj.flags |= (1u<<12)" or whatever (but that wouldn't
strictly be needed I think.
But by adding the "HASH_STREAM" flag you could I think stop duplicating
the "Generate the header" etc. here and call write_object_file_flags().
I don't so much care about how it's done within unpack-objects.c, but
not having another meaning to is_null_oid() in play would be really
nice, and it this case it seems entirely avoidable.
I did refactor it according to your suggestions in my next patch version.
Using a HASH_STREAM tag is indeed a better way to deal with it, and it
can also reduce my refactor to the original contents.
Thanks.
-Han Xin
From: Han Xin <redacted>
Changes since v3:
* Add "size" to "struct input_stream" which used by following commits.
* Increase the buffer size of "struct input_zstream_data" from 4096 to
8192, which is consistent with the "fixed_buf" in the "index-pack.c".
* Refactor "read stream in a loop in write_loose_object()" which
introduced a performance problem reported by Derrick Stolee[1].
Thank you for finding the issue. It seems simple enough to add that size
information and regain the performance back to nearly no overhead. Your
hyperfine statistics are within noise, which is great. Thanks!
-Stolee
From: Han Xin <redacted>
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.
This can be improved by feeding data to "write_loose_object()" in a
stream. The input stream is implemented as an interface.
In the first step, we add a new flag called "HASH_STREAM" and make a
simple implementation, feeding the entire buffer in the stream to
"write_loose_object()" as a refactor.
Helped-by: Ævar Arnfjörð Bjarmason [off-list ref]
Helped-by: Jiang Xin [off-list ref]
Signed-off-by: Han Xin <redacted>
---
cache.h | 1 +
object-file.c | 7 ++++++-
object-store.h | 5 +++++
3 files changed, 12 insertions(+), 1 deletion(-)
@@ -1898,7 +1898,12 @@ static int write_loose_object(const struct object_id *oid, char *hdr,the_hash_algo->update_fn(&c,hdr,hdrlen);/* Then the data itself.. */-stream.next_in=(void*)buf;+if(flags&HASH_STREAM){+structinput_stream*in_stream=(structinput_stream*)buf;+stream.next_in=(void*)in_stream->read(in_stream,&len);+}else{+stream.next_in=(void*)buf;+}stream.avail_in=len;do{unsignedchar*in0=stream.next_in;
From: Han Xin <redacted>
When streaming a large blob object to "write_loose_object()", we have no
chance to run "write_object_file_prepare()" to calculate the oid in
advance. So we need to handle undetermined oid in function
"write_loose_object()".
In the original implementation, 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 promise that "oid" is constant in "write_loose_object()" has been
removed because it will be filled after reading all stream data.
Helped-by: Ævar Arnfjörð Bjarmason [off-list ref]
Helped-by: Jiang Xin [off-list ref]
Signed-off-by: Han Xin <redacted>
---
object-file.c | 48 +++++++++++++++++++++++++++++++++++++++---------
1 file changed, 39 insertions(+), 9 deletions(-)
@@ -1872,16 +1872,21 @@ static int write_loose_object(const struct object_id *oid, char *hdr,staticstructstrbuftmp_file=STRBUF_INIT;staticstructstrbuffilename=STRBUF_INIT;-loose_object_path(the_repository,&filename,oid);+if(flags&HASH_STREAM)+/* When oid is not determined, save tmp file to odb path. */+strbuf_addf(&filename,"%s/",get_object_directory());+else+loose_object_path(the_repository,&filename,oid);fd=create_tmpfile(&tmp_file,filename.buf);if(fd<0){if(flags&HASH_SILENT)-return-1;+err=-1;elseif(errno==EACCES)-returnerror(_("insufficient permission for adding an object to repository database %s"),get_object_directory());+err=error(_("insufficient permission for adding an object to repository database %s"),get_object_directory());else-returnerror_errno(_("unable to create temporary file"));+err=error_errno(_("unable to create temporary file"));+gotocleanup;}/* Set it up */
@@ -1923,12 +1928,34 @@ static int write_loose_object(const struct object_id *oid, char *hdr,die(_("deflateEnd on object %s failed (%d)"),oid_to_hex(oid),ret);the_hash_algo->final_oid_fn(¶no_oid,&c);-if(!oideq(oid,¶no_oid))+if(!(flags&HASH_STREAM)&&!oideq(oid,¶no_oid))die(_("confused by unstable object source data for %s"),oid_to_hex(oid));close_loose_object(fd);+if(flags&HASH_STREAM){+intdirlen;++oidcpy((structobject_id*)oid,¶no_oid);+loose_object_path(the_repository,&filename,oid);++/* We finally know the object path, and create the missing dir. */+dirlen=directory_size(filename.buf);+if(dirlen){+structstrbufdir=STRBUF_INIT;+strbuf_add(&dir,filename.buf,dirlen-1);+if(mkdir(dir.buf,0777)&&errno!=EEXIST)+err=-1;+elseif(adjust_shared_perm(dir.buf))+err=-1;+else+strbuf_release(&dir);+if(err<0)+gotocleanup;+}+}+if(mtime){structutimbufutb;utb.actime=mtime;
@@ -1938,7 +1965,10 @@ static int write_loose_object(const struct object_id *oid, char *hdr,warning_errno(_("failed utime() on %s"),tmp_file.buf);}-returnfinalize_object_file(tmp_file.buf,filename.buf);+err=finalize_object_file(tmp_file.buf,filename.buf);+cleanup:+strbuf_release(&filename);+returnerr;}staticintfreshen_loose_object(conststructobject_id*oid)
@@ -2015,7 +2045,7 @@ int force_object_loose(const struct object_id *oid, time_t mtime)if(!buf)returnerror(_("cannot read object for %s"),oid_to_hex(oid));hdrlen=xsnprintf(hdr,sizeof(hdr),"%s %"PRIuMAX,type_name(type),(uintmax_t)len)+1;-ret=write_loose_object(oid,hdr,hdrlen,buf,len,mtime,0);+ret=write_loose_object((structobject_id*)oid,hdr,hdrlen,buf,len,mtime,0);free(buf);returnret;
From: Han Xin <redacted>
In order to prepare the stream version of "write_loose_object()", read
the input stream in a loop in "write_loose_object()", so that we can
feed the contents of large blob object to "write_loose_object()" using
a small fixed buffer.
Helped-by: Jiang Xin [off-list ref]
Signed-off-by: Han Xin <redacted>
---
object-file.c | 23 +++++++++++++++--------
1 file changed, 15 insertions(+), 8 deletions(-)
@@ -1903,22 +1903,29 @@ static int write_loose_object(struct object_id *oid, char *hdr,the_hash_algo->update_fn(&c,hdr,hdrlen);/* Then the data itself.. */-if(flags&HASH_STREAM){-structinput_stream*in_stream=(structinput_stream*)buf;-stream.next_in=(void*)in_stream->read(in_stream,&len);-}else{+if(!(flags&HASH_STREAM)){stream.next_in=(void*)buf;+stream.avail_in=len;+flush=Z_FINISH;}-stream.avail_in=len;do{unsignedchar*in0=stream.next_in;-ret=git_deflate(&stream,Z_FINISH);+if(flags&HASH_STREAM&&!stream.avail_in){+structinput_stream*in_stream=(structinput_stream*)buf;+constvoid*in=in_stream->read(in_stream,&stream.avail_in);+stream.next_in=(void*)in;+in0=(unsignedchar*)in;+/* All data has been read. */+if(len+hdrlen==stream.total_in+stream.avail_in)+flush=Z_FINISH;+}+ret=git_deflate(&stream,flush);the_hash_algo->update_fn(&c,in0,stream.next_in-in0);if(write_buffer(fd,compressed,stream.next_out-compressed)<0)die(_("unable to write loose object file"));stream.next_out=compressed;stream.avail_out=sizeof(compressed);-}while(ret==Z_OK);+}while(ret==Z_OK||ret==Z_BUF_ERROR);if(ret!=Z_STREAM_END)die(_("unable to deflate new object %s (%d)"),oid_to_hex(oid),
From: Han Xin <redacted>
In dry_run mode, "get_data()" is used to verify the inflation of data,
and the returned buffer will not be used at all and will be freed
immediately. Even in dry_run mode, it is dangerous to allocate a
full-size buffer for a large blob object. Therefore, only allocate a
low memory footprint when calling "get_data()" in dry_run mode.
Suggested-by: Jiang Xin <redacted>
Signed-off-by: Han Xin <redacted>
---
builtin/unpack-objects.c | 18 ++++++++++++------
1 file changed, 12 insertions(+), 6 deletions(-)
@@ -396,7 +402,7 @@ static void unpack_delta_entry(enum object_type type, unsigned long delta_size,if(base_offset<=0||base_offset>=obj_list[nr].offset)die("offset value out of bound for delta base object");-delta_data=get_data(delta_size);+delta_data=get_data(delta_size,dry_run);if(dry_run||!delta_data){free(delta_data);return;
From: Han Xin <redacted>
We will use "write_object_file_flags()" in "unpack_non_delta_entry()" to
read the entire data contents in stream. When read in stream, we needn't
prepare "oid" before "write_loose_object()", only generate the header.
Signed-off-by: Han Xin <redacted>
---
object-file.c | 5 +++++
1 file changed, 5 insertions(+)
@@ -2002,6 +2002,11 @@ int write_object_file_flags(const void *buf, unsigned long len,{charhdr[MAX_HEADER_LEN];inthdrlen=sizeof(hdr);+if(flags&HASH_STREAM){+/* Generate the header */+hdrlen=xsnprintf(hdr,hdrlen,"%s %"PRIuMAX,type,(uintmax_t)len)+1;+returnwrite_loose_object(oid,hdr,hdrlen,buf,len,0,flags);+}/* Normally if we have it in the pack then we do not bother writing*itoutinto.git/objects/??/?{38}file.
From: Han Xin <redacted>
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.
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. Therefore, only unpack object
larger than the "core.BigFileStreamingThreshold" in zstream. See the following
benchmarks:
hyperfine \
--setup \
'if ! test -d scalar.git; then git clone --bare https://github.com/microsoft/scalar.git; cp scalar.git/objects/pack/*.pack small.pack; fi' \
--prepare 'rm -rf dest.git && git init --bare dest.git'
Summary
'./git -C dest.git -c core.bigfilethreshold=512m unpack-objects <small.pack' in 'origin/master'
1.01 ± 0.04 times faster than './git -C dest.git -c core.bigfilethreshold=512m unpack-objects <small.pack' in 'HEAD~1'
1.01 ± 0.04 times faster than './git -C dest.git -c core.bigfilethreshold=512m unpack-objects <small.pack' in 'HEAD~0'
1.03 ± 0.10 times faster than './git -C dest.git -c core.bigfilethreshold=16k unpack-objects <small.pack' in 'origin/master'
1.02 ± 0.07 times faster than './git -C dest.git -c core.bigfilethreshold=16k unpack-objects <small.pack' in 'HEAD~0'
1.10 ± 0.04 times faster than './git -C dest.git -c core.bigfilethreshold=16k unpack-objects <small.pack' in 'HEAD~1'
Helped-by: Ævar Arnfjörð Bjarmason [off-list ref]
Helped-by: Derrick Stolee [off-list ref]
Helped-by: Jiang Xin [off-list ref]
Signed-off-by: Han Xin <redacted>
---
Documentation/config/core.txt | 11 +++++
builtin/unpack-objects.c | 70 ++++++++++++++++++++++++++++-
cache.h | 1 +
config.c | 5 +++
environment.c | 1 +
t/t5590-unpack-non-delta-objects.sh | 70 +++++++++++++++++++++++++++++
6 files changed, 157 insertions(+), 1 deletion(-)
create mode 100755 t/t5590-unpack-non-delta-objects.sh
@@ -424,6 +424,17 @@ be delta compressed, but larger binary media files won't be. + Common unit suffixes of 'k', 'm', or 'g' are supported.+core.bigFileStreamingThreshold::+ Files larger than this will be streamed out to a temporary+ object file while being hashed, which will when be renamed+ in-place to a loose object, particularly if the+ `core.bigFileThreshold' setting dictates that they're always+ written out as loose objects.+++Default is 128 MiB on all platforms.+++Common unit suffixes of 'k', 'm', or 'g' are supported.+ core.excludesFile:: Specifies the pathname to the file that contains patterns to describe paths that are not meant to be tracked, in addition
@@ -326,11 +326,79 @@ static void added_object(unsigned nr, enum object_type type,}}+structinput_zstream_data{+git_zstream*zstream;+unsignedcharbuf[8192];+intstatus;+};++staticconstvoid*feed_input_zstream(structinput_stream*in_stream,unsignedlong*readlen)+{+structinput_zstream_data*data=in_stream->data;+git_zstream*zstream=data->zstream;+void*in=fill(1);++if(!len||data->status==Z_STREAM_END){+*readlen=0;+returnNULL;+}++zstream->next_out=data->buf;+zstream->avail_out=sizeof(data->buf);+zstream->next_in=in;+zstream->avail_in=len;++data->status=git_inflate(zstream,0);+use(len-zstream->avail_in);+*readlen=sizeof(data->buf)-zstream->avail_out;++returndata->buf;+}++staticvoidwrite_stream_blob(unsignednr,unsignedlongsize)+{+git_zstreamzstream;+structinput_zstream_datadata;+structinput_streamin_stream={+.read=feed_input_zstream,+.data=&data,+};+intret;++memset(&zstream,0,sizeof(zstream));+memset(&data,0,sizeof(data));+data.zstream=&zstream;+git_inflate_init(&zstream);++if((ret=write_object_file_flags(&in_stream,size,type_name(OBJ_BLOB),&obj_list[nr].oid,HASH_STREAM)))+die(_("failed to write object in stream %d"),ret);++if(zstream.total_out!=size||data.status!=Z_STREAM_END)+die(_("inflate returned %d"),data.status);+git_inflate_end(&zstream);++if(strict&&!dry_run){+structblob*blob=lookup_blob(the_repository,&obj_list[nr].oid);+if(blob)+blob->object.flags|=FLAG_WRITTEN;+else+die("invalid blob object from stream");+}+obj_list[nr].obj=NULL;+}+staticvoidunpack_non_delta_entry(enumobject_typetype,unsignedlongsize,unsignednr){-void*buf=get_data(size,dry_run);+void*buf;++/* Write large blob in stream without allocating full buffer. */+if(!dry_run&&type==OBJ_BLOB&&size>big_file_streaming_threshold){+write_stream_blob(nr,size);+return;+}+buf=get_data(size,dry_run);if(!dry_run&&buf)write_object(nr,type,buf,size);else
@@ -0,0 +1,70 @@+#!/bin/sh+#+# Copyright (c) 2021 Han Xin+#++test_description='Test unpack-objects when receive pack'++GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main+exportGIT_TEST_DEFAULT_INITIAL_BRANCH_NAME++../test-lib.sh++prepare_dest(){+test_when_finished"rm -rf dest.git"&&+gitinit--baredest.git&&+git-Cdest.gitconfigcore.bigFileStreamingThreshold$1+git-Cdest.gitconfigcore.bigFileThreshold$1+}++test_expect_success"setup repo with big blobs (1.5 MB)"'+test-toolgenrandomfoo1500000>big-blob&&+test_commit--appendfoobig-blob&&+test-toolgenrandombar1500000>big-blob&&+test_commit--appendbarbig-blob&&+(+cd.git&&+findobjects/??-typef|sort+)>expect&&+PACK=$(echomain|gitpack-objects--revstest)+'++test_expect_success'setup env: GIT_ALLOC_LIMIT to 1MB''+GIT_ALLOC_LIMIT=1m&&+exportGIT_ALLOC_LIMIT+'++test_expect_success'fail to unpack-objects: cannot allocate''+prepare_dest2m&&+test_must_failgit-Cdest.gitunpack-objects<test-$PACK.pack2>err&&+grep"fatal: attempting to allocate"err&&+(+cddest.git&&+findobjects/??-typef|sort+)>actual&&+test_file_not_emptyactual&&+!test_cmpexpectactual+'++test_expect_success'unpack big object in stream''+prepare_dest1m&&+git-Cdest.gitunpack-objects<test-$PACK.pack&&+git-Cdest.gitfsck&&+(+cddest.git&&+findobjects/??-typef|sort+)>actual&&+test_cmpexpectactual+'++test_expect_success'unpack-objects dry-run''+prepare_dest1m&&+git-Cdest.gitunpack-objects-n<test-$PACK.pack&&+(+cddest.git&&+findobjects/-typef+)>actual&&+test_must_be_emptyactual+'++test_done
From: Han Xin <redacted>
When streaming a large blob object to "write_loose_object()", we have no
chance to run "write_object_file_prepare()" to calculate the oid in
advance. So we need to handle undetermined oid in function
"write_loose_object()".
In the original implementation, 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 promise that "oid" is constant in "write_loose_object()" has been
removed because it will be filled after reading all stream data.
Helped-by: Ævar Arnfjörð Bjarmason [off-list ref]
Helped-by: Jiang Xin [off-list ref]
Signed-off-by: Han Xin <redacted>
---
object-file.c | 48 +++++++++++++++++++++++++++++++++++++++---------
1 file changed, 39 insertions(+), 9 deletions(-)
@@ -1872,16 +1872,21 @@ static int write_loose_object(const struct object_id *oid, char *hdr,staticstructstrbuftmp_file=STRBUF_INIT;staticstructstrbuffilename=STRBUF_INIT;-loose_object_path(the_repository,&filename,oid);+if(flags&HASH_STREAM)+/* When oid is not determined, save tmp file to odb path. */+strbuf_addf(&filename,"%s/",get_object_directory());+else+loose_object_path(the_repository,&filename,oid);fd=create_tmpfile(&tmp_file,filename.buf);if(fd<0){if(flags&HASH_SILENT)-return-1;+err=-1;elseif(errno==EACCES)-returnerror(_("insufficient permission for adding an object to repository database %s"),get_object_directory());+err=error(_("insufficient permission for adding an object to repository database %s"),get_object_directory());else-returnerror_errno(_("unable to create temporary file"));+err=error_errno(_("unable to create temporary file"));+gotocleanup;}/* Set it up */
@@ -1923,12 +1928,34 @@ static int write_loose_object(const struct object_id *oid, char *hdr,die(_("deflateEnd on object %s failed (%d)"),oid_to_hex(oid),ret);the_hash_algo->final_oid_fn(¶no_oid,&c);-if(!oideq(oid,¶no_oid))+if(!(flags&HASH_STREAM)&&!oideq(oid,¶no_oid))die(_("confused by unstable object source data for %s"),oid_to_hex(oid));
Here we don't have a meaningful "const" OID anymore, but still if we die
we use the "oid".
close_loose_object(fd);
+ if (flags & HASH_STREAM) {
+ int dirlen;
+
+ oidcpy((struct object_id *)oid, ¶no_oid);
This cast isn't needed anymore now that you stripped the "const" off,
but more on that later...
+ loose_object_path(the_repository, &filename, oid);
+
+ /* We finally know the object path, and create the missing dir. */
+ dirlen = directory_size(filename.buf);
+ if (dirlen) {
+ struct strbuf dir = STRBUF_INIT;
+ strbuf_add(&dir, filename.buf, dirlen - 1);
+ if (mkdir(dir.buf, 0777) && errno != EEXIST)
+ err = -1;
+ else if (adjust_shared_perm(dir.buf))
+ err = -1;
+ else
+ strbuf_release(&dir);
+ if (err < 0)
+ goto cleanup;
Can't we use one of the existing utility functions for this? Testing
locally I could replace this with:
diff --git a/object-file.c b/object-file.c
index 7c93db11b2d..05e1fae893d 100644
--- a/object-file.c
+++ b/object-file.c
@@ -1952,14 +1952,11 @@ static int write_loose_object(struct object_id *oid, char *hdr,
if (dirlen) {
struct strbuf dir = STRBUF_INIT;
strbuf_add(&dir, filename.buf, dirlen - 1);
- if (mkdir(dir.buf, 0777) && errno != EEXIST)
+
+ if (mkdir_in_gitdir(dir.buf) < 0) {
err = -1;
- else if (adjust_shared_perm(dir.buf))
- err = -1;
- else
- strbuf_release(&dir);
- if (err < 0)
goto cleanup;
+ }
}
}
And your tests still pass. Maybe they have a blind spot, or maybe we can
just use the existing function.
Reading this series is an odd mixture of of things that would really be
much easier to understand if they were combined, e.g. 1/6 adding APIs
that aren't used by anything, but then adding one codepath (also
unused), that we then use later. Could just add it at the same time as
the use and the patch would be easier to read....
...and then this, which *is* something that could be split up into an
earlier cleanup step, i.e. the strbuf leak here exists before this
series, fixing it is good, but splitting that up into its own patch
would make this diff smaller & the actual behavior changes easier to
reason about.
quoted hunk
static int freshen_loose_object(const struct object_id *oid)
...on the "more on that later", here we're casting the "oid" from const
for a function that's never going to be involved in the streaming
codepath.
I know I suggested the HASH_STREAM flag, but what I was really going for
was "let's share more of the code?", looking at this v5 (which is
already much better than v4) I think a better approach is to split up
write_loose_object().
I.e. it already calls close_loose_object() and finalize_object_file() to
do some of its work, but around that we have:
1. Figuring out a path for the (temp) object file
2. Creating the tempfile
3. Setting up zlib
4. Once zlib is set up inspect its state, die with a message
about oid_to_hex(oid) if we failed
5. Optionally, do HASH_STREAM stuff
Maybe force a loose object if "mtime".
I think if that's split up so that each of those is its own little
function what's now write_loose_object() can call those in sequence, and
a new stream_loose_object() can just do #1 differentl, followed by the
same #2 and #4, but do #4 differently etc.
You'll still be able to re-use the write_object_file_prepare()
etc. logic.
As an example your 5/6 copy/pastes the xsnprintf() formatting of the
object header. It's just one line, but it's also code that's very
central to git, so I think instead of just copy/pasting it a prep step
of factoring it out would make sense, and that would be a prep cleanup
that would help later readability. E.g.:
diff --git a/object-file.c b/object-file.c
index eac67f6f5f9..a7dcbd929e9 100644
--- a/object-file.c
+++ b/object-file.c
@@ -1009,6 +1009,13 @@ void *xmmap(void *start, size_t length,
return ret;
}
+static int generate_object_header(char *buf, int bufsz, const char *type_name,
+ unsigned long size)
+{
+ return xsnprintf(buf, bufsz, "%s %"PRIuMAX , type_name,
+ (uintmax_t)size) + 1;
+}
+
/*
* With an in-core object data in "map", rehash it to make sure the
* object name actually matches "oid" to detect object corruption.
@@ -1037,7 +1044,7 @@ int check_object_signature(struct repository *r, const struct object_id *oid,
return -1;
/* Generate the header */
- hdrlen = xsnprintf(hdr, sizeof(hdr), "%s %"PRIuMAX , type_name(obj_type), (uintmax_t)size) + 1;
+ hdrlen = generate_object_header(hdr, sizeof(hdr), type_name(obj_type), size);
/* Sha1.. */
r->hash_algo->init_fn(&c);
@@ -1737,7 +1744,7 @@ static void write_object_file_prepare(const struct git_hash_algo *algo,
git_hash_ctx c;
/* Generate the header */
- *hdrlen = xsnprintf(hdr, *hdrlen, "%s %"PRIuMAX , type, (uintmax_t)len)+1;
+ *hdrlen = generate_object_header(hdr, *hdrlen, type, len);
/* Sha1.. */
algo->init_fn(&c);
@@ -2009,7 +2016,7 @@ int force_object_loose(const struct object_id *oid, time_t mtime)
buf = read_object(the_repository, oid, &type, &len);
if (!buf)
return error(_("cannot read object for %s"), oid_to_hex(oid));
- hdrlen = xsnprintf(hdr, sizeof(hdr), "%s %"PRIuMAX , type_name(type), (uintmax_t)len) + 1;
+ hdrlen = generate_object_header(hdr, sizeof(hdr), type_name(type), len);
ret = write_loose_object(oid, hdr, hdrlen, buf, len, mtime, 0);
free(buf);
Then in your change on top you just call that generate_object_header(),
or better yet your amended write_object_file_flags() can just call a
similarly amended write_object_file_prepare() directly.
@@ -1874,11 +1874,14 @@ static int write_loose_object(const struct object_id *oid, char *hdr,fd=create_tmpfile(&tmp_file,filename.buf);if(fd<0){if(flags&HASH_SILENT)-return-1;+ret=-1;elseif(errno==EACCES)-returnerror(_("insufficient permission for adding an object to repository database %s"),get_object_directory());+ret=error(_("insufficient permission for adding an "+"object to repository database %s"),+get_object_directory());else-returnerror_errno(_("unable to create temporary file"));+ret=error_errno(_("unable to create temporary file"));+gotocleanup;}/* Set it up */
@@ -1930,7 +1933,11 @@ static int write_loose_object(const struct object_id *oid, char *hdr,warning_errno(_("failed utime() on %s"),tmp_file.buf);}-returnfinalize_object_file(tmp_file.buf,filename.buf);+ret=finalize_object_file(tmp_file.buf,filename.buf);+cleanup:+strbuf_release(&filename);+strbuf_release(&tmp_file);+returnret;}staticintfreshen_loose_object(conststructobject_id*oid)
From: Han Xin <redacted>
There are 3 places where "xsnprintf" is used to generate the object
header, and I originally planned to add a fourth in the latter patch.
According to Ævar Arnfjörð Bjarmason’s suggestion, although it's just
one line, it's also code that's very central to git, so reafactor them
into a function which will help later readability.
Helped-by: Ævar Arnfjörð Bjarmason [off-list ref]
Signed-off-by: Han Xin <redacted>
---
object-file.c | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
From: Han Xin <redacted>
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.
This can be improved by feeding data to "stream_loose_object()" in
stream instead of read into the whole buf.
As this new method "stream_loose_object()" has many similarities with
"write_loose_object()", we split up "write_loose_object()" into some
steps:
1. Figuring out a path for the (temp) object file.
2. Creating the tempfile.
3. Setting up zlib and write header.
4. Write object data and handle errors.
5. Optionally, do someting after write, maybe force a loose object if
"mtime".
Helped-by: Ævar Arnfjörð Bjarmason [off-list ref]
Signed-off-by: Han Xin <redacted>
---
object-file.c | 98 +++++++++++++++++++++++++++++++++------------------
1 file changed, 63 insertions(+), 35 deletions(-)
@@ -1854,17 +1876,48 @@ static int create_tmpfile(struct strbuf *tmp, const char *filename)strbuf_reset(tmp);strbuf_add(tmp,filename,dirlen-1);if(mkdir(tmp->buf,0777)&&errno!=EEXIST)-return-1;+break;if(adjust_shared_perm(tmp->buf))-return-1;+break;/* Try again */strbuf_addstr(tmp,"/tmp_obj_XXXXXX");fd=git_mkstemp_mode(tmp->buf,0444);+}while(0);++if(fd<0&&!(flags&HASH_SILENT)){+if(errno==EACCES)+returnerror(_("insufficient permission for adding an "+"object to repository database %s"),+get_object_directory());+else+returnerror_errno(_("unable to create temporary file"));}+returnfd;}+staticvoidsetup_stream_and_header(git_zstream*stream,+unsignedchar*compressed,+unsignedlongcompressed_size,+git_hash_ctx*c,+char*hdr,+inthdrlen)+{+/* Set it up */+git_deflate_init(stream,zlib_compression_level);+stream->next_out=compressed;+stream->avail_out=compressed_size;+the_hash_algo->init_fn(c);++/* First header.. */+stream->next_in=(unsignedchar*)hdr;+stream->avail_in=hdrlen;+while(git_deflate(stream,0)==Z_OK)+;/* nothing */+the_hash_algo->update_fn(c,hdr,hdrlen);+}+staticintwrite_loose_object(conststructobject_id*oid,char*hdr,inthdrlen,constvoid*buf,unsignedlonglen,time_tmtime,unsignedflags)
@@ -1879,31 +1932,15 @@ static int write_loose_object(const struct object_id *oid, char *hdr,loose_object_path(the_repository,&filename,oid);-fd=create_tmpfile(&tmp_file,filename.buf);+fd=create_tmpfile(&tmp_file,filename.buf,flags);if(fd<0){-if(flags&HASH_SILENT)-ret=-1;-elseif(errno==EACCES)-ret=error(_("insufficient permission for adding an "-"object to repository database %s"),-get_object_directory());-else-ret=error_errno(_("unable to create temporary file"));+ret=-1;gotocleanup;}-/* Set it up */-git_deflate_init(&stream,zlib_compression_level);-stream.next_out=compressed;-stream.avail_out=sizeof(compressed);-the_hash_algo->init_fn(&c);--/* First header.. */-stream.next_in=(unsignedchar*)hdr;-stream.avail_in=hdrlen;-while(git_deflate(&stream,0)==Z_OK)-;/* nothing */-the_hash_algo->update_fn(&c,hdr,hdrlen);+/* Set it up and write header */+setup_stream_and_header(&stream,compressed,sizeof(compressed),+&c,hdr,hdrlen);/* Then the data itself.. */stream.next_in=(void*)buf;
@@ -1932,16 +1969,7 @@ static int write_loose_object(const struct object_id *oid, char *hdr,close_loose_object(fd);-if(mtime){-structutimbufutb;-utb.actime=mtime;-utb.modtime=mtime;-if(utime(tmp_file.buf,&utb)<0&&-!(flags&HASH_SILENT))-warning_errno(_("failed utime() on %s"),tmp_file.buf);-}--ret=finalize_object_file(tmp_file.buf,filename.buf);+ret=finalize_object_file_with_mtime(tmp_file.buf,filename.buf,mtime,flags);cleanup:strbuf_release(&filename);strbuf_release(&tmp_file);
From: Han Xin <redacted>
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.
This can be improved by feeding data to "stream_loose_object()" in a
stream. The input stream is implemented as an interface.
When streaming a large blob object to "write_loose_object()", we have no
chance to run "write_object_file_prepare()" to calculate the oid in
advance. So we need to handle undetermined oid in a new function called
"stream_loose_object()".
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.
We will reuse "write_object_file_flags()" in "unpack_non_delta_entry()" to
read the entire data contents in stream, so a new flag "HASH_STREAM" is
added. When read in stream, we needn't prepare the "oid" before
"write_loose_object()", only generate the header.
"freshen_packed_object()" or "freshen_loose_object()" will be called
inside "stream_loose_object()" after obtaining the "oid".
Helped-by: Ævar Arnfjörð Bjarmason [off-list ref]
Helped-by: Jiang Xin [off-list ref]
Signed-off-by: Han Xin <redacted>
---
cache.h | 1 +
object-file.c | 92 ++++++++++++++++++++++++++++++++++++++++++++++++++
object-store.h | 5 +++
3 files changed, 98 insertions(+)
@@ -1994,6 +1994,88 @@ static int freshen_packed_object(const struct object_id *oid)return1;}+staticintstream_loose_object(structobject_id*oid,char*hdr,inthdrlen,+conststructinput_stream*in_stream,+unsignedlonglen,time_tmtime,unsignedflags)+{+intfd,ret,err=0,flush=0;+unsignedcharcompressed[4096];+git_zstreamstream;+git_hash_ctxc;+structobject_idparano_oid;+staticstructstrbuftmp_file=STRBUF_INIT;+staticstructstrbuffilename=STRBUF_INIT;+intdirlen;++/* When oid is not determined, save tmp file to odb path. */+strbuf_addf(&filename,"%s/",get_object_directory());++fd=create_tmpfile(&tmp_file,filename.buf,flags);+if(fd<0){+err=-1;+gotocleanup;+}++/* Set it up and write header */+setup_stream_and_header(&stream,compressed,sizeof(compressed),+&c,hdr,hdrlen);++/* Then the data itself.. */+do{+unsignedchar*in0=stream.next_in;+if(!stream.avail_in){+constvoid*in=in_stream->read(in_stream,&stream.avail_in);+stream.next_in=(void*)in;+in0=(unsignedchar*)in;+/* All data has been read. */+if(len+hdrlen==stream.total_in+stream.avail_in)+flush=Z_FINISH;+}+ret=git_deflate(&stream,flush);+the_hash_algo->update_fn(&c,in0,stream.next_in-in0);+if(write_buffer(fd,compressed,stream.next_out-compressed)<0)+die(_("unable to write loose object file"));+stream.next_out=compressed;+stream.avail_out=sizeof(compressed);+}while(ret==Z_OK||ret==Z_BUF_ERROR);++if(ret!=Z_STREAM_END)+die(_("unable to deflate new object streamingly (%d)"),ret);+ret=git_deflate_end_gently(&stream);+if(ret!=Z_OK)+die(_("deflateEnd on object streamingly failed (%d)"),ret);+the_hash_algo->final_oid_fn(¶no_oid,&c);++close_loose_object(fd);++oidcpy(oid,¶no_oid);++if(freshen_packed_object(oid)||freshen_loose_object(oid)){+unlink_or_warn(tmp_file.buf);+gotocleanup;+}++loose_object_path(the_repository,&filename,oid);++/* We finally know the object path, and create the missing dir. */+dirlen=directory_size(filename.buf);+if(dirlen){+structstrbufdir=STRBUF_INIT;+strbuf_add(&dir,filename.buf,dirlen-1);++if(mkdir_in_gitdir(dir.buf)<0){+err=-1;+gotocleanup;+}+}++err=finalize_object_file_with_mtime(tmp_file.buf,filename.buf,mtime,flags);+cleanup:+strbuf_release(&tmp_file);+strbuf_release(&filename);+returnerr;+}+intwrite_object_file_flags(constvoid*buf,unsignedlonglen,constchar*type,structobject_id*oid,unsignedflags)
@@ -2001,6 +2083,16 @@ int write_object_file_flags(const void *buf, unsigned long len,charhdr[MAX_HEADER_LEN];inthdrlen=sizeof(hdr);+/* When streaming a large blob object (marked as HASH_STREAM),+*wehavenochancetorun"write_object_file_prepare()"to+*calculatethe"oid"inadvance.Call"stream_loose_object()"+*towritelooseobjectinstream.+*/+if(flags&HASH_STREAM){+hdrlen=generate_object_header(hdr,hdrlen,type,len);+returnstream_loose_object(oid,hdr,hdrlen,buf,len,0,flags);+}+/* Normally if we have it in the pack then we do not bother writing*itoutinto.git/objects/??/?{38}file.*/
From: Han Xin <redacted>
In dry_run mode, "get_data()" is used to verify the inflation of data,
and the returned buffer will not be used at all and will be freed
immediately. Even in dry_run mode, it is dangerous to allocate a
full-size buffer for a large blob object. Therefore, only allocate a
low memory footprint when calling "get_data()" in dry_run mode.
Suggested-by: Jiang Xin <redacted>
Signed-off-by: Han Xin <redacted>
---
builtin/unpack-objects.c | 23 +++++++++++++++++------
1 file changed, 17 insertions(+), 6 deletions(-)
@@ -396,7 +407,7 @@ static void unpack_delta_entry(enum object_type type, unsigned long delta_size,if(base_offset<=0||base_offset>=obj_list[nr].offset)die("offset value out of bound for delta base object");-delta_data=get_data(delta_size);+delta_data=get_data(delta_size,dry_run);if(dry_run||!delta_data){free(delta_data);return;
From: Han Xin <redacted>
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.
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. Therefore, only unpack
object larger than the "core.BigFileStreamingThreshold" in zstream. See
the following benchmarks:
hyperfine \
--setup \
'if ! test -d scalar.git; then git clone --bare https://github.com/microsoft/scalar.git; cp scalar.git/objects/pack/*.pack small.pack; fi' \
--prepare 'rm -rf dest.git && git init --bare dest.git'
Summary
'./git -C dest.git -c core.bigfilethreshold=512m unpack-objects <small.pack' in 'origin/master'
1.01 ± 0.04 times faster than './git -C dest.git -c core.bigfilethreshold=512m unpack-objects <small.pack' in 'HEAD~1'
1.01 ± 0.04 times faster than './git -C dest.git -c core.bigfilethreshold=512m unpack-objects <small.pack' in 'HEAD~0'
1.03 ± 0.10 times faster than './git -C dest.git -c core.bigfilethreshold=16k unpack-objects <small.pack' in 'origin/master'
1.02 ± 0.07 times faster than './git -C dest.git -c core.bigfilethreshold=16k unpack-objects <small.pack' in 'HEAD~0'
1.10 ± 0.04 times faster than './git -C dest.git -c core.bigfilethreshold=16k unpack-objects <small.pack' in 'HEAD~1'
Helped-by: Ævar Arnfjörð Bjarmason [off-list ref]
Helped-by: Derrick Stolee [off-list ref]
Helped-by: Jiang Xin [off-list ref]
Signed-off-by: Han Xin <redacted>
---
Documentation/config/core.txt | 11 ++++
builtin/unpack-objects.c | 73 +++++++++++++++++++++++-
cache.h | 1 +
config.c | 5 ++
environment.c | 1 +
t/t5590-unpack-non-delta-objects.sh | 87 +++++++++++++++++++++++++++++
6 files changed, 177 insertions(+), 1 deletion(-)
create mode 100755 t/t5590-unpack-non-delta-objects.sh
@@ -424,6 +424,17 @@ be delta compressed, but larger binary media files won't be. + Common unit suffixes of 'k', 'm', or 'g' are supported.+core.bigFileStreamingThreshold::+ Files larger than this will be streamed out to a temporary+ object file while being hashed, which will when be renamed+ in-place to a loose object, particularly if the+ `core.bigFileThreshold' setting dictates that they're always+ written out as loose objects.+++Default is 128 MiB on all platforms.+++Common unit suffixes of 'k', 'm', or 'g' are supported.+ core.excludesFile:: Specifies the pathname to the file that contains patterns to describe paths that are not meant to be tracked, in addition
@@ -331,11 +331,82 @@ static void added_object(unsigned nr, enum object_type type,}}+structinput_zstream_data{+git_zstream*zstream;+unsignedcharbuf[8192];+intstatus;+};++staticconstvoid*feed_input_zstream(conststructinput_stream*in_stream,+unsignedlong*readlen)+{+structinput_zstream_data*data=in_stream->data;+git_zstream*zstream=data->zstream;+void*in=fill(1);++if(!len||data->status==Z_STREAM_END){+*readlen=0;+returnNULL;+}++zstream->next_out=data->buf;+zstream->avail_out=sizeof(data->buf);+zstream->next_in=in;+zstream->avail_in=len;++data->status=git_inflate(zstream,0);+use(len-zstream->avail_in);+*readlen=sizeof(data->buf)-zstream->avail_out;++returndata->buf;+}++staticvoidwrite_stream_blob(unsignednr,unsignedlongsize)+{+git_zstreamzstream;+structinput_zstream_datadata;+structinput_streamin_stream={+.read=feed_input_zstream,+.data=&data,+};++memset(&zstream,0,sizeof(zstream));+memset(&data,0,sizeof(data));+data.zstream=&zstream;+git_inflate_init(&zstream);++if(write_object_file_flags(&in_stream,size,+type_name(OBJ_BLOB),+&obj_list[nr].oid,+HASH_STREAM))+die(_("failed to write object in stream"));++if(zstream.total_out!=size||data.status!=Z_STREAM_END)+die(_("inflate returned %d"),data.status);+git_inflate_end(&zstream);++if(strict){+structblob*blob=lookup_blob(the_repository,&obj_list[nr].oid);+if(blob)+blob->object.flags|=FLAG_WRITTEN;+else+die(_("invalid blob object from stream"));+}+obj_list[nr].obj=NULL;+}+staticvoidunpack_non_delta_entry(enumobject_typetype,unsignedlongsize,unsignednr){-void*buf=get_data(size,dry_run);+void*buf;++/* Write large blob in stream without allocating full buffer. */+if(!dry_run&&type==OBJ_BLOB&&size>big_file_streaming_threshold){+write_stream_blob(nr,size);+return;+}+buf=get_data(size,dry_run);if(!dry_run&&buf)write_object(nr,type,buf,size);else
@@ -0,0 +1,87 @@+#!/bin/sh+#+# Copyright (c) 2021 Han Xin+#++test_description='Test unpack-objects when receive pack'++GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main+exportGIT_TEST_DEFAULT_INITIAL_BRANCH_NAME++../test-lib.sh++prepare_dest(){+test_when_finished"rm -rf dest.git"&&+gitinit--baredest.git&&+git-Cdest.gitconfigcore.bigFileStreamingThreshold$1&&+git-Cdest.gitconfigcore.bigFileThreshold$1+}++test_expect_success"setup repo with big blobs (1.5 MB)"'+test-toolgenrandomfoo1500000>big-blob&&+test_commit--appendfoobig-blob&&+test-toolgenrandombar1500000>big-blob&&+test_commit--appendbarbig-blob&&+(+cd.git&&+findobjects/??-typef|sort+)>expect&&+PACK=$(echomain|gitpack-objects--revstest)+'++test_expect_success'setup env: GIT_ALLOC_LIMIT to 1MB''+GIT_ALLOC_LIMIT=1m&&+exportGIT_ALLOC_LIMIT+'++test_expect_success'fail to unpack-objects: cannot allocate''+prepare_dest2m&&+test_must_failgit-Cdest.gitunpack-objects<test-$PACK.pack2>err&&+grep"fatal: attempting to allocate"err&&+(+cddest.git&&+findobjects/??-typef|sort+)>actual&&+test_file_not_emptyactual&&+!test_cmpexpectactual+'++test_expect_success'unpack big object in stream''+prepare_dest1m&&+git-Cdest.gitunpack-objects<test-$PACK.pack&&+git-Cdest.gitfsck&&+(+cddest.git&&+findobjects/??-typef|sort+)>actual&&+test_cmpexpectactual+'++test_expect_success'unpack big object in stream with existing oids''+prepare_dest1m&&+git-Cdest.gitindex-pack--stdin<test-$PACK.pack&&+(+cddest.git&&+findobjects/??-typef|sort+)>actual&&+test_must_be_emptyactual&&+git-Cdest.gitunpack-objects<test-$PACK.pack&&+git-Cdest.gitfsck&&+(+cddest.git&&+findobjects/??-typef|sort+)>actual&&+test_must_be_emptyactual+'++test_expect_success'unpack-objects dry-run''+prepare_dest1m&&+git-Cdest.gitunpack-objects-n<test-$PACK.pack&&+(+cddest.git&&+findobjects/-typef+)>actual&&+test_must_be_emptyactual+'++test_done
fd = create_tmpfile(&tmp_file, filename.buf);
if (fd < 0) {
if (flags & HASH_SILENT)
- return -1;
+ ret = -1;
else if (errno == EACCES)
- return error(_("insufficient permission for adding an object to repository database %s"), get_object_directory());
+ ret = error(_("insufficient permission for adding an "
+ "object to repository database %s"),
+ get_object_directory());
else
- return error_errno(_("unable to create temporary file"));
+ ret = error_errno(_("unable to create temporary file"));
+ goto cleanup;
}
/* Set it up */
@@ -1930,7 +1933,11 @@ static int write_loose_object(const struct object_id *oid, char *hdr, warning_errno(_("failed utime() on %s"), tmp_file.buf); }- return finalize_object_file(tmp_file.buf, filename.buf);+ ret = finalize_object_file(tmp_file.buf, filename.buf);+cleanup:+ strbuf_release(&filename);+ strbuf_release(&tmp_file);
There was no leak before. Both strbufs are static and both functions
they are passed to (loose_object_path() and create_tmpfile()) reset
them first. So while the allocated memory was not released before,
it was reused.
Not sure if making write_loose_object() allocate and release these
buffers on every call has much of a performance impact. The only
reason I can think of for wanting such a change is to get rid of the
static buffers, to allow the function to be used by concurrent
threads.
So I think either keeping the code as-is or also making the strbufs
non-static would be better (but then discussing a possible
performance impact in the commit message would be nice).
+ return ret;
}
static int freshen_loose_object(const struct object_id *oid)
From: René Scharfe <hidden> Date: 2021-12-17 21:22:25
Am 17.12.21 um 12:26 schrieb Han Xin:
From: Han Xin <redacted>
In dry_run mode, "get_data()" is used to verify the inflation of data,
and the returned buffer will not be used at all and will be freed
immediately. Even in dry_run mode, it is dangerous to allocate a
full-size buffer for a large blob object. Therefore, only allocate a
low memory footprint when calling "get_data()" in dry_run mode.
Clever. Looks good to me.
For some reason I was expecting this patch to have some connection to
one of the earlier ones (perhaps because get_data() was mentioned),
but it is technically independent.
@@ -396,7 +407,7 @@ static void unpack_delta_entry(enum object_type type, unsigned long delta_size,if(base_offset<=0||base_offset>=obj_list[nr].offset)die("offset value out of bound for delta base object");-delta_data=get_data(delta_size);+delta_data=get_data(delta_size,dry_run);if(dry_run||!delta_data){free(delta_data);return;
From: René Scharfe <hidden> Date: 2021-12-17 22:52:43
Am 17.12.21 um 12:26 schrieb Han Xin:
quoted hunk
From: Han Xin <redacted>
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.
This can be improved by feeding data to "stream_loose_object()" in a
stream. The input stream is implemented as an interface.
When streaming a large blob object to "write_loose_object()", we have no
chance to run "write_object_file_prepare()" to calculate the oid in
advance. So we need to handle undetermined oid in a new function called
"stream_loose_object()".
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.
We will reuse "write_object_file_flags()" in "unpack_non_delta_entry()" to
read the entire data contents in stream, so a new flag "HASH_STREAM" is
added. When read in stream, we needn't prepare the "oid" before
"write_loose_object()", only generate the header.
"freshen_packed_object()" or "freshen_loose_object()" will be called
inside "stream_loose_object()" after obtaining the "oid".
Helped-by: Ævar Arnfjörð Bjarmason [off-list ref]
Helped-by: Jiang Xin [off-list ref]
Signed-off-by: Han Xin <redacted>
---
cache.h | 1 +
object-file.c | 92 ++++++++++++++++++++++++++++++++++++++++++++++++++
object-store.h | 5 +++
3 files changed, 98 insertions(+)
@@ -1994,6 +1994,88 @@ static int freshen_packed_object(const struct object_id *oid)return1;}+staticintstream_loose_object(structobject_id*oid,char*hdr,inthdrlen,+conststructinput_stream*in_stream,+unsignedlonglen,time_tmtime,unsignedflags)+{+intfd,ret,err=0,flush=0;+unsignedcharcompressed[4096];+git_zstreamstream;+git_hash_ctxc;+structobject_idparano_oid;+staticstructstrbuftmp_file=STRBUF_INIT;+staticstructstrbuffilename=STRBUF_INIT;
Note these static strbufs.
+ int dirlen;
+
+ /* When oid is not determined, save tmp file to odb path. */
+ strbuf_addf(&filename, "%s/", get_object_directory());
+
+ fd = create_tmpfile(&tmp_file, filename.buf, flags);
+ if (fd < 0) {
+ err = -1;
+ goto cleanup;
+ }
+
+ /* Set it up and write header */
+ setup_stream_and_header(&stream, compressed, sizeof(compressed),
+ &c, hdr, hdrlen);
+
+ /* Then the data itself.. */
+ do {
+ unsigned char *in0 = stream.next_in;
+ if (!stream.avail_in) {
+ const void *in = in_stream->read(in_stream, &stream.avail_in);
+ stream.next_in = (void *)in;
+ in0 = (unsigned char *)in;
+ /* All data has been read. */
+ if (len + hdrlen == stream.total_in + stream.avail_in)
+ flush = Z_FINISH;
+ }
+ ret = git_deflate(&stream, flush);
+ the_hash_algo->update_fn(&c, in0, stream.next_in - in0);
+ if (write_buffer(fd, compressed, stream.next_out - compressed) < 0)
+ die(_("unable to write loose object file"));
+ stream.next_out = compressed;
+ stream.avail_out = sizeof(compressed);
+ } while (ret == Z_OK || ret == Z_BUF_ERROR);
+
+ if (ret != Z_STREAM_END)
+ die(_("unable to deflate new object streamingly (%d)"), ret);
+ ret = git_deflate_end_gently(&stream);
+ if (ret != Z_OK)
+ die(_("deflateEnd on object streamingly failed (%d)"), ret);
+ the_hash_algo->final_oid_fn(¶no_oid, &c);
+
+ close_loose_object(fd);
+
+ oidcpy(oid, ¶no_oid);
+
+ if (freshen_packed_object(oid) || freshen_loose_object(oid)) {
+ unlink_or_warn(tmp_file.buf);
+ goto cleanup;
+ }
+
+ loose_object_path(the_repository, &filename, oid);
+
+ /* We finally know the object path, and create the missing dir. */
+ dirlen = directory_size(filename.buf);
+ if (dirlen) {
+ struct strbuf dir = STRBUF_INIT;
+ strbuf_add(&dir, filename.buf, dirlen - 1);
+
+ if (mkdir_in_gitdir(dir.buf) < 0) {
+ err = -1;
+ goto cleanup;
+ }
+ }
+
+ err = finalize_object_file_with_mtime(tmp_file.buf, filename.buf, mtime, flags);
+cleanup:
+ strbuf_release(&tmp_file);
+ strbuf_release(&filename);
The static strbufs are released here. That combination is strange --
why keep the variable values between calls by making them static, but
throw away the allocated buffers instead of reusing them?
Given that this function is only used for huge objects I think making
the strbufs non-static and releasing them is the best choice here.
quoted hunk
+ return err;
+}
+
int write_object_file_flags(const void *buf, unsigned long len,
const char *type, struct object_id *oid,
unsigned flags)
@@ -2001,6 +2083,16 @@ int write_object_file_flags(const void *buf, unsigned long len, char hdr[MAX_HEADER_LEN]; int hdrlen = sizeof(hdr);+ /* When streaming a large blob object (marked as HASH_STREAM),+ * we have no chance to run "write_object_file_prepare()" to+ * calculate the "oid" in advance. Call "stream_loose_object()"+ * to write loose object in stream.+ */+ if (flags & HASH_STREAM) {+ hdrlen = generate_object_header(hdr, hdrlen, type, len);+ return stream_loose_object(oid, hdr, hdrlen, buf, len, 0, flags);+ }
So stream_loose_object() is called by passing the flag HASH_STREAM to
write_object_file_flags() and passing a struct input_stream via its
buf pointer. That's ... unconventional. Certainly scary. Why not
export stream_loose_object() and call it directly? Demo patch below.
quoted hunk
+
/* Normally if we have it in the pack then we do not bother writing
* it out into .git/objects/??/?{38} file.
*/
@@ -1994,9 +1994,9 @@ static int freshen_packed_object(const struct object_id *oid)return1;}-staticintstream_loose_object(structobject_id*oid,char*hdr,inthdrlen,-conststructinput_stream*in_stream,-unsignedlonglen,time_tmtime,unsignedflags)+intstream_loose_object(structinput_stream*in_stream,unsignedlonglen,+constchar*type,time_tmtime,unsignedflags,+structobject_id*oid){intfd,ret,err=0,flush=0;unsignedcharcompressed[4096];
@@ -2006,6 +2006,10 @@ static int stream_loose_object(struct object_id *oid, char *hdr, int hdrlen,staticstructstrbuftmp_file=STRBUF_INIT;staticstructstrbuffilename=STRBUF_INIT;intdirlen;+charhdr[MAX_HEADER_LEN];+inthdrlen=sizeof(hdr);++hdrlen=generate_object_header(hdr,hdrlen,type,len);/* When oid is not determined, save tmp file to odb path. */strbuf_addf(&filename,"%s/",get_object_directory());
@@ -2083,16 +2087,6 @@ int write_object_file_flags(const void *buf, unsigned long len,charhdr[MAX_HEADER_LEN];inthdrlen=sizeof(hdr);-/* When streaming a large blob object (marked as HASH_STREAM),-*wehavenochancetorun"write_object_file_prepare()"to-*calculatethe"oid"inadvance.Call"stream_loose_object()"-*towritelooseobjectinstream.-*/-if(flags&HASH_STREAM){-hdrlen=generate_object_header(hdr,hdrlen,type,len);-returnstream_loose_object(oid,hdr,hdrlen,buf,len,0,flags);-}-/* Normally if we have it in the pack then we do not bother writing*itoutinto.git/objects/??/?{38}file.*/
Add a convenience function to wrap the xsnprintf() command that
generates loose object headers. This code was copy/pasted in various
parts of the codebase, let's define it in one place and re-use it from
there.
All except one caller of it had a valid "enum object_type" for us,
it's only write_object_file_prepare() which might need to deal with
"git hash-object --literally" and a potential garbage type. Let's have
the primary API use an "enum object_type", and define an *_extended()
function that can take an arbitrary "const char *" for the type.
See [1] for the discussion that prompted this patch, i.e. new code in
object-file.c that wanted to copy/paste the xsnprintf() invocation.
1. https://lore.kernel.org/git/211213.86bl1l9bfz.gmgdl@evledraar.gmail.com/
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
On Fri, Dec 17 2021, Han Xin wrote:
From: Han Xin <redacted>
There are 3 places where "xsnprintf" is used to generate the object
header, and I originally planned to add a fourth in the latter patch.
According to Ævar Arnfjörð Bjarmason’s suggestion, although it's just
one line, it's also code that's very central to git, so reafactor them
into a function which will help later readability.
Helped-by: Ævar Arnfjörð Bjarmason [off-list ref]
Signed-off-by: Han Xin <redacted>
I came up with this after my comment on the earlier round suggesting
to factor out that header formatting. I don't know if this more
thorough approach is worth it or if you'd like to replace your change
with this one, but just posting it here as an RFC.
builtin/index-pack.c | 3 +--
bulk-checkin.c | 4 ++--
cache.h | 21 +++++++++++++++++++++
http-push.c | 2 +-
object-file.c | 14 +++++++++++---
5 files changed, 36 insertions(+), 8 deletions(-)
@@ -220,8 +220,8 @@ static int deflate_to_pack(struct bulk_checkin_state *state,if(seekback==(off_t)-1)returnerror("cannot find the current offset");-header_len=xsnprintf((char*)obuf,sizeof(obuf),"%s %"PRIuMAX,-type_name(type),(uintmax_t)size)+1;+header_len=format_loose_header((char*)obuf,sizeof(obuf),+type,(uintmax_t)size);the_hash_algo->init_fn(&ctx);the_hash_algo->update_fn(&ctx,obuf,header_len);
@@ -363,7 +363,7 @@ static void start_put(struct transfer_request *request)git_zstreamstream;unpacked=read_object_file(&request->obj->oid,&type,&len);-hdrlen=xsnprintf(hdr,sizeof(hdr),"%s %"PRIuMAX,type_name(type),(uintmax_t)len)+1;+hdrlen=format_loose_header(hdr,sizeof(hdr),type,(uintmax_t)len);/* Set it up */git_deflate_init(&stream,zlib_compression_level);