{"thread":{"id":"56296","subject":"[GSOC] [QUESTION] ref-filter: can %(raw) implement reuse oi.content?","startedAt":"2021-08-16T14:01:45Z","lastAt":"2021-08-25T08:11:59Z","messageCount":14,"participants":["ZheNing Hu","Christian Couder"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"432801","messageId":"CAOLTT8TJ_VBh8mreExXHdCFb1e6hB1-Exsn6OFdo2sZNkVFJkA@mail.gmail.com","threadId":"56296","inReplyTo":null,"subject":"[GSOC] [QUESTION] ref-filter: can %(raw) implement reuse oi.content?","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-08-16T14:00:22Z","receivedAt":"2021-08-16T14:01:45Z","isPatch":false,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"Hi,\n\nIn the implementation of %(raw) atom\n(bd0708c7 ref-filter: add %(raw) atom), we use xmemdupz()\nto copy the content of the object. But if we can reuse the content\nof the object?\n\nSince git cat-file --batch needs to use ref-filter\nas the backend, if the object buffer can be reused correctly here,\nwe can save a lot of copies and improve its performance by about 6%.\n\nTracing back to the source, the object buffer is allocated from\noid_object_info_extended(), but in parse_object_buffer() we may lose\nthe ownership of the buffer (maybe the buffer is eaten), but I browsed the\nsource code of for-each-ref.c or cat-file.c, and I don’t seem to find that the\nbuffers which have been eaten are released by the program.\n\nSo can we reuse it?\n\nThis is what I want to do:\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 93ce2a6ef2..1f6c1daabd 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1443,7 +1443,7 @@ static void grab_sub_body_contents(struct\natom_value *val, int deref, struct exp\n unsigned long buf_size = data->size;\n\n if (atom->u.raw_data.option == RAW_BARE) {\n- v->s = xmemdupz(buf, buf_size);\n+ v->s = buf;\n v->s_size = buf_size;\n } else if (atom->u.raw_data.option == RAW_LENGTH) {\n v->s = xstrfmt(\"%\"PRIuMAX, (uintmax_t)buf_size);\n@@ -1768,8 +1768,6 @@ static int get_object(struct ref_array_item\n*ref, int deref, struct object **obj\n }\n\n grab_common_values(ref->value, deref, oi);\n- if (!eaten)\n- free(oi->content);\n return 0;\n }\n\nThanks.\n--\nZheNing Hu\n"},{"id":"432987","messageId":"CAOLTT8QRrHp4OmCS7-sV481cMPK2nmVuiaQpqzv_cM5T0HKbRw@mail.gmail.com","threadId":"56296","inReplyTo":"CAOLTT8TJ_VBh8mreExXHdCFb1e6hB1-Exsn6OFdo2sZNkVFJkA@mail.gmail.com","subject":"Fwd: [GSOC] [QUESTION] ref-filter: can %(raw) implement reuse oi.content?","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-08-17T14:34:42Z","receivedAt":"2021-08-17T14:35:04Z","isPatch":false,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"Hi,\n\nBecause this email was accidentally sent to everyone’s mail\ninstead of CC everyone, so this email was re-sent again.\n\nIn the implementation of %(raw) atom\n(bd0708c7 ref-filter: add %(raw) atom), we use xmemdupz()\nto copy the content of the object. But if we can reuse the content\nof the object?\n\nSince git cat-file --batch needs to use ref-filter\nas the backend, if the object buffer can be reused correctly here,\nwe can save a lot of copies and improve its performance by about 6%.\n\nTracing back to the source, the object buffer is allocated from\noid_object_info_extended(), but in parse_object_buffer() we may lose\nthe ownership of the buffer (maybe the buffer is eaten), but I browsed the\nsource code of for-each-ref.c or cat-file.c, and I don’t seem to find that the\nbuffers which have been eaten are released by the program.\n\nSo can we reuse it? (Or can we free() it freely?)\n\nThis is what I want to do:\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 93ce2a6ef2..1f6c1daabd 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1443,7 +1443,7 @@ static void grab_sub_body_contents(struct\natom_value *val, int deref, struct exp\n unsigned long buf_size = data->size;\n\n if (atom->u.raw_data.option == RAW_BARE) {\n- v->s = xmemdupz(buf, buf_size);\n+ v->s = buf;\n v->s_size = buf_size;\n } else if (atom->u.raw_data.option == RAW_LENGTH) {\n v->s = xstrfmt(\"%\"PRIuMAX, (uintmax_t)buf_size);\n@@ -1768,8 +1768,6 @@ static int get_object(struct ref_array_item\n*ref, int deref, struct object **obj\n }\n\n grab_common_values(ref->value, deref, oi);\n- if (!eaten)\n- free(oi->content);\n return 0;\n }\n\nThanks.\n--\nZheNing Hu\n"},{"id":"432989","messageId":"CAP8UFD0AvakWdpJKBDrEZstVvFOGdjVUkWsBXn7geiUdVzAZGQ@mail.gmail.com","threadId":"56296","inReplyTo":"CAOLTT8TJ_VBh8mreExXHdCFb1e6hB1-Exsn6OFdo2sZNkVFJkA@mail.gmail.com","subject":"Re: [GSOC] [QUESTION] ref-filter: can %(raw) implement reuse oi.content?","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2021-08-17T16:09:52Z","receivedAt":"2021-08-17T16:10:08Z","isPatch":false,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Hi,\n\nOn Mon, Aug 16, 2021 at 4:00 PM ZheNing Hu wrote:\n>\n> Hi,\n>\n> In the implementation of %(raw) atom\n> (bd0708c7 ref-filter: add %(raw) atom), we use xmemdupz()\n> to copy the content of the object. But if we can reuse the content\n> of the object?\n>\n> Since git cat-file --batch needs to use ref-filter\n> as the backend, if the object buffer can be reused correctly here,\n> we can save a lot of copies and improve its performance by about 6%.\n\nYeah, that would be great.\n\n> Tracing back to the source, the object buffer is allocated from\n> oid_object_info_extended(), but in parse_object_buffer() we may lose\n> the ownership of the buffer (maybe the buffer is eaten), but I browsed the\n> source code of for-each-ref.c or cat-file.c, and I don’t seem to find that the\n> buffers which have been eaten are released by the program.\n>\n> So can we reuse it?\n>\n> This is what I want to do:\n>\n> diff --git a/ref-filter.c b/ref-filter.c\n> index 93ce2a6ef2..1f6c1daabd 100644\n> --- a/ref-filter.c\n> +++ b/ref-filter.c\n> @@ -1443,7 +1443,7 @@ static void grab_sub_body_contents(struct\n> atom_value *val, int deref, struct exp\n> unsigned long buf_size = data->size;\n>\n> if (atom->u.raw_data.option == RAW_BARE) {\n> - v->s = xmemdupz(buf, buf_size);\n> + v->s = buf;\n\nIt seems to me that this could work only if 'buf' isn't freed. Have\nyou checked that? Did we leak 'buf' before this patch? Otherwise when\nare we freeing it?\n\n> v->s_size = buf_size;\n> } else if (atom->u.raw_data.option == RAW_LENGTH) {\n> v->s = xstrfmt(\"%\"PRIuMAX, (uintmax_t)buf_size);\n> @@ -1768,8 +1768,6 @@ static int get_object(struct ref_array_item\n> *ref, int deref, struct object **obj\n> }\n>\n> grab_common_values(ref->value, deref, oi);\n> - if (!eaten)\n> - free(oi->content);\n\nThis change might not be needed. 'oi->content' seems to come from the\n'buf' above, either directly or through a copy, but if we can indeed\nown the 'buf' completely, then we should be able to dispose of it how\nwe want.\n\n> return 0;\n> }\n"},{"id":"433040","messageId":"CAOLTT8TB0HUBnYc1U7o36wBp=qxKDOLA6D9K19niNt_y_W==ag@mail.gmail.com","threadId":"56296","inReplyTo":"CAP8UFD0AvakWdpJKBDrEZstVvFOGdjVUkWsBXn7geiUdVzAZGQ@mail.gmail.com","subject":"Re: [GSOC] [QUESTION] ref-filter: can %(raw) implement reuse oi.content?","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-08-18T04:51:13Z","receivedAt":"2021-08-18T04:51:27Z","isPatch":false,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"Christian Couder 于2021年8月18日周三 上午12:10写道：\n>\n> Hi,\n>\n> On Mon, Aug 16, 2021 at 4:00 PM ZheNing Hu wrote:\n> >\n> > Hi,\n> >\n> > In the implementation of %(raw) atom\n> > (bd0708c7 ref-filter: add %(raw) atom), we use xmemdupz()\n> > to copy the content of the object. But if we can reuse the content\n> > of the object?\n> >\n> > Since git cat-file --batch needs to use ref-filter\n> > as the backend, if the object buffer can be reused correctly here,\n> > we can save a lot of copies and improve its performance by about 6%.\n>\n> Yeah, that would be great.\n>\n> > Tracing back to the source, the object buffer is allocated from\n> > oid_object_info_extended(), but in parse_object_buffer() we may lose\n> > the ownership of the buffer (maybe the buffer is eaten), but I browsed the\n> > source code of for-each-ref.c or cat-file.c, and I don’t seem to find that the\n> > buffers which have been eaten are released by the program.\n> >\n> > So can we reuse it?\n> >\n> > This is what I want to do:\n> >\n> > diff --git a/ref-filter.c b/ref-filter.c\n> > index 93ce2a6ef2..1f6c1daabd 100644\n> > --- a/ref-filter.c\n> > +++ b/ref-filter.c\n> > @@ -1443,7 +1443,7 @@ static void grab_sub_body_contents(struct\n> > atom_value *val, int deref, struct exp\n> > unsigned long buf_size = data->size;\n> >\n> > if (atom->u.raw_data.option == RAW_BARE) {\n> > - v->s = xmemdupz(buf, buf_size);\n> > + v->s = buf;\n>\n> It seems to me that this could work only if 'buf' isn't freed. Have\n> you checked that? Did we leak 'buf' before this patch? Otherwise when\n> are we freeing it?\n>\nThis is how I use gdb find out if this buffer have been freed:\n\nBreakpoint 1, get_object (ref=0x5555559ad7a0, deref=1,\nobj=0x7fffffffc930, oi=0x5555559a21e0 , err=0x7fffffffcb80)\nat ref-filter.c:1744\n1744 {\n(gdb) n\n1746 int eaten = 1;\n(gdb)\n1747 if (oi->info.contentp) {\n(gdb)\n1749 oi->info.sizep = &oi->size;\n(gdb)\n1750 oi->info.typep = &oi->type;\n(gdb)\n1752 if (oid_object_info_extended(the_repository, &oi->oid,\n&oi->info,\n(gdb)\n1756 if (oi->info.disk_sizep && oi->disk_size < 0)\n(gdb)\n1759 if (oi->info.contentp) {\n(gdb) p oi->info.contentp\n$1 = (void **) 0x5555559a2240 \n(gdb) p oi->content\n$2 = (void *) 0x5555559afc90\n(gdb) b free if $rdi == 0x5555559afc90\nBreakpoint 2 at 0x7ffff7e1f980\n(gdb) d 1\n(gdb) c\nContinuing.\ntree 2025c6f1d02348585487ab5597d77151f158325f\nparent 532c266f73e11df407a8939135f2722c003b7539\nauthor ZheNing Hu 1628909038 +0800\ncommitter ZheNing Hu 1628909038 +0800\n\ntest2\n\ntree 2025c6f1d02348585487ab5597d77151f158325f\nparent 532c266f73e11df407a8939135f2722c003b7539\nauthor ZheNing Hu 1628909038 +0800\ncommitter ZheNing Hu 1628909038 +0800\n\ntest2\n\n[Inferior 1 (process 74283) exited normally]\n\nWe didn't stop at the breakpoint, this means that we did not free() the memory.\nOr will this be used as a caching mechanism? I dunno...\n\n> > v->s_size = buf_size;\n> > } else if (atom->u.raw_data.option == RAW_LENGTH) {\n> > v->s = xstrfmt(\"%\"PRIuMAX, (uintmax_t)buf_size);\n> > @@ -1768,8 +1768,6 @@ static int get_object(struct ref_array_item\n> > *ref, int deref, struct object **obj\n> > }\n> >\n> > grab_common_values(ref->value, deref, oi);\n> > - if (!eaten)\n> > - free(oi->content);\n>\n> This change might not be needed. 'oi->content' seems to come from the\n> 'buf' above, either directly or through a copy, but if we can indeed\n> own the 'buf' completely, then we should be able to dispose of it how\n> we want.\n>\n\nSince we will release the `v->s` (now is the buf) in free_array_item(), if we\ncontinue to call free() in get_object(), it may cause double-free.\n\n> > return 0;\n> > }\n\nThanks.\n--\nZheNing Hu\n"},{"id":"433048","messageId":"CAP8UFD15vZ4B3dxPamaxqySZgLwffHU1Rx21bHRLY9zndjvAew@mail.gmail.com","threadId":"56296","inReplyTo":"CAOLTT8TB0HUBnYc1U7o36wBp=qxKDOLA6D9K19niNt_y_W==ag@mail.gmail.com","subject":"Re: [GSOC] [QUESTION] ref-filter: can %(raw) implement reuse oi.content?","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2021-08-18T08:53:49Z","receivedAt":"2021-08-18T08:54:04Z","isPatch":false,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Wed, Aug 18, 2021 at 6:51 AM ZheNing Hu wrote:\n>\n> Christian Couder 于2021年8月18日周三 上午12:10写道：\n> >\n> > Hi,\n> >\n> > On Mon, Aug 16, 2021 at 4:00 PM ZheNing Hu wrote:\n> > >\n> > > Hi,\n> > >\n> > > In the implementation of %(raw) atom\n> > > (bd0708c7 ref-filter: add %(raw) atom), we use xmemdupz()\n> > > to copy the content of the object. But if we can reuse the content\n> > > of the object?\n> > >\n> > > Since git cat-file --batch needs to use ref-filter\n> > > as the backend, if the object buffer can be reused correctly here,\n> > > we can save a lot of copies and improve its performance by about 6%.\n> >\n> > Yeah, that would be great.\n> >\n> > > Tracing back to the source, the object buffer is allocated from\n> > > oid_object_info_extended(), but in parse_object_buffer() we may lose\n> > > the ownership of the buffer (maybe the buffer is eaten), but I browsed the\n> > > source code of for-each-ref.c or cat-file.c, and I don’t seem to find that the\n> > > buffers which have been eaten are released by the program.\n> > >\n> > > So can we reuse it?\n> > >\n> > > This is what I want to do:\n> > >\n> > > diff --git a/ref-filter.c b/ref-filter.c\n> > > index 93ce2a6ef2..1f6c1daabd 100644\n> > > --- a/ref-filter.c\n> > > +++ b/ref-filter.c\n> > > @@ -1443,7 +1443,7 @@ static void grab_sub_body_contents(struct\n> > > atom_value *val, int deref, struct exp\n> > > unsigned long buf_size = data->size;\n> > >\n> > > if (atom->u.raw_data.option == RAW_BARE) {\n> > > - v->s = xmemdupz(buf, buf_size);\n> > > + v->s = buf;\n> >\n> > It seems to me that this could work only if 'buf' isn't freed. Have\n> > you checked that? Did we leak 'buf' before this patch? Otherwise when\n> > are we freeing it?\n> >\n> This is how I use gdb find out if this buffer have been freed:\n\nI was asking about 'buf' before the patch.\n\nBefore the patch, we were doing:\n\nv->s = xmemdupz(buf, buf_size);\n\nwhich means that in v->s there is a copy of 'buf', not 'buf' itself.\nSo I was wondering if 'buf' was freed then, not the copy in in v->s.\n"},{"id":"433051","messageId":"CAOLTT8SjLPXT7ows-MZQLmDwzPN5pUrqj26+PHVbTevkBn3Tug@mail.gmail.com","threadId":"56296","inReplyTo":"CAP8UFD15vZ4B3dxPamaxqySZgLwffHU1Rx21bHRLY9zndjvAew@mail.gmail.com","subject":"Re: [GSOC] [QUESTION] ref-filter: can %(raw) implement reuse oi.content?","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-08-18T09:07:26Z","receivedAt":"2021-08-18T09:07:44Z","isPatch":false,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"Christian Couder 于2021年8月18日周三 下午4:54写道：\n>\n> On Wed, Aug 18, 2021 at 6:51 AM ZheNing Hu wrote:\n> >\n> > Christian Couder 于2021年8月18日周三 上午12:10写道：\n> > >\n> > > Hi,\n> > >\n> > > On Mon, Aug 16, 2021 at 4:00 PM ZheNing Hu wrote:\n> > > >\n> > > > Hi,\n> > > >\n> > > > In the implementation of %(raw) atom\n> > > > (bd0708c7 ref-filter: add %(raw) atom), we use xmemdupz()\n> > > > to copy the content of the object. But if we can reuse the content\n> > > > of the object?\n> > > >\n> > > > Since git cat-file --batch needs to use ref-filter\n> > > > as the backend, if the object buffer can be reused correctly here,\n> > > > we can save a lot of copies and improve its performance by about 6%.\n> > >\n> > > Yeah, that would be great.\n> > >\n> > > > Tracing back to the source, the object buffer is allocated from\n> > > > oid_object_info_extended(), but in parse_object_buffer() we may lose\n> > > > the ownership of the buffer (maybe the buffer is eaten), but I browsed the\n> > > > source code of for-each-ref.c or cat-file.c, and I don’t seem to find that the\n> > > > buffers which have been eaten are released by the program.\n> > > >\n> > > > So can we reuse it?\n> > > >\n> > > > This is what I want to do:\n> > > >\n> > > > diff --git a/ref-filter.c b/ref-filter.c\n> > > > index 93ce2a6ef2..1f6c1daabd 100644\n> > > > --- a/ref-filter.c\n> > > > +++ b/ref-filter.c\n> > > > @@ -1443,7 +1443,7 @@ static void grab_sub_body_contents(struct\n> > > > atom_value *val, int deref, struct exp\n> > > > unsigned long buf_size = data->size;\n> > > >\n> > > > if (atom->u.raw_data.option == RAW_BARE) {\n> > > > - v->s = xmemdupz(buf, buf_size);\n> > > > + v->s = buf;\n> > >\n> > > It seems to me that this could work only if 'buf' isn't freed. Have\n> > > you checked that? Did we leak 'buf' before this patch? Otherwise when\n> > > are we freeing it?\n> > >\n> > This is how I use gdb find out if this buffer have been freed:\n>\n> I was asking about 'buf' before the patch.\n>\n> Before the patch, we were doing:\n>\n> v->s = xmemdupz(buf, buf_size);\n>\n> which means that in v->s there is a copy of 'buf', not 'buf' itself.\n> So I was wondering if 'buf' was freed then, not the copy in in v->s.\n\nI think the 'buf' is not freed in ref-filter logic.\n\nBut one thing worth noting is:\n\nparse_object_buffer() may take this buffer away, and store it in\ntree->buffer or use set_commit_buffer() to store it in commit_slab.\n\nSo in theory, as long as we don’t use parse_object_buffer(), this\ndynamically allocated memory can be used freely for us. If we use\nparse_object_buffer() and free the buffer, there seems to be a risk,\nbut it does not appear so far.\n\n--\nZheNing Hu\n"},{"id":"433064","messageId":"CAOLTT8RY213BMTq+wx8yS=0QjY55L1BnCgPHQph1uos2oX03gw@mail.gmail.com","threadId":"56296","inReplyTo":"CAOLTT8SjLPXT7ows-MZQLmDwzPN5pUrqj26+PHVbTevkBn3Tug@mail.gmail.com","subject":"Re: [GSOC] [QUESTION] ref-filter: can %(raw) implement reuse oi.content?","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-08-18T11:11:22Z","receivedAt":"2021-08-18T11:11:37Z","isPatch":false,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"ZheNing Hu 于2021年8月18日周三 下午5:07写道：\n> I think the 'buf' is not freed in ref-filter logic.\n>\n> But one thing worth noting is:\n>\n> parse_object_buffer() may take this buffer away, and store it in\n> tree->buffer or use set_commit_buffer() to store it in commit_slab.\n>\n> So in theory, as long as we don’t use parse_object_buffer(), this\n> dynamically allocated memory can be used freely for us. If we use\n> parse_object_buffer() and free the buffer, there seems to be a risk,\n> but it does not appear so far.\n>\n\nWhen parse_object_buffer() hints that we don’t free the buffer when\neaten == 1, I have a better idea, If the buffer is \"eaten\", we copy it,\notherwise we use the originally allocated space.\n\n-static void grab_sub_body_contents(struct atom_value *val, int deref,\nstruct expand_data *data)\n+static void grab_sub_body_contents(struct atom_value *val, int deref,\nstruct expand_data *data, int eaten)\n {\n int i;\n const char *subpos = NULL, *bodypos = NULL, *sigpos = NULL;\n@@ -1499,7 +1499,10 @@ static void grab_sub_body_contents(struct\natom_value *val, int deref, struct exp\n unsigned long buf_size = data->size;\n\n if (atom->u.raw_data.option == RAW_BARE) {\n- v->s = buf;\n+ if (eaten)\n+ v->s = xmemdupz(buf, buf_size);\n+ else\n+ v->s = buf;\n v->s_size = buf_size;\n } else if (atom->u.raw_data.option == RAW_LENGTH) {\n v->s = xstrfmt_len(&v->s_size,\n\"%\"PRIuMAX, (uintmax_t)buf_size);\n\nAs parse_object_buffer() does internally: the buffer of commit/tree objects\nneeds to be copied, but blob/tag not. You said that the number of commits\nis generally significantly greater than the others. It seems that we cannot\nmake full use of this idea. But remember the \"skip_parse_buffer\" before?\nIf we skip the parse_object_buffer(), this buffer is also \"!eaten\".\n\nIn other words, those default paths of git cat-file --batch are included in it!\nSo from the perspective of performance regression, this patch will be\nbeneficial.\n\nThanks.\n--\nZheNing Hu\n"},{"id":"433097","messageId":"CAOLTT8Qx3=C=MwRmKbrp=G5T_rQVcaLbZfzzO60m7P-_k1qh8A@mail.gmail.com","threadId":"56296","inReplyTo":"CAOLTT8RY213BMTq+wx8yS=0QjY55L1BnCgPHQph1uos2oX03gw@mail.gmail.com","subject":"Re: [GSOC] [QUESTION] ref-filter: can %(raw) implement reuse oi.content?","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-08-19T01:39:33Z","receivedAt":"2021-08-19T01:39:47Z","isPatch":false,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"Hi, Christian and Hariom,\n\nI want to use this patch series as the temporary final version of GSOC project:\n\nhttps://github.com/adlternative/git/commits/cat-file-reuse-ref-filter-logic\n\nDue to the branch ref-filter-opt-code-logic or branch\nref-filter-opt-perf patch series\ntemporarily unable to reflect its optimization to git cat-file\n--batch. Therefore, using\nbranch cat-file-reuse-ref-filter-logic is the most effective now.\n\nThis is the final performance regression test result:\nTest upstream/master this\ntree\n------------------------------------------------------------------------------------\n1006.2: cat-file --batch-check 0.06(0.06+0.00)\n0.08(0.07+0.00) +33.3%\n1006.3: cat-file --batch-check with atoms 0.06(0.04+0.01)\n0.06(0.06+0.00) +0.0%\n1006.4: cat-file --batch 0.49(0.47+0.02)\n0.48(0.47+0.01) -2.0%\n1006.5: cat-file --batch with atoms 0.48(0.44+0.03)\n0.47(0.46+0.01) -2.1%\n\ngit cat-file --batch has a performance improvement of about 2%.\ngit cat-file --batch-check still has a performance gap of 33.3%.\n\nThe performance degradation of git cat-file --batch-check is actually\nnot very big.\n\nupstream/master (225bc32a98):\n\n$ hyperfine --warmup=10 \"~/git/bin-wrappers/git cat-file\n--batch-check --batch-all-objects\"\nBenchmark #1: ~/git/bin-wrappers/git cat-file --batch-check --batch-all-objects\n Time (mean ± σ): 596.2 ms ± 5.7 ms [User: 563.0 ms, System: 32.5 ms]\n Range (min … max): 586.9 ms … 607.9 ms 10 runs\n\ncat-file-reuse-ref-filter-logic (709a0c5c12):\n\n$ hyperfine --warmup=10 \"~/git/bin-wrappers/git cat-file\n--batch-check --batch-all-objects\"\nBenchmark #1: ~/git/bin-wrappers/git cat-file --batch-check --batch-all-objects\n Time (mean ± σ): 601.3 ms ± 5.8 ms [User: 566.9 ms, System: 33.9 ms]\n Range (min … max): 596.7 ms … 613.3 ms 10 runs\n\nThe execution time of git cat-file --batch-check is only a few\nmilliseconds away.\n\nBut look at the execution time changes of git cat-file --batch:\n\nupstream/master (225bc32a98):\n\n$ time ~/git/bin-wrappers/git cat-file --batch --batch-all-objects\n>/dev/null\n/home/adl/git/bin-wrappers/git cat-file --batch --batch-all-objects >\n 24.61s user 0.30s system 99% cpu 24.908 total\n\ncat-file-reuse-ref-filter-logic (709a0c5c12):\n\n$ time ~/git/bin-wrappers/git cat-file --batch --batch-all-objects >/dev/null\ncat-file --batch --batch-all-objects > /dev/null 25.10s user 0.30s\nsystem 99% cpu 25.417 total\n\nThe execution time has been reduced by nearly 0.5 seconds. Intuition\ntells me that the performance improvement of git cat-file --batch will be\nmore important.\n\nIn fact, git cat-file origin code directly adds the obtained object data\nto the output buffer; But after using ref-filter logic, it needs to copy\nthe object data to the intermediate data (atom_value), and finally\nto the output buffer. At present, we cannot easily eliminate intermediate\ndata, because git for-each-ref --sort has a lot of dependence on it,\nbut we can reduce the overhead of copying or allocating memory as\nmuch as possible.\n\nI had an idea that I didn't implement before: partial data delayed evaluation.\nOr to be more specific, waiting until the data is about to be added to\nthe output\nbuffer, form specific output content, this may be a way to bypass the\nintermediate\ndata.\n\nTo be optimistic, I think this patch can be merged with the current\nperformance of\ngit cat-file --batch. Of course, this still needs more suggestions\nfrom reviewers.\n\nThanks.\n--\nZheNing Hu\n"},{"id":"433240","messageId":"CAP8UFD0aDrqHxNp2ztecmZR49gs=SVJTN1NUzJvz6+ewpY-_wA@mail.gmail.com","threadId":"56296","inReplyTo":"CAOLTT8RY213BMTq+wx8yS=0QjY55L1BnCgPHQph1uos2oX03gw@mail.gmail.com","subject":"Re: [GSOC] [QUESTION] ref-filter: can %(raw) implement reuse oi.content?","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2021-08-20T15:58:07Z","receivedAt":"2021-08-20T15:58:22Z","isPatch":false,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Wed, Aug 18, 2021 at 1:11 PM ZheNing Hu wrote:\n>\n> ZheNing Hu 于2021年8月18日周三 下午5:07写道：\n> > I think the 'buf' is not freed in ref-filter logic.\n> >\n> > But one thing worth noting is:\n> >\n> > parse_object_buffer() may take this buffer away, and store it in\n> > tree->buffer or use set_commit_buffer() to store it in commit_slab.\n> >\n> > So in theory, as long as we don’t use parse_object_buffer(), this\n> > dynamically allocated memory can be used freely for us. If we use\n> > parse_object_buffer() and free the buffer, there seems to be a risk,\n> > but it does not appear so far.\n> >\n>\n> When parse_object_buffer() hints that we don’t free the buffer when\n> eaten == 1, I have a better idea, If the buffer is \"eaten\", we copy it,\n> otherwise we use the originally allocated space.\n>\n> -static void grab_sub_body_contents(struct atom_value *val, int deref,\n> struct expand_data *data)\n> +static void grab_sub_body_contents(struct atom_value *val, int deref,\n> struct expand_data *data, int eaten)\n> {\n> int i;\n> const char *subpos = NULL, *bodypos = NULL, *sigpos = NULL;\n> @@ -1499,7 +1499,10 @@ static void grab_sub_body_contents(struct\n> atom_value *val, int deref, struct exp\n> unsigned long buf_size = data->size;\n>\n> if (atom->u.raw_data.option == RAW_BARE) {\n> - v->s = buf;\n> + if (eaten)\n> + v->s = xmemdupz(buf, buf_size);\n\nCan the 'buf' already be eaten at this point? I thought that it could\nbe eaten only through v->s so after this code.\n\n> + else\n> + v->s = buf;\n> v->s_size = buf_size;\n> } else if (atom->u.raw_data.option == RAW_LENGTH) {\n> v->s = xstrfmt_len(&v->s_size,\n> \"%\"PRIuMAX, (uintmax_t)buf_size);\n>\n> As parse_object_buffer() does internally: the buffer of commit/tree objects\n> needs to be copied, but blob/tag not. You said that the number of commits\n> is generally significantly greater than the others. It seems that we cannot\n> make full use of this idea. But remember the \"skip_parse_buffer\" before?\n> If we skip the parse_object_buffer(), this buffer is also \"!eaten\".\n>\n> In other words, those default paths of git cat-file --batch are included in it!\n> So from the perspective of performance regression, this patch will be\n> beneficial.\n\nYeah, it seems that we can benefit from something like this, but it'd\nbe nice if things were clarified a bit more.\n"},{"id":"433242","messageId":"CAP8UFD0WB7FYtp9aX4qz5BmLiNz1S5PA1U8-cB8b9zRqdZHOjw@mail.gmail.com","threadId":"56296","inReplyTo":"CAOLTT8Qx3=C=MwRmKbrp=G5T_rQVcaLbZfzzO60m7P-_k1qh8A@mail.gmail.com","subject":"Re: [GSOC] [QUESTION] ref-filter: can %(raw) implement reuse oi.content?","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2021-08-20T16:13:19Z","receivedAt":"2021-08-20T16:13:45Z","isPatch":false,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Hi ZheNing,\n\nOn Thu, Aug 19, 2021 at 3:39 AM ZheNing Hu wrote:\n>\n> Hi, Christian and Hariom,\n>\n> I want to use this patch series as the temporary final version of GSOC project:\n>\n> https://github.com/adlternative/git/commits/cat-file-reuse-ref-filter-logic\n\nI am still not very happy with the last patch in the series,but it can\nbe improved later.\n\n> Due to the branch ref-filter-opt-code-logic or branch\n> ref-filter-opt-perf patch series\n> temporarily unable to reflect its optimization to git cat-file\n> --batch. Therefore, using\n> branch cat-file-reuse-ref-filter-logic is the most effective now.\n>\n> This is the final performance regression test result:\n> Test upstream/master this\n> tree\n> ------------------------------------------------------------------------------------\n> 1006.2: cat-file --batch-check 0.06(0.06+0.00)\n> 0.08(0.07+0.00) +33.3%\n> 1006.3: cat-file --batch-check with atoms 0.06(0.04+0.01)\n> 0.06(0.06+0.00) +0.0%\n> 1006.4: cat-file --batch 0.49(0.47+0.02)\n> 0.48(0.47+0.01) -2.0%\n> 1006.5: cat-file --batch with atoms 0.48(0.44+0.03)\n> 0.47(0.46+0.01) -2.1%\n>\n> git cat-file --batch has a performance improvement of about 2%.\n> git cat-file --batch-check still has a performance gap of 33.3%.\n>\n> The performance degradation of git cat-file --batch-check is actually\n> not very big.\n>\n> upstream/master (225bc32a98):\n>\n> $ hyperfine --warmup=10 \"~/git/bin-wrappers/git cat-file\n> --batch-check --batch-all-objects\"\n> Benchmark #1: ~/git/bin-wrappers/git cat-file --batch-check --batch-all-objects\n> Time (mean ± σ): 596.2 ms ± 5.7 ms [User: 563.0 ms, System: 32.5 ms]\n> Range (min … max): 586.9 ms … 607.9 ms 10 runs\n>\n> cat-file-reuse-ref-filter-logic (709a0c5c12):\n>\n> $ hyperfine --warmup=10 \"~/git/bin-wrappers/git cat-file\n> --batch-check --batch-all-objects\"\n> Benchmark #1: ~/git/bin-wrappers/git cat-file --batch-check --batch-all-objects\n> Time (mean ± σ): 601.3 ms ± 5.8 ms [User: 566.9 ms, System: 33.9 ms]\n> Range (min … max): 596.7 ms … 613.3 ms 10 runs\n>\n> The execution time of git cat-file --batch-check is only a few\n> milliseconds away.\n\nYeah, it looks like less than 1% overhead.\n\nGreat work!\n\n> But look at the execution time changes of git cat-file --batch:\n>\n> upstream/master (225bc32a98):\n>\n> $ time ~/git/bin-wrappers/git cat-file --batch --batch-all-objects\n> >/dev/null\n> /home/adl/git/bin-wrappers/git cat-file --batch --batch-all-objects >\n> 24.61s user 0.30s system 99% cpu 24.908 total\n>\n> cat-file-reuse-ref-filter-logic (709a0c5c12):\n>\n> $ time ~/git/bin-wrappers/git cat-file --batch --batch-all-objects >/dev/null\n> cat-file --batch --batch-all-objects > /dev/null 25.10s user 0.30s\n> system 99% cpu 25.417 total\n>\n> The execution time has been reduced by nearly 0.5 seconds.\n\nIt looks like it has increased by 0.5s, not been reduced.\n\n> Intuition\n> tells me that the performance improvement of git cat-file --batch will be\n> more important.\n>\n> In fact, git cat-file origin code directly adds the obtained object data\n> to the output buffer; But after using ref-filter logic, it needs to copy\n> the object data to the intermediate data (atom_value), and finally\n> to the output buffer. At present, we cannot easily eliminate intermediate\n> data, because git for-each-ref --sort has a lot of dependence on it,\n> but we can reduce the overhead of copying or allocating memory as\n> much as possible.\n\nOk.\n\n> I had an idea that I didn't implement before: partial data delayed evaluation.\n> Or to be more specific, waiting until the data is about to be added to\n> the output\n> buffer, form specific output content, this may be a way to bypass the\n> intermediate\n> data.\n\nYeah, that might be a good idea.\n"},{"id":"433295","messageId":"CAOLTT8St0PGPsw6zki2fwO3iVG_4W=s6GEqZfp3cmH=xngcO2A@mail.gmail.com","threadId":"56296","inReplyTo":"CAP8UFD0aDrqHxNp2ztecmZR49gs=SVJTN1NUzJvz6+ewpY-_wA@mail.gmail.com","subject":"Re: [GSOC] [QUESTION] ref-filter: can %(raw) implement reuse oi.content?","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-08-21T02:16:02Z","receivedAt":"2021-08-21T02:16:15Z","isPatch":false,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"Christian Couder 于2021年8月20日周五 下午11:58写道：\n>\n> On Wed, Aug 18, 2021 at 1:11 PM ZheNing Hu wrote:\n> >\n> > ZheNing Hu 于2021年8月18日周三 下午5:07写道：\n> > > I think the 'buf' is not freed in ref-filter logic.\n> > >\n> > > But one thing worth noting is:\n> > >\n> > > parse_object_buffer() may take this buffer away, and store it in\n> > > tree->buffer or use set_commit_buffer() to store it in commit_slab.\n> > >\n> > > So in theory, as long as we don’t use parse_object_buffer(), this\n> > > dynamically allocated memory can be used freely for us. If we use\n> > > parse_object_buffer() and free the buffer, there seems to be a risk,\n> > > but it does not appear so far.\n> > >\n> >\n> > When parse_object_buffer() hints that we don’t free the buffer when\n> > eaten == 1, I have a better idea, If the buffer is \"eaten\", we copy it,\n> > otherwise we use the originally allocated space.\n> >\n> > -static void grab_sub_body_contents(struct atom_value *val, int deref,\n> > struct expand_data *data)\n> > +static void grab_sub_body_contents(struct atom_value *val, int deref,\n> > struct expand_data *data, int eaten)\n> > {\n> > int i;\n> > const char *subpos = NULL, *bodypos = NULL, *sigpos = NULL;\n> > @@ -1499,7 +1499,10 @@ static void grab_sub_body_contents(struct\n> > atom_value *val, int deref, struct exp\n> > unsigned long buf_size = data->size;\n> >\n> > if (atom->u.raw_data.option == RAW_BARE) {\n> > - v->s = buf;\n> > + if (eaten)\n> > + v->s = xmemdupz(buf, buf_size);\n>\n> Can the 'buf' already be eaten at this point? I thought that it could\n> be eaten only through v->s so after this code.\n>\n\nI think where the 'buf' is eaten is parse_object_buffer(). set_commit_buffer()\nor parse_tree_buffer() cache the 'buf'. 'eaten' means we don't need to free it\nmanually. In grab_sub_body_contents() we can indeed write the address of\n'buf' to v->s, but what I worry about is that before we completely write v->s to\nthe output buffer through append_atom(), will this buf be freed in\nsomewhere else?\n\n> > + else\n> > + v->s = buf;\n> > v->s_size = buf_size;\n> > } else if (atom->u.raw_data.option == RAW_LENGTH) {\n> > v->s = xstrfmt_len(&v->s_size,\n> > \"%\"PRIuMAX, (uintmax_t)buf_size);\n> >\n> > As parse_object_buffer() does internally: the buffer of commit/tree objects\n> > needs to be copied, but blob/tag not. You said that the number of commits\n> > is generally significantly greater than the others. It seems that we cannot\n> > make full use of this idea. But remember the \"skip_parse_buffer\" before?\n> > If we skip the parse_object_buffer(), this buffer is also \"!eaten\".\n> >\n> > In other words, those default paths of git cat-file --batch are included in it!\n> > So from the perspective of performance regression, this patch will be\n> > beneficial.\n>\n> Yeah, it seems that we can benefit from something like this, but it'd\n\nOnly when the data allocated to us is dynamically allocated and we do have\nthe ownership of it, we can benefit by reducing copies and allocate\nmemory again.\n\n> be nice if things were clarified a bit more.\n\nOK. In get_object(), eaten means that the oi->content we obtained is cached\nby git in parse_object_buffer(). This means that we don't need to free this buf\nmanually. In 28511adfa5 ([GSOC] ref-filter: skip parse_object_buffer()), we skip\nparse_buffer() when using some atoms, which can avoid some meaningless parsing.\nSo under this path, our'buf' is not cached. This means we have\nownership of it, so we\ncan free it freely, and benefit from this.\n\nThanks.\n--\nZheNing Hu\n"},{"id":"433296","messageId":"CAOLTT8QtLzP5jfqav-UnJj21NczVW-XhRmfxGQ__qdNah_C1yQ@mail.gmail.com","threadId":"56296","inReplyTo":"CAP8UFD0WB7FYtp9aX4qz5BmLiNz1S5PA1U8-cB8b9zRqdZHOjw@mail.gmail.com","subject":"Re: [GSOC] [QUESTION] ref-filter: can %(raw) implement reuse oi.content?","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-08-21T02:36:48Z","receivedAt":"2021-08-21T02:37:03Z","isPatch":false,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"Christian Couder 于2021年8月21日周六 上午12:13写道：\n>\n> Hi ZheNing,\n>\n> On Thu, Aug 19, 2021 at 3:39 AM ZheNing Hu wrote:\n> >\n> > Hi, Christian and Hariom,\n> >\n> > I want to use this patch series as the temporary final version of GSOC project:\n> >\n> > https://github.com/adlternative/git/commits/cat-file-reuse-ref-filter-logic\n>\n> I am still not very happy with the last patch in the series,but it can\n> be improved later.\n>\n\nTo be free to tell me what's not good about it, I can try my best to\nimprove it. :-)\n\n\n> > Due to the branch ref-filter-opt-code-logic or branch\n> > ref-filter-opt-perf patch series\n> > temporarily unable to reflect its optimization to git cat-file\n> > --batch. Therefore, using\n> > branch cat-file-reuse-ref-filter-logic is the most effective now.\n> >\n> > This is the final performance regression test result:\n> > Test upstream/master this\n> > tree\n> > ------------------------------------------------------------------------------------\n> > 1006.2: cat-file --batch-check 0.06(0.06+0.00)\n> > 0.08(0.07+0.00) +33.3%\n> > 1006.3: cat-file --batch-check with atoms 0.06(0.04+0.01)\n> > 0.06(0.06+0.00) +0.0%\n> > 1006.4: cat-file --batch 0.49(0.47+0.02)\n> > 0.48(0.47+0.01) -2.0%\n> > 1006.5: cat-file --batch with atoms 0.48(0.44+0.03)\n> > 0.47(0.46+0.01) -2.1%\n> >\n> > git cat-file --batch has a performance improvement of about 2%.\n> > git cat-file --batch-check still has a performance gap of 33.3%.\n> >\n> > The performance degradation of git cat-file --batch-check is actually\n> > not very big.\n> >\n> > upstream/master (225bc32a98):\n> >\n> > $ hyperfine --warmup=10 \"~/git/bin-wrappers/git cat-file\n> > --batch-check --batch-all-objects\"\n> > Benchmark #1: ~/git/bin-wrappers/git cat-file --batch-check --batch-all-objects\n> > Time (mean ± σ): 596.2 ms ± 5.7 ms [User: 563.0 ms, System: 32.5 ms]\n> > Range (min … max): 586.9 ms … 607.9 ms 10 runs\n> >\n> > cat-file-reuse-ref-filter-logic (709a0c5c12):\n> >\n> > $ hyperfine --warmup=10 \"~/git/bin-wrappers/git cat-file\n> > --batch-check --batch-all-objects\"\n> > Benchmark #1: ~/git/bin-wrappers/git cat-file --batch-check --batch-all-objects\n> > Time (mean ± σ): 601.3 ms ± 5.8 ms [User: 566.9 ms, System: 33.9 ms]\n> > Range (min … max): 596.7 ms … 613.3 ms 10 runs\n> >\n> > The execution time of git cat-file --batch-check is only a few\n> > milliseconds away.\n>\n> Yeah, it looks like less than 1% overhead.\n>\n> Great work!\n>\n> > But look at the execution time changes of git cat-file --batch:\n> >\n> > upstream/master (225bc32a98):\n> >\n> > $ time ~/git/bin-wrappers/git cat-file --batch --batch-all-objects\n> > >/dev/null\n> > /home/adl/git/bin-wrappers/git cat-file --batch --batch-all-objects >\n> > 24.61s user 0.30s system 99% cpu 24.908 total\n> >\n> > cat-file-reuse-ref-filter-logic (709a0c5c12):\n> >\n> > $ time ~/git/bin-wrappers/git cat-file --batch --batch-all-objects >/dev/null\n> > cat-file --batch --batch-all-objects > /dev/null 25.10s user 0.30s\n> > system 99% cpu 25.417 total\n> >\n> > The execution time has been reduced by nearly 0.5 seconds.\n>\n> It looks like it has increased by 0.5s, not been reduced.\n>\n\nUnfortunately, you are right, it is not faster, but slower.\nIt seems that the 2% optimization measured by t/perf does not seem to be\nso credible? I donno.\n\nTest upstream/master this\ntree\n------------------------------------------------------------------------------------\n1006.2: cat-file --batch-check 0.07(0.06+0.01)\n0.08(0.07+0.01) +14.3%\n1006.3: cat-file --batch-check with atoms 0.06(0.05+0.01)\n0.07(0.05+0.01) +16.7%\n1006.4: cat-file --batch 0.49(0.46+0.03)\n0.48(0.47+0.01) -2.0%\n1006.5: cat-file --batch with atoms 0.48(0.45+0.03)\n0.47(0.46+0.01) -2.1%\n\nDo we need to focus on the benchmark instead of the sum of the\nbenchmark plus the\nvariance? i.e. 1006.4, benchmark are 0.46 and 0.47, From this perspective, the\nperformance of git cat-file --batch will be worse.\n\n> > Intuition\n> > tells me that the performance improvement of git cat-file --batch will be\n> > more important.\n> >\n> > In fact, git cat-file origin code directly adds the obtained object data\n> > to the output buffer; But after using ref-filter logic, it needs to copy\n> > the object data to the intermediate data (atom_value), and finally\n> > to the output buffer. At present, we cannot easily eliminate intermediate\n> > data, because git for-each-ref --sort has a lot of dependence on it,\n> > but we can reduce the overhead of copying or allocating memory as\n> > much as possible.\n>\n> Ok.\n>\n> > I had an idea that I didn't implement before: partial data delayed evaluation.\n> > Or to be more specific, waiting until the data is about to be added to\n> > the output\n> > buffer, form specific output content, this may be a way to bypass the\n> > intermediate\n> > data.\n>\n> Yeah, that might be a good idea.\n\nI will try to do it.\n\nThanks.\n--\nZheNing Hu\n"},{"id":"433508","messageId":"CAP8UFD0pZVCHG_i+u_6QAP5yKUpmdYm+fkwr9aJMCDXukKF_7g@mail.gmail.com","threadId":"56296","inReplyTo":"CAOLTT8St0PGPsw6zki2fwO3iVG_4W=s6GEqZfp3cmH=xngcO2A@mail.gmail.com","subject":"Re: [GSOC] [QUESTION] ref-filter: can %(raw) implement reuse oi.content?","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2021-08-24T07:11:14Z","receivedAt":"2021-08-24T07:11:32Z","isPatch":false,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Sat, Aug 21, 2021 at 4:16 AM ZheNing Hu wrote:\n>\n> Christian Couder 于2021年8月20日周五 下午11:58写道：\n> >\n> > On Wed, Aug 18, 2021 at 1:11 PM ZheNing Hu wrote:\n\n> > > -static void grab_sub_body_contents(struct atom_value *val, int deref,\n> > > struct expand_data *data)\n> > > +static void grab_sub_body_contents(struct atom_value *val, int deref,\n> > > struct expand_data *data, int eaten)\n> > > {\n> > > int i;\n> > > const char *subpos = NULL, *bodypos = NULL, *sigpos = NULL;\n> > > @@ -1499,7 +1499,10 @@ static void grab_sub_body_contents(struct\n> > > atom_value *val, int deref, struct exp\n> > > unsigned long buf_size = data->size;\n> > >\n> > > if (atom->u.raw_data.option == RAW_BARE) {\n> > > - v->s = buf;\n> > > + if (eaten)\n> > > + v->s = xmemdupz(buf, buf_size);\n> >\n> > Can the 'buf' already be eaten at this point? I thought that it could\n> > be eaten only through v->s so after this code.\n>\n> I think where the 'buf' is eaten is parse_object_buffer().\n\nYeah, right, and parse_object_buffer() seems to be called by\nget_object() which calls grab_values() after that. So the buf can\nindeed be eaten at that point.\n\n> set_commit_buffer()\n> or parse_tree_buffer() cache the 'buf'. 'eaten' means we don't need to free it\n> manually.\n\nYeah, but it doesn't mean that we cannot use the buf. So perhaps we\ncould still use it, even if it's eaten.\n\n> In grab_sub_body_contents() we can indeed write the address of\n> 'buf' to v->s, but what I worry about is that before we completely write v->s to\n> the output buffer through append_atom(), will this buf be freed in\n> somewhere else?\n\nYeah, that's a good question. But actually it seems that the buf is\nfreed (at least in get_object()) only when it's not eaten.\n\n> > > + else\n> > > + v->s = buf;\n> > > v->s_size = buf_size;\n> > > } else if (atom->u.raw_data.option == RAW_LENGTH) {\n> > > v->s = xstrfmt_len(&v->s_size,\n> > > \"%\"PRIuMAX, (uintmax_t)buf_size);\n> > >\n> > > As parse_object_buffer() does internally: the buffer of commit/tree objects\n> > > needs to be copied, but blob/tag not. You said that the number of commits\n> > > is generally significantly greater than the others. It seems that we cannot\n> > > make full use of this idea. But remember the \"skip_parse_buffer\" before?\n> > > If we skip the parse_object_buffer(), this buffer is also \"!eaten\".\n> > >\n> > > In other words, those default paths of git cat-file --batch are included in it!\n> > > So from the perspective of performance regression, this patch will be\n> > > beneficial.\n> >\n> > Yeah, it seems that we can benefit from something like this, but it'd\n>\n> Only when the data allocated to us is dynamically allocated and we do have\n> the ownership of it, we can benefit by reducing copies and allocate\n> memory again.\n>\n> > be nice if things were clarified a bit more.\n>\n> OK. In get_object(), eaten means that the oi->content we obtained is cached\n> by git in parse_object_buffer(). This means that we don't need to free this buf\n> manually. In 28511adfa5 ([GSOC] ref-filter: skip parse_object_buffer()), we skip\n> parse_buffer() when using some atoms, which can avoid some meaningless parsing.\n> So under this path, our'buf' is not cached. This means we have\n> ownership of it, so we\n> can free it freely, and benefit from this.\n\nIn the patch we are not freeing it, we only duplicate it when it's\neaten. So I am not sure that the above explanation are clear enough.\nThat's the issue I have with this patch, but maybe the commit message\ncan now be discussed and improved after sending the patch series to\nthe mailing list instead of in this thread.\n\nThanks!\n"},{"id":"433717","messageId":"CAOLTT8S-bbnb8BecJRzLQytEEnZKU_VwRwiF1tk+gy0yrjTOog@mail.gmail.com","threadId":"56296","inReplyTo":"CAP8UFD0pZVCHG_i+u_6QAP5yKUpmdYm+fkwr9aJMCDXukKF_7g@mail.gmail.com","subject":"Re: [GSOC] [QUESTION] ref-filter: can %(raw) implement reuse oi.content?","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-08-25T08:11:45Z","receivedAt":"2021-08-25T08:11:59Z","isPatch":false,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"Christian Couder 于2021年8月24日周二 下午3:11写道：\n>\n> > set_commit_buffer()\n> > or parse_tree_buffer() cache the 'buf'. 'eaten' means we don't need to free it\n> > manually.\n>\n> Yeah, but it doesn't mean that we cannot use the buf. So perhaps we\n> could still use it, even if it's eaten.\n>\n\nYes. Just like the following question: How do we know that the eaten 'buf' has\nnot been free() by some logic in git when we want to use it?\n\n> > In grab_sub_body_contents() we can indeed write the address of\n> > 'buf' to v->s, but what I worry about is that before we completely write v->s to\n> > the output buffer through append_atom(), will this buf be freed in\n> > somewhere else?\n>\n> Yeah, that's a good question. But actually it seems that the buf is\n> freed (at least in get_object()) only when it's not eaten.\n>\n\nget_object() will free the 'buf' which has not been eaten, but we can\ntake use of\nit (v->s = buf and we free it later)\n\n> > > > + else\n> > > > + v->s = buf;\n> > > > v->s_size = buf_size;\n> > > > } else if (atom->u.raw_data.option == RAW_LENGTH) {\n> > > > v->s = xstrfmt_len(&v->s_size,\n> > > > \"%\"PRIuMAX, (uintmax_t)buf_size);\n> > > >\n> > > > As parse_object_buffer() does internally: the buffer of commit/tree objects\n> > > > needs to be copied, but blob/tag not. You said that the number of commits\n> > > > is generally significantly greater than the others. It seems that we cannot\n> > > > make full use of this idea. But remember the \"skip_parse_buffer\" before?\n> > > > If we skip the parse_object_buffer(), this buffer is also \"!eaten\".\n> > > >\n> > > > In other words, those default paths of git cat-file --batch are included in it!\n> > > > So from the perspective of performance regression, this patch will be\n> > > > beneficial.\n> > >\n> > > Yeah, it seems that we can benefit from something like this, but it'd\n> >\n> > Only when the data allocated to us is dynamically allocated and we do have\n> > the ownership of it, we can benefit by reducing copies and allocate\n> > memory again.\n> >\n> > > be nice if things were clarified a bit more.\n> >\n> > OK. In get_object(), eaten means that the oi->content we obtained is cached\n> > by git in parse_object_buffer(). This means that we don't need to free this buf\n> > manually. In 28511adfa5 ([GSOC] ref-filter: skip parse_object_buffer()), we skip\n> > parse_buffer() when using some atoms, which can avoid some meaningless parsing.\n> > So under this path, our'buf' is not cached. This means we have\n> > ownership of it, so we\n> > can free it freely, and benefit from this.\n>\n> In the patch we are not freeing it, we only duplicate it when it's\n> eaten. So I am not sure that the above explanation are clear enough.\n> That's the issue I have with this patch, but maybe the commit message\n> can now be discussed and improved after sending the patch series to\n> the mailing list instead of in this thread.\n>\n\nMy thoughts are:\n\n1. those 'buf' which have been eaten, because we don't know when they\nwill be freed, if we\nuse it in append_atom(), it may be a little unsafe if it have been\nfreed, but it is safe to use after\ncopying it;\n2. those uneaten 'buf', it can be used safely.\n\nMaybe we can guarantee that the buf have not been freed when append_atom()?\nI don't have a good grasp.\n\n> Thanks!\n\nThanks.\n--\nZheNing Hu\n"}]}