[MPlayer-dev-eng] [PATCHv2 7/8] stream ftp: Allocate command buffer on-heap

[Alexander Strasser]

Reimar Döffinger wrote:
> On Tue, Nov 20, 2012 at 12:33:34AM +0100, Alexander Strasser wrote:
> > Reimar Döffinger wrote:
> > > On 19 Nov 2012, at 22:24, Alexander Strasser <eclipse7 at gmx.net> wrote:
> > > > Alexander Strasser wrote:
> > > >> Signed-off-by: Alexander Strasser <eclipse7 at gmx.net>
> > > >> ---
> > > >> stream/stream_ftp.c | 44 +++++++++++++++++++++++++-------------------
> > > >> 1 file changed, 25 insertions(+), 19 deletions(-)
> > > >> 
> > > > [...]
> > > >> @@ -434,8 +439,9 @@ static int open_f(stream_t *stream,int mode, void* opts, int* file_format) {
> > > >> 
> > > >>   // Allocate buffers
> > > >>   p->buf = malloc(BUFSIZE);
> > > >> +  p->cmd_buf = malloc(CMD_BUFSIZE);
> > > > 
> > > >  It occurred to me today that I was probably too conservative with
> > > > this patch.
> > > > 
> > > >  If we replace above allocation with something along the lines of:
> > > > 
> > > >  p->cmd_buf_size = 5 + FFMIN(strlen(p->filename)+1, 1<<15); // "RETR <file>"
> > > >  p->cmd_buf = malloc(p->cmd_buf_size);
> > > 
> > > I find the FFMIN rather questionable, particularly since the 32 kB it limits to is small enough that there's little point in not always just allocating the maximum.
> > > Due to memory fragmentation placing it directly in some struct instead of a separate malloc might actually be worth more than making the allocation as small as possible.
> > 
> >   OK! I am not entirely convinced but your view is more pragmatic and
> > you have a point. Additionally the code is simpler and better for most
> > (all?) of its current users.
>
> Well, I only said "might" anyway. But if you are curious you could
> run MPlayer playing even some simple audio file through valgrind's
> massif tool and even without cache I think you'll come to the
> conclusion that 32 kB just don't matter...

  I am afraid there was a slight misunderstanding. I mean from the perspective
of the isolated module. But I do not think it matters much and I do think
your overall assertion is correct.

> > > Using a separate size field might have its advantages though even if it would be a constant for now.
> > 
> >   Do you meant because it could be injected from outside (e.g. via options)?
> > Then it could be useful for the recv buffer size too. But could also be done
> > at the point we plan to make if configurable. Did you have anything else in
> > mind?
> 
> Mostly it just means that everything is in place if you want to change
> it.

  OK, thanks for the explanation.

> > > >  - Don't allocate 8k every time we are opening a file via ftp
> > > 
> > > I don't really think this matters at all.
> > > 
> > > >  - If we are opening a filename longer than ~8k it will still work
> > > 
> > > Sure, but  most FTP servers will run e.g. Linux with a PATH_MAX of 4kB, so that is truly unlikely to ever happen.
> > 
> >   When looking at your statements above what I wonder if you would oppose to
> > just make it an array of size 8k inside the private context struct?
> 
> No I wouldn't mind that, that's basically what I meant by "placing it
> directly in some struct".
> 
> >   It would not go so well with configurable buffer sizes I talked about above.
> > But for now we don't know if we will implement that ever.
> 
> I see little point in having it configurable really.

  I see--- but I decided to commit the original patches I sent here
because it fits better with my local patch queue. Also I didn't want
to have the cmd buffer that lives for the lifetime of the stream to
be allocated differently than the recv buffer maintenance reasons.

  With my next patch series I may decide to put both of them into the
priv struct with the current fixed sizes. As I think it won't be of
much use to have them configurable and the recv buffer probably won't
need to grow (after all we have the cache2 mechanism in place).

  All eight patches committed.

  Closing now with the usual: Flame me on -cvslog :)

Thanks for the reviews and discussions,
  Alexander