Re: [PATCH v7 01/20] trace-cmd library: Add support for compression algorithms

[Tzvetomir Stoyanov <tz.stoyanov@gmail.com>]

On Tue, Jan 25, 2022 at 7:30 PM Tzvetomir Stoyanov
<tz.stoyanov@gmail.com> wrote:
>
> On Tue, Jan 25, 2022 at 5:08 PM Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> > On Tue, 25 Jan 2022 09:48:43 +0200
> > Tzvetomir Stoyanov <tz.stoyanov@gmail.com> wrote:
> >
> > > > > >
> > > > > > > +     free(bytes);
> > > > > > > +     handle->pointer = 0;
> > > > > > > +     handle->capacity = s_uncompressed;
> > > > > >
> > > > > > If size is the greater of the two (compressed could be larger) and
> > > > > > capacity is the allocated buffer, shouldn't we make it equal to size
> > > > > > and not uncompressed?
> > > > >
> > > > > The name "capacity" in reading logic is a bit confusing, as it
> > > > > actually is used to track the size of uncompressed data. The buffer
> > > > > may be larger, but if the uncompressed is smaller - the rest is
> > > > > garbage and must not be read.
> > > >
> > > > Then the extend function is broken:
> > >
> > > For this particular use case, the logic is not correct - the realloc()
> > > could be redundant, if the real buffer size is bigger than the
> > > capacity. But as usually a compression handle is used for reading or
> > > for writing only, this is unlikely to happen. At least I cannot
> > > imagine a use case where the same compression handle will be used for
> > > reading a compress block and for writing compressed data. But even if
> > > we hit that case - there is no data corruption or memory leak, just a
> > > redundant realloc(). To solve that, reading and writing capacities
> > > could be introduced ?
> > >     struct tracecmd_compression {
> > >        ...
> > >        unsigned int            capacity_read;
> > >        unsigned int            capacity_write;
> > >        ...
> > >    }
> >
> > A redundant realloc is not an issue, in fact, they are pretty fast (it
> > detects that the requested size still fits what is allocated and just
> > returns).
> >
> > >
> > > But that change will be for a very rare case - when the compressed
> > > data size is bigger than the uncompressed (usually happens only with a
> > > very small data) + the same compression handle is used for compression
> > > and decompression (not used in that way in trace-cmd).
> > >
> > > >
> > > > static inline int buffer_extend(struct tracecmd_compression *handle, unsigned int size)
> > > > {
> > > >         int extend;
> > > >         char *buf;
> > > >
> > > >         if (size <= handle->capacity)
> > > >                 return 0;
> > > >
> > > >         extend = ((size - handle->capacity) / BUFSIZ + 1) * BUFSIZ;
> > > >         buf = realloc(handle->buffer, handle->capacity + extend);
> > > >         if (!buf)
> > > >                 return -1;
> > > >         handle->buffer = buf;
> > > >         handle->capacity += extend;
> > > >
> > > >         return 0;
> > > > }
> > > >
> > > > As it sets the capacity to the allocated size, not what is stored.
> > > >
> > > > Perhaps you meant:
> > > >
> > > >         handle->capacity += size;
> >
> > The above should still work, instead of doing the += extend.
> >
> > The point I was making is that capacity should either be the requested size
> > allocated, or what is actually allocated. The above function sets it to
> > what is allocated, and the other code sets it to what was requested. We
> > should pick it to be one or the other.
>
> I didn't know about that realloc optimization (check if requested size
> fits into already allocated memory). In that case, we can use the
> requested size in both places. But still allocate a page aligned
> buffer in buffer_extend(), to avoid frequent reallocs (although
> realloc will be called almost every time).
>

I did some tests, these extra realloc() calls actually cause 7-8% slow
down on writing compressed trace files. So, I decided to go with the
other approach - use capacity_read and capacity_write.

> >
> > -- Steve

>
>
>
> --
> Tzvetomir (Ceco) Stoyanov
> VMware Open Source Technology Center



-- 
Tzvetomir (Ceco) Stoyanov
VMware Open Source Technology Center