* [PATCH] read-cache.c: reduce unnecessary cache entry name copying
@ 2022-06-05 13:37 ZheNing Hu via GitGitGadget
2022-06-06 9:48 ` René Scharfe
0 siblings, 1 reply; 3+ messages in thread
From: ZheNing Hu via GitGitGadget @ 2022-06-05 13:37 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, brian m. carlson, Christian Couder, ZheNing Hu,
ZheNing Hu
From: ZheNing Hu <adlternative@gmail.com>
In function create_from_disk, we have already copy the cache
entries name from disk or previous cache entry, we can reduce
unnecessary copy before that.
Signed-off-by: ZheNing Hu <adlternative@gmail.com>
---
read-cache.c: reduce unnecessary cache entry name copying
Index cache entries name are copied twice wrongly, so reduce the first
one to fix it.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1249%2Fadlternative%2Fzh%2Frm-ce-copy-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1249/adlternative/zh/rm-ce-copy-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1249
read-cache.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/read-cache.c b/read-cache.c
index 96ce489c7c5..e61af3a3d4d 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1944,8 +1944,6 @@ static struct cache_entry *create_from_disk(struct mem_pool *ce_mem_pool,
ce->ce_namelen = len;
ce->index = 0;
oidread(&ce->oid, ondisk->data);
- memcpy(ce->name, name, len);
- ce->name[len] = '\0';
if (expand_name_field) {
if (copy_len)
base-commit: ab336e8f1c8009c8b1aab8deb592148e69217085
--
gitgitgadget
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] read-cache.c: reduce unnecessary cache entry name copying
2022-06-05 13:37 [PATCH] read-cache.c: reduce unnecessary cache entry name copying ZheNing Hu via GitGitGadget
@ 2022-06-06 9:48 ` René Scharfe
2022-06-06 17:23 ` Junio C Hamano
0 siblings, 1 reply; 3+ messages in thread
From: René Scharfe @ 2022-06-06 9:48 UTC (permalink / raw)
To: ZheNing Hu via GitGitGadget, git
Cc: Junio C Hamano, brian m. carlson, Christian Couder, ZheNing Hu
Am 05.06.22 um 15:37 schrieb ZheNing Hu via GitGitGadget:
> From: ZheNing Hu <adlternative@gmail.com>
>
> In function create_from_disk, we have already copy the cache
> entries name from disk or previous cache entry, we can reduce
> unnecessary copy before that.
>
> Signed-off-by: ZheNing Hu <adlternative@gmail.com>
> ---
> read-cache.c: reduce unnecessary cache entry name copying
>
> Index cache entries name are copied twice wrongly, so reduce the first
> one to fix it.
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1249%2Fadlternative%2Fzh%2Frm-ce-copy-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1249/adlternative/zh/rm-ce-copy-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1249
>
> read-cache.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/read-cache.c b/read-cache.c
> index 96ce489c7c5..e61af3a3d4d 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -1944,8 +1944,6 @@ static struct cache_entry *create_from_disk(struct mem_pool *ce_mem_pool,
> ce->ce_namelen = len;
> ce->index = 0;
> oidread(&ce->oid, ondisk->data);
> - memcpy(ce->name, name, len);
> - ce->name[len] = '\0';
This removal looks correct to me. The extra copying was added by
575fa8a3ed (read-cache: read data in a hash-independent way, 2019-02-19)
but its commit message doesn't mention it.
>
> if (expand_name_field) {
> if (copy_len)
Some more context lines would help to see the redundancy; here they are:
if (expand_name_field) {
if (copy_len)
memcpy(ce->name, previous_ce->name, copy_len);
memcpy(ce->name + copy_len, name, len + 1 - copy_len);
*ent_size = (name - ((char *)ondisk)) + len + 1 - copy_len;
} else {
memcpy(ce->name, name, len + 1);
*ent_size = ondisk_ce_size(ce);
}
>
> base-commit: ab336e8f1c8009c8b1aab8deb592148e69217085
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] read-cache.c: reduce unnecessary cache entry name copying
2022-06-06 9:48 ` René Scharfe
@ 2022-06-06 17:23 ` Junio C Hamano
0 siblings, 0 replies; 3+ messages in thread
From: Junio C Hamano @ 2022-06-06 17:23 UTC (permalink / raw)
To: René Scharfe
Cc: ZheNing Hu via GitGitGadget, git, brian m. carlson,
Christian Couder, ZheNing Hu
René Scharfe <l.s.r@web.de> writes:
>> diff --git a/read-cache.c b/read-cache.c
>> index 96ce489c7c5..e61af3a3d4d 100644
>> --- a/read-cache.c
>> +++ b/read-cache.c
>> @@ -1944,8 +1944,6 @@ static struct cache_entry *create_from_disk(struct mem_pool *ce_mem_pool,
>> ce->ce_namelen = len;
>> ce->index = 0;
>> oidread(&ce->oid, ondisk->data);
>> - memcpy(ce->name, name, len);
>> - ce->name[len] = '\0';
>
> This removal looks correct to me. The extra copying was added by
> 575fa8a3ed (read-cache: read data in a hash-independent way, 2019-02-19)
> but its commit message doesn't mention it.
I agree with the analysis. When we are prefix-compressing the
entry, name[] may be much shorter than len, so this memcpy() may
potentially be reading beyond the end of the on-disk buffer, so the
original code is not just redundant, but it may simply be wrong.
Thanks, both. Will queue.
>>
>> if (expand_name_field) {
>> if (copy_len)
>
> Some more context lines would help to see the redundancy; here they are:
>
> if (expand_name_field) {
> if (copy_len)
> memcpy(ce->name, previous_ce->name, copy_len);
> memcpy(ce->name + copy_len, name, len + 1 - copy_len);
> *ent_size = (name - ((char *)ondisk)) + len + 1 - copy_len;
> } else {
> memcpy(ce->name, name, len + 1);
> *ent_size = ondisk_ce_size(ce);
> }
>
>>
>> base-commit: ab336e8f1c8009c8b1aab8deb592148e69217085
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-06-06 17:23 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-05 13:37 [PATCH] read-cache.c: reduce unnecessary cache entry name copying ZheNing Hu via GitGitGadget
2022-06-06 9:48 ` René Scharfe
2022-06-06 17:23 ` Junio C Hamano
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).