On Thu, Jan 9, 2020 at 10:22 AM Amar Tumballi <ama...@gmail.com> wrote:
> > > On Thu, Jan 9, 2020 at 2:33 PM Xavi Hernandez <jaher...@redhat.com> wrote: > >> On Thu, Jan 9, 2020 at 9:44 AM Amar Tumballi <ama...@gmail.com> wrote: >> >>> >>> >>> On Thu, Jan 9, 2020 at 1:38 PM Xavi Hernandez <jaher...@redhat.com> >>> wrote: >>> >>>> On Sun, Dec 22, 2019 at 4:56 PM Yaniv Kaul <yk...@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@kadalu glusterfs]$ git grep extra_stdfree | wc -l > 40 > [amar@kadalu glusterfs]$ git grep extra_free | wc -l > 5 > Yes, they call dict_unserialize() and then store the unserialized data into those variables. That's what I'm saying it's not necessary. > >> >> >>> >>>> 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@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@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@gluster.org https://lists.gluster.org/mailman/listinfo/gluster-devel
