Re: [RFC 16/17] object_array_entry: copy name before storing in name field

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: May 20, 2013, 14:42 UTC
Message-ID: <519A365E.6020807@alum.mit.edu>
In-Reply-To: <CALKQrgeZ3yOogZhumQ6sQ=7B4XoEUDxPNt6Lj7ZAs13oJTOeUA@mail.gmail.com>

On 05/20/2013 12:33 PM, Johan Herland wrote:

Show 28 quoted lines

> On Sun, May 19, 2013 at 10:27 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> This is the culmination of the last few commits.  Since some callers
>> want to store refnames in the name field of object_array elements, but
>> we don't want those callers to assume that the refnames that they got
>> from for_each_ref() have infinite lifetime, the easiest thing to do is
>> have object_array make a copy of the names before writing them in the
>> entries, and to free the names for entries that are no longer in use.
>> This change fixes the problem, but has some disadvantages:
>>
>> * It requires extra copies to be made of strings that are already
>>   copies, for example when the results of path_name(path, name) are
>>   used as a name in revision.c:add_object().  This might be rare
>>   enough that it can be ignored (though the original result of
>>   path_name() would have to be freed, which this patch doesn't do so
>>   there is a memory leak).
>>
>> * Many callers store the empty string ("") as the name; for example,
>>   most of the entries created during a run of rev-list have "" as
>>   their name.  This means that lots of needless copies of "" are being
>>   made.  I think that the best solution to this problem would be to
>>   store NULL rather than "" for such entries, but I haven't figured
>>   out all of the places where the name is used.
> 
> Use strbufs?
> 
> No allocation (except for the strbuf object itself) is needed for
> empty strings, and string ownership and be transferred to and from it
> to prevent extra copies.

That would cost two extra size_t per object_array_entry. I have the feeling that this structure is used often enough that the extra overhead would be a disadvantage, but I'm not sure.

The obvious alternative would be to teach users to deal with NULL and either add another constructor alternative that transfers string ownership or *always* transfer string ownership and change the callers to call xstrdup() if they don't already own the name string. I think I will try that approach first.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/