Re: [PATCH v7 1/5] unpack-objects.c: add dry_run mode for get_data()
- From
René Scharfe <l.s.r@web.de>
- Date
- Dec 21, 2021, 14:43 UTC
- Message-ID
- <3f1e3a59-1288-ecff-88b5-53bbd510a6a0@web.de>
- In-Reply-To
- <211221.86bl1arqls.gmgdl@evledraar.gmail.com>
Am 21.12.21 um 15:09 schrieb Ævar Arnfjörð Bjarmason:
Show 45 quoted lines
>
> On Tue, Dec 21 2021, Han Xin wrote:
>
>> From: Han Xin <hanxin.hx@alibaba-inc.com>
>>
>> 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 <zhiyou.jx@alibaba-inc.com>
>> Signed-off-by: Han Xin <hanxin.hx@alibaba-inc.com>
>> ---
>> builtin/unpack-objects.c | 23 +++++++++---
>> t/t5590-unpack-non-delta-objects.sh | 57 +++++++++++++++++++++++++++++
>> 2 files changed, 74 insertions(+), 6 deletions(-)
>> create mode 100755 t/t5590-unpack-non-delta-objects.sh
>>
>> diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c
>> index 4a9466295b..9104eb48da 100644
>> --- a/builtin/unpack-objects.c
>> +++ b/builtin/unpack-objects.c
>> @@ -96,15 +96,21 @@ static void use(int bytes)
>> display_throughput(progress, consumed_bytes);
>> }
>>
>> -static void *get_data(unsigned long size)
>> +static void *get_data(size_t size, int dry_run)
>> {
>> git_zstream stream;
>> - void *buf = xmallocz(size);
>> + size_t bufsize;
>> + void *buf;
>>
>> memset(&stream, 0, sizeof(stream));
>> + if (dry_run && size > 8192)
>> + bufsize = 8192;
>> + else
>> + bufsize = size;
>> + buf = xmallocz(bufsize);
>
> Maybe I'm misunderstanding this, but the commit message says it would be
> dangerous to allocate a very larger buffer, but here we only limit the
> size under "dry_run".This patch reduces the memory usage of dry runs, as its commit message says. The memory usage of one type of actual (non-dry) unpack is reduced by patch 5.
> Removing that "&& size > 8192" makes all the tests pass still, so there > seems to be some missing coverage here in any case.
How would you test that an 8KB buffer is allocated even though a smaller one would suffice? And why? Wasting a few KB shouldn't be noticeable.
René