Re: Chunked mempools, a second verdict

Andres Kroonmaa wrote:
> By now I've already got it that we not after symbol name count. What
> actually had confused me nuts was counting all sort of src/ functions
> and wrappers as part of the API.

Good.

> /* Allocator API */
> extern MemPool *memPoolCreate(const char *label, size_t obj_size);
> extern void *memPoolAlloc(MemPool * pool);
> extern void memPoolFree(MemPool * pool, void *obj);
> extern void memPoolDestroy(MemPool * pool);

Almost fine, only a minor technicality. memPoolDestroy should read

  extern void memPoolDestroy(MemPool ** pool);

and NULL:ify the passed pointer when the pool has been destroyed. This
is common pratice in Squid to limit the risk of dangling pointers. Most
commonly seen with safe_free().

> /* Tune API */
> extern void memPoolSetChunkSize(MemPool * pool, size_t chunksize);
> extern void memPoolSetIdleLimit(size_t new_idle_limit);

Fine, as long as these are optional.

> /* Stats API */
> extern size_t memPoolGetIdleLimit(); /* might include in Totals */
> extern int memPoolGetCount(); /* might be unnecessary */
> extern MemPool *memPoolGetPool(int index);
> extern int memPoolGetStats(MemPoolStats * stats, MemPool * pool);
> extern int memPoolGetTotals(MemPoolTotals * stats);

I think the memPoolGetCount/memPoolGetPool interface should be replaced
by a iterator interface
  extern MemPool * memPoolGetFirstPool(void);
  extern MemPool * memPoolGetNextPool(MemPool *previous_pool);

or even one using a abstract iterator type

  extern MemPoolIterator * memPoolIterate(void);
  extern MemPool * memPoolGetNextPool(MemPoolIterator **);
and optionally (in which case one * above goes away)
  extern void memPoolIterateDone(MemPoolIterator **)

Note: The ** is to be able to NULL:ify the pointer as described above.

memPoolGetTotals should probably be named memPoolGetGlobalStats, and
include the idle limit. Also applies to the data type.

> /* Stats history API */
> extern void memPoolCheckRates(); /* stats history checkpoints */

Explain please.

> extern void memPoolUpdateTime(time_t time); /* better avoid */

Cut. Use current_dtime or a similar global variable.

> /* Module housekeeping API */
> extern void memPoolInit(void);
> extern void memPoolClean(time_t maxage); /* maybe pass curtime also? */
> If we implement selfIniting, selfcleaning memPools, may omit both.

The Init call should not be needed I think. Can be done automatically on
memPoolCreate() and undone when the last pool is memPoolDestroy():ed.

Self-cleaning memory pools would be nice, but either way is fine.

> /* Wrappers under src/mem.c */
> /* NOT part of an API, so in terms of library API - irrelevant */
> extern void memConfigure(void);
> extern void memPoolCleanIdlePools(void *unused);
> extern int memPoolInUseCount(MemPool * pool);
> extern int memPoolsTotalAllocated(void);

Fine.

> /* cachemgr presenter, could also be moved into mem.c */
> void memReport(StoreEntry *);

Think it should. There is no need for a separate pools report only
reporting the mem.c types.

> /* MemMeter */
>
> extern void memMeterSyncHWater(MemMeter * m);
> This shouldn't be part of an API. Why not a macro?

Please explain where/how MemMeter is used. If internal to MemPool.c then
move it in there and it is fully out of the discussion.

Now, please write up the MemPool API in programmers guide and it should
be ready for HEAD after some testing. I have started a section for you
at the end showing the documentation syntax we use in other API
definitons.

--
Henrik