git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jameson Miller <jamill@microsoft.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: "git@vger.kernel.org" <git@vger.kernel.org>,
	"peff@peff.net" <peff@peff.net>
Subject: Re: [PATCH v3 3/3] Move reusable parts of memory pool into its own file
Date: Thu, 29 Mar 2018 14:12:10 +0000	[thread overview]
Message-ID: <BL0PR2101MB11061477A78E978A65C0E4D4CEA20@BL0PR2101MB1106.namprd21.prod.outlook.com> (raw)
In-Reply-To: <xmqqlgeda2pd.fsf@gitster-ct.c.googlers.com>

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@gmail.com> on behalf of Junio C Hamano <gitster@pobox.com>
Sent: Tuesday, March 27, 2018 12:43 PM
To: Jameson Miller
Cc: git@vger.kernel.org; peff@peff.net
Subject: Re: [PATCH v3 3/3] Move reusable parts of memory pool into its own file 
 
Jameson Miller <jamill@microsoft.com> 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@microsoft.com>
> ---
>  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.

  reply	other threads:[~2018-03-29 14:12 UTC|newest]

Thread overview: 29+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-03-21 16:41 [PATCH 0/3] Extract memory pool logic into reusable component jameson.miller81
2018-03-21 16:41 ` [PATCH 1/3] fast-import: rename mem_pool to fi_mem_pool jameson.miller81
2018-03-21 16:41 ` [PATCH 2/3] Introduce a reusable memory pool type jameson.miller81
2018-03-21 16:41 ` [PATCH 3/3] fast-import: use built-in mem pool jameson.miller81
2018-03-21 19:27 ` [PATCH 0/3] Extract memory pool logic into reusable component Junio C Hamano
2018-03-23 14:44 ` [PATCH v2 " Jameson Miller
2018-03-23 14:44 ` [PATCH v2 1/5] fast-import: rename mem_pool type to mp_block Jameson Miller
2018-03-23 16:42   ` Junio C Hamano
2018-03-23 14:44 ` [PATCH v2 2/5] fast-import: introduce mem_pool type Jameson Miller
2018-03-23 17:15   ` Junio C Hamano
2018-03-23 14:44 ` [PATCH v2 3/5] fast-import: update pool_* functions to work on local pool Jameson Miller
2018-03-23 17:19   ` Junio C Hamano
2018-03-23 14:44 ` [PATCH v2 4/5] Move the reusable parts of memory pool into its own file Jameson Miller
2018-03-23 20:27   ` Junio C Hamano
2018-03-23 14:44 ` [PATCH v2 5/5] Expand implementation of mem-pool type Jameson Miller
2018-03-23 20:41   ` Junio C Hamano
2018-03-26 17:03 ` [PATCH v3 0/3] Extract memory pool logic into reusable component Jameson Miller
2018-03-26 17:03 ` [PATCH v3 1/3] fast-import: rename mem_pool type to mp_block Jameson Miller
2018-03-26 17:03 ` [PATCH v3 2/3] fast-import: introduce mem_pool type Jameson Miller
2018-03-26 17:34   ` Eric Sunshine
2018-03-27 16:09   ` Junio C Hamano
2018-03-26 17:03 ` [PATCH v3 3/3] Move reusable parts of memory pool into its own file Jameson Miller
2018-03-27 16:43   ` Junio C Hamano
2018-03-29 14:12     ` Jameson Miller [this message]
2018-04-11 18:37 ` [PATCH v4 1/3] fast-import: rename mem_pool type to mp_block Jameson Miller
2018-04-11 18:37 ` [PATCH v4 0/3] Extract memory pool logic into reusable component Jameson Miller
2018-04-17 16:43   ` Jameson Miller
2018-04-11 18:37 ` [PATCH v4 2/3] fast-import: introduce mem_pool type Jameson Miller
2018-04-11 18:37 ` [PATCH v4 3/3] Move reusable parts of memory pool into its own file Jameson Miller

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=BL0PR2101MB11061477A78E978A65C0E4D4CEA20@BL0PR2101MB1106.namprd21.prod.outlook.com \
    --to=jamill@microsoft.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=peff@peff.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).