Re: [PATCH] calloc should not duplicate malloc logic.

[Siddhesh Poyarekar <siddhesh at redhat dot com>]

On Wed, Feb 26, 2014 at 05:25:21PM +0100, OndÅej BÃlka wrote:
> Siddhesh, before you speak you need check the facts. Use a benchmark
> so you could quantify if it is worth of concern or difference will just 
> disappear in noise.

You're posting a patch and also admitting to a performance regression.
I'm not sure why you think I should spend my time writing a benchmark
to refute your patch.

> As there is currently not a benchmark I modified my malloc benchmark and
> results look mostly indistinguishible.
> 
> Also for subpage requests trying not to clear memory is bad idea anyway
> as application will write to that area and it will act as prefetching
> whcih could sometimes improve overall performance.

Sub-page requests don't go to mmap unless someone is stupid enough to
set the mmap threshold that low.

> A performance is not primary concern now, maintainability is. As in
> previous cauldron improving malloc was one of main points for
> improvement. If we do not move quickly then we will not get lot done.

It is widely agreed that malloc is a very important point for
improvement.  However, moving fast in that direction does not
necessarily mean dumping patches on mainline.  We as maintainers have
the privilege of creating private branches where we can work on our
own ideas, come up with a patch set that shows an obvious improvement
in either design or performance or both.  I'd prefer you use that
rather than dump half-baked changes with a promise of something coming
up real soon to justify that half-baked change.

> Siddhesh a following program already consumes a gigabyte in rss so your
> claim is nonsense again.
> 
> #include <malloc.h>
> int main()
> {
>   char *x = calloc (1000000000,1);
>   sleep(1000);
> }

You haven't understood why this is a bad idea.  The pages will have
been zeroed twice with your patch, once by mmap and again with memset.

> And benchmark is following:

Post it as a patch for review and work on it with the reviewers to get
it included.  For one, the benchmark does not address the issue I
raised at all, which is allocation sizes that currently use mmap.  I
vaguely recognize it as a benchmark you had proposed in the past that
others including me had raised questions about.  You might want to
work on fixing that first.

Again, I'd love to see your work on the allocator on a private branch
and would love to add it as a patchset to Fedora before it is reviewed
for mainline fitness when it is ready for some real world testing, but
I am personally opposed to dumping WIP patches on mainline when there
is no real target in sight.

Siddhesh

Attachment: pgpnyKJF_Cbek.pgp
Description: PGP signature