Re: Bad COMPACT_ALLOC_CHUNK size in tsearch/spell.c?

On Tue, Apr 26, 2011 at 11:46 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> I wrote:
>> Hmm ... the idea behind that comment is evidently that it's best if the
>> blocks allocated by this code form independent malloc blocks in aset.c;
>> but right offhand I can't see why that'd be important.  It's obviously
>> not functionally necessary, since the code works as-is, and I don't
>> immediately see a reason to think that it'd be more efficient.  If
>> anything it might be best the way it is, with the allocation
>> corresponding to the largest allowable small-chunk size.
>
>> I'm thinking the comment is just brain fade ... which is annoying
>> because I have a feeling it's my own comment :-( ... but I'm darned
>> if I can reconstruct the reasoning for it now.
>
> After a bit longer, the reasoning came back to me ...
>
> The key point here is that alloc requests larger than ALLOC_CHUNK_LIMIT
> don't affect aset.c's other allocation behavior: it hands you out a
> block gotten directly from malloc, end of story.  However, if you keep
> on asking for chunks <= ALLOC_CHUNK_LIMIT, those will come out of larger
> and larger malloc'd blocks.  That means larger and larger potential
> wastage when you stop asking for chunks.  In normal usage this doesn't
> matter a whole lot because if you stop asking for space in a context,
> you're probably going to release it not long after that.  But ispell
> is building a dictionary that will likely live for the duration of the
> process, so if it wastes space comparable to the useful size of the
> dictionary, that's a tad more annoying.
>
> In short: the property suggested by the comment is meant to minimize
> wasted memory, and we're not doing it right to achieve that.
>
> Now on the other hand, it probably takes more cycles to build it using
> a number of individual malloc calls than the way we're doing it now.
> But since it's a once-per-process overhead, I'm thinking that's probably
> a good tradeoff.
>
> So now I think the comment is right and we need to bump up the value of
> the constant.

right -- spell.c was deliberately trying to influence allocator
behavior. Should it really do that though without direct
participation of some form? (like, exposing the allocation chunk size
macro?) It seems a bit dodgy as it stands...

merlin