| Message ID | patch-v12-7.8-11f7aa026b4-20220329T135446Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | unpack-objects: support streaming blobs to disk | expand |
Am 29.03.22 um 15:56 schrieb Ævar Arnfjörð Bjarmason: > The unpack_one() function will call either a non-trivial > unpack_delta_entry() or a trivial unpack_non_delta_entry(). Let's > inline the latter in the only caller. > > Since 21666f1aae4 (convert object type handling from a string to a > number, 2007-02-26) the unpack_non_delta_entry() function has been > rather trivial, and in a preceding commit the "dry_run" condition it > was handling went away. > > This is not done as an optimization, as the compiler will easily > discover that it can do the same, rather this makes a subsequent > commit easier to reason about. How exactly does inlining the function make the next patch easier to understand or discuss? Plugging in the stream_blob() call to handle the big blobs looks the same with or without the unpack_non_delta_entry() call to me. > As it'll be handling "OBJ_BLOB" in a > special manner let's re-arrange that "case" in preparation for that > change. > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- > builtin/unpack-objects.c | 18 +++++++----------- > 1 file changed, 7 insertions(+), 11 deletions(-) Reducing the number of lines can be an advantage. *shrug* > > diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c > index e3d30025979..d374599d544 100644 > --- a/builtin/unpack-objects.c > +++ b/builtin/unpack-objects.c > @@ -338,15 +338,6 @@ static void added_object(unsigned nr, enum object_type type, > } > } > > -static void unpack_non_delta_entry(enum object_type type, unsigned long size, > - unsigned nr) > -{ > - void *buf = get_data(size); > - > - if (buf) > - write_object(nr, type, buf, size); > -} > - > static int resolve_against_held(unsigned nr, const struct object_id *base, > void *delta_data, unsigned long delta_size) > { > @@ -479,12 +470,17 @@ static void unpack_one(unsigned nr) > } > > switch (type) { > + case OBJ_BLOB: > case OBJ_COMMIT: > case OBJ_TREE: > - case OBJ_BLOB: > case OBJ_TAG: > - unpack_non_delta_entry(type, size, nr); > + { > + void *buf = get_data(size); > + > + if (buf) > + write_object(nr, type, buf, size); > return; > + } > case OBJ_REF_DELTA: > case OBJ_OFS_DELTA: > unpack_delta_entry(type, size, nr);
On Wed, Mar 30 2022, René Scharfe wrote: > Am 29.03.22 um 15:56 schrieb Ævar Arnfjörð Bjarmason: >> The unpack_one() function will call either a non-trivial >> unpack_delta_entry() or a trivial unpack_non_delta_entry(). Let's >> inline the latter in the only caller. >> >> Since 21666f1aae4 (convert object type handling from a string to a >> number, 2007-02-26) the unpack_non_delta_entry() function has been >> rather trivial, and in a preceding commit the "dry_run" condition it >> was handling went away. >> >> This is not done as an optimization, as the compiler will easily >> discover that it can do the same, rather this makes a subsequent >> commit easier to reason about. > > How exactly does inlining the function make the next patch easier to > understand or discuss? Plugging in the stream_blob() call to handle the > big blobs looks the same with or without the unpack_non_delta_entry() > call to me. The earlier version of it without this prep cleanup can be seen at https://lore.kernel.org/git/patch-v10-6.6-6a70e49a346-20220204T135538Z-avarab@gmail.com/ So yes, this could be skipped, but I tought with this step it was easier to understand. We previously had to change "void *buf = get_data(size);" in the function to just "void *buf", and do the assignment after the condition that's being checked here. I think it's also more obvious in terms of control flow if we're checking OBJ_BLOB here to not call a function which has a special-case just for OBJ_BLOB, we can just do that here instead. >> As it'll be handling "OBJ_BLOB" in a >> special manner let's re-arrange that "case" in preparation for that >> change. >> >> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> >> --- >> builtin/unpack-objects.c | 18 +++++++----------- >> 1 file changed, 7 insertions(+), 11 deletions(-) > > Reducing the number of lines can be an advantage. *shrug* There was also the (admittedly rather small) knock-on-effect on 8/8. Before this it was 8 lines added / 1 removed when it came to the code impacted by this change, now it's a 5 added/0 removed in the below "switch". So I think it's worth keeping.
Am 31.03.22 um 14:42 schrieb Ævar Arnfjörð Bjarmason: > > On Wed, Mar 30 2022, René Scharfe wrote: > >> Am 29.03.22 um 15:56 schrieb Ævar Arnfjörð Bjarmason: >>> The unpack_one() function will call either a non-trivial >>> unpack_delta_entry() or a trivial unpack_non_delta_entry(). Let's >>> inline the latter in the only caller. >>> >>> Since 21666f1aae4 (convert object type handling from a string to a >>> number, 2007-02-26) the unpack_non_delta_entry() function has been >>> rather trivial, and in a preceding commit the "dry_run" condition it >>> was handling went away. >>> >>> This is not done as an optimization, as the compiler will easily >>> discover that it can do the same, rather this makes a subsequent >>> commit easier to reason about. >> >> How exactly does inlining the function make the next patch easier to >> understand or discuss? Plugging in the stream_blob() call to handle the >> big blobs looks the same with or without the unpack_non_delta_entry() >> call to me. > > The earlier version of it without this prep cleanup can be seen at > https://lore.kernel.org/git/patch-v10-6.6-6a70e49a346-20220204T135538Z-avarab@gmail.com/ This plugged the special case into unpack_non_delta_entry(). The alternative I had in mind was to plug it into the switch statement as the current patch does, just without inlining unpack_non_delta_entry(). > So yes, this could be skipped, but I tought with this step it was easier > to understand. > > We previously had to change "void *buf = get_data(size);" in the > function to just "void *buf", and do the assignment after the condition > that's being checked here. > > I think it's also more obvious in terms of control flow if we're > checking OBJ_BLOB here to not call a function which has a special-case > just for OBJ_BLOB, we can just do that here instead. > >>> As it'll be handling "OBJ_BLOB" in a >>> special manner let's re-arrange that "case" in preparation for that >>> change. >>> >>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> >>> --- >>> builtin/unpack-objects.c | 18 +++++++----------- >>> 1 file changed, 7 insertions(+), 11 deletions(-) >> >> Reducing the number of lines can be an advantage. *shrug* > > There was also the (admittedly rather small) knock-on-effect on > 8/8. Before this it was 8 lines added / 1 removed when it came to the > code impacted by this change, now it's a 5 added/0 removed in the below > "switch". > > So I think it's worth keeping.
diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c index e3d30025979..d374599d544 100644 --- a/builtin/unpack-objects.c +++ b/builtin/unpack-objects.c @@ -338,15 +338,6 @@ static void added_object(unsigned nr, enum object_type type, } } -static void unpack_non_delta_entry(enum object_type type, unsigned long size, - unsigned nr) -{ - void *buf = get_data(size); - - if (buf) - write_object(nr, type, buf, size); -} - static int resolve_against_held(unsigned nr, const struct object_id *base, void *delta_data, unsigned long delta_size) { @@ -479,12 +470,17 @@ static void unpack_one(unsigned nr) } switch (type) { + case OBJ_BLOB: case OBJ_COMMIT: case OBJ_TREE: - case OBJ_BLOB: case OBJ_TAG: - unpack_non_delta_entry(type, size, nr); + { + void *buf = get_data(size); + + if (buf) + write_object(nr, type, buf, size); return; + } case OBJ_REF_DELTA: case OBJ_OFS_DELTA: unpack_delta_entry(type, size, nr);
The unpack_one() function will call either a non-trivial unpack_delta_entry() or a trivial unpack_non_delta_entry(). Let's inline the latter in the only caller. Since 21666f1aae4 (convert object type handling from a string to a number, 2007-02-26) the unpack_non_delta_entry() function has been rather trivial, and in a preceding commit the "dry_run" condition it was handling went away. This is not done as an optimization, as the compiler will easily discover that it can do the same, rather this makes a subsequent commit easier to reason about. As it'll be handling "OBJ_BLOB" in a special manner let's re-arrange that "case" in preparation for that change. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- builtin/unpack-objects.c | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-)