Re: [PATCH GSoC v2 4/6] fetch-object-info: parse type from server response

Junio C Hamano <[email protected]> writes:

>> +		} else if (!strcmp(reader->line, "type")) {
>> +			type_index = (int)i;
>> +			for (size_t j = 0; j < args->oids->nr; j++)
>> +				object_info_data[j].typep =
>> +					xcalloc(1, sizeof(*object_info_data[j].typep));
>
> Do object_info_data[j].typep and object_info_data[k].typep need to
> be independently freeable?  Separate allocations by calling calloc
> args->oids->nr times would allow that, but if there is no such need,
> nr contiguous allocation of them,

Stepping back a bit, the design of "odb.h:struct object_info" look
rather curious.

Why does the struct store scalar values like "enum object_type" and
"size_t" as a pointer to elsewhere, and does not store the values
right there in the structure itself?  By forcing the caller to
allocate an "enum object_type" for each of these object_info[]
elements, the design requires 8-byte for a pointer to the heap and
malloc overhead, probably ~16 bytes or more, in addition to store a
single "enum object_type" that can be stored in a single byte.

We are probably using this pointer indirection to say "ah, typep is
NULL so the caller did not ask for this information and the object
layer does not have to provide one", plus "typep is NULL so the
engine did not give this information for the object".  But we can do
so with two bitfields "unsigned typep_asked:1, typep_valid:1;"
instead of paying ~24-byte or more heap allocation overhead.

Again, this is not something we can change in the middle of this
topic, but since I noticed it and found iffy, I'll leave a note here
to stir the pot anyway.

Stepping back a bit, the design of odb.h:struct object_info looks
rather curious.

Why does the struct store scalar values like enum object_type and
size_t as pointers to elsewhere, rather than storing the values
right there in the structure itself?  By forcing the caller to
allocate an enum object_type for each of these object_info[]
elements, the design requires an 8-byte pointer to the heap and
malloc overhead, probably ~16 bytes or more, to store a single
enum object_type that could fit in a single byte.

We are probably using this pointer indirection to say "ah, '.typep'
is NULL so the caller did not ask for this information and the
object layer does not have to provide it", plus "'.typep' is NULL
so the engine did not give this information for the object".  But we
can do so with two bitfields 

    unsigned type_asked:1,
             type_valid:1;

instead of paying ~24 bytes or more of heap allocation overhead.

Again, this is not something we can change in the middle of this
topic, but since I noticed it and found it iffy, I'll leave a note
here to stir the pot anyway.  It could be something we may want to
clean-up much later after all the dust settles from this year's
GSoC.  I dunno.