From: Jameson Miller Date: Mon, 23 Apr 2018 18:20:14 GMT Subject: Re: [PATCH v1 3/5] mem-pool: fill out functionality Message-ID: <5584faf6-23a0-366a-bd87-cf9e420f212c@gmail.com> In-Reply-To: <20180423104931.c465b6a260219c96feba41ff@google.com> On 04/23/2018 01:49 PM, Jonathan Tan wrote: > On Mon, 23 Apr 2018 13:27:09 -0400 > Jameson Miller wrote: > >>> This seems overly complicated - the struct mem_pool already has a linked >>> list of pages, so couldn't you create a custom page and insert it behind >>> the current front page instead whenever you needed a large-size page? >> >> Yes - that is another option. However, the linked list of pages includes >> memory that *could* have space for an allocation, while the "custom" >> region will never have left over memory that can be used for other >> allocations. When searching pages for memory to satisfy a request, there >> is no reason to search through the "custom" pages. There is a trade-off >> between complexity and implementation, so I am open to suggestions. >> >> This was discussed in [1], where it originally was implemented closer to >> what you describe here. >> >>> Also, when combining, there could be some wasted space on one of the >>> pages. I'm not sure if that's worth calling out, though. >> >> Yes, we bring over the whole page. However, these pages are now >> available for new allocations. >> >> [1] >> https://public-inbox.org/git/xmqqk1u2k91l.fsf@gitster-ct.c.googlers.com/ > > Ah, I didn't realize that the plan was to search over all pages when > allocating memory from the pool, instead of only searching the last > page. This seems like a departure from the fast-import.c way, where as > far as I can tell, new_object() searches only one page. If we do plan to > do this, searching all pages doesn't seem like a good idea to me, > especially since the objects we're storing in the pool are of similar > size. I see. However, the new_object() logic in fast-import is a different than the logic mem_pool was abstracting, and is not covered by the mem_pool functionality. The behavior of searching over all pages for one to satisfy the request existed previously and was not changed in the mem_pool implementation. > > If we decide to go ahead with searching all the pages, though, the > "custom" pages should probably be another linked list instead of an > array. > This is an option - I went with the current design because we only need pointers to a block of memory (as well tracking how large the allocation is for verification purposes). We don't necessarily need the extra overhead of a structure to track the linked list nodes, when it is provided by the existing array manipulation functions. I am open to feedback on this point, however.