[Gluster-devel] What do extra_free and extrastd_free params do in the dictionary object?

[Amar Tumballi]

On Thu, Jan 9, 2020 at 2:33 PM Xavi Hernandez <jahernan at redhat.com> wrote:

> On Thu, Jan 9, 2020 at 9:44 AM Amar Tumballi <amarts at gmail.com> wrote:
>
>>
>>
>> On Thu, Jan 9, 2020 at 1:38 PM Xavi Hernandez <jahernan at redhat.com>
>> wrote:
>>
>>> On Sun, Dec 22, 2019 at 4:56 PM Yaniv Kaul <ykaul at redhat.com> wrote:
>>>
>>>> I could not find a relevant use for them. Can anyone enlighten me?
>>>>
>>>
>>> I'm not sure why they are needed. They seem to be used to keep the
>>> unserialized version of a dict around until the dict is destroyed. I
>>> thought this could be because we were using pointers to the unserialized
>>> data inside dict, but that's not the case currently. However, checking very
>>> old versions (pre 3.2), I see that dict values were not allocated, but a
>>> pointer to the unserialized data was used.
>>>
>>
>> Xavi,
>>
>> While you are right about the intent, it is used still, at least when I
>> grepped latest repo to keep a reference in protocol layer.
>>
>> This is done to reduce a copy after the dictionary's binary content is
>> received from RPC. The 'extra_free' flag is used when we use a
>> GF_*ALLOC()'d buffer in protocol to receive dictionary, and extra_stdfree
>> is used when RPC itself allocates the buffer and hence uses 'free()' to
>> free the buffer.
>>
>
> I don't see it. When dict_unserialize() is called, key and value are
> allocated and copied, so  why do we need to keep the raw data after that ?
>
> In 3.1 the value was simply a pointer to the unserialized data, but
> starting with 3.2, value is memdup'ed. Key is always copied. I don't see
> any other reference to the unserialized data right now. I think that
> instead of assigning the raw data to extra_(std)free, we should simply
> release that memory and remove those fields.
>
> Am I missing something else ?
>

I did grep on 'extra_stdfree' and 'extra_free' and saw that many handshake/
and protocol code seemed to use it. Haven't gone deeper to check which part.

[amar at kadalu glusterfs]$ git grep extra_stdfree | wc -l
40
[amar at kadalu glusterfs]$ git grep extra_free | wc -l
5


>
>
>>
>>> I think this is not needed anymore. Probably we could remove these
>>> fields if that's the only reason.
>>>
>>
>> If keeping them is hard to maintain, we can add few allocation to remove
>> those elements, that shouldn't matter much IMO too. We are not using
>> dictionary itself as protocol now (which we did in 1.x series though).
>>
>> Regards,
>> Amar
>> ---
>> https://kadalu.io
>>
>>
>>
>>> TIA,
>>>> Y.
>>>> _______________________________________________
>>>>
>>>> Community Meeting Calendar:
>>>>
>>>> APAC Schedule -
>>>> Every 2nd and 4th Tuesday at 11:30 AM IST
>>>> Bridge: https://bluejeans.com/441850968
>>>>
>>>>
>>>> NA/EMEA Schedule -
>>>> Every 1st and 3rd Tuesday at 01:00 PM EDT
>>>> Bridge: https://bluejeans.com/441850968
>>>>
>>>> Gluster-devel mailing list
>>>> Gluster-devel at gluster.org
>>>> https://lists.gluster.org/mailman/listinfo/gluster-devel
>>>>
>>>> _______________________________________________
>>>
>>> Community Meeting Calendar:
>>>
>>> APAC Schedule -
>>> Every 2nd and 4th Tuesday at 11:30 AM IST
>>> Bridge: https://bluejeans.com/441850968
>>>
>>>
>>> NA/EMEA Schedule -
>>> Every 1st and 3rd Tuesday at 01:00 PM EDT
>>> Bridge: https://bluejeans.com/441850968
>>>
>>> Gluster-devel mailing list
>>> Gluster-devel at gluster.org
>>> https://lists.gluster.org/mailman/listinfo/gluster-devel
>>>
>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.gluster.org/pipermail/gluster-devel/attachments/20200109/98621773/attachment.html>