Thank you for the thorough and detailed review - I very much appreciate it. > I > am OK if the structure of this series is to make that change after > these three steps we see here. I will add (back) the accounting of large memory blocks in a future patch series, taking into account your existing feedback. As this patch series did not have any consumers that "discarded" a memory pool, I decided to wait until I had a consumer that used this functionality. > Will queue; the proposed log for step 2/3 may want to be a bit > polished, though. I am away the rest of this week and next week, but will update this patch series when I get back (Week of April 9th). Thank you Jameson ________________________________________ From: Junio C Hamano <jch2355@xxxxxxxxx> on behalf of Junio C Hamano <gitster@xxxxxxxxx> Sent: Tuesday, March 27, 2018 12:43 PM To: Jameson Miller Cc: git@xxxxxxxxxxxxxxx; peff@xxxxxxxx Subject: Re: [PATCH v3 3/3] Move reusable parts of memory pool into its own file Jameson Miller <jamill@xxxxxxxxxxxxx> writes: > This moves the reusable parts of the memory pool logic used by > fast-import.c into its own file for use by other components. > > Signed-off-by: Jameson Miller <jamill@xxxxxxxxxxxxx> > --- > Makefile | 1 + > fast-import.c | 70 +---------------------------------------------------------- > mem-pool.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++ > mem-pool.h | 34 +++++++++++++++++++++++++++++ > 4 files changed, 91 insertions(+), 69 deletions(-) > create mode 100644 mem-pool.c > create mode 100644 mem-pool.h OK. This is indeed straight-forward line movements and nothing else, other than obviously a few static helpers are now extern. I said I'd anticipate that the allocation that bypasses the pool subsystem would want to become traceable by the pool subsystem, which would allow us to free() the pieces of memory allocated directly with xmalloc() in mem_pool_alloc() instead of leaking. I am OK if the structure of this series is to make that change after these three steps we see here. When that happens, it will start to make sense to bill the "this is too big so do not attempt to carve it out from block, and do not overallocate and make the remainder available for later requests" to the pool instance mem_pool_alloc() is working on, as that piece of memory is also known to a specific pool instance. After I wrote review for 2/3, I found out that you changed the meaning of total_allocd (which should probably be described in its log message). Unlike the original that counted "total", it now is used only for memory that is allocated directly by fast-import.c and does not account for memory obtained by calling mem-pool. The output routine is changed in 2/3 to add fi_mem_pool's pool_alloc to it, so billing oversized allocation that does *not* belong to any specific pool to _some_ pool and ignoring total_allocd would cancel things out. It still feels a bit fishy, but I think it is OK. So all in all, I think we are in no worse shape than the original (we call it "bug-to-bug compatible" ;-)), and successfully extracted a reusable piece in a separate file in a clean way so that we can refine and extend it further. Nicely done. Will queue; the proposed log for step 2/3 may want to be a bit polished, though. Thanks.
