git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / Atom feed
From: Jonathan Tan <jonathantanmy@google.com>
To: stolee@gmail.com
Cc: jonathantanmy@google.com, git@vger.kernel.org, peff@peff.net,
	mh@glandium.org
Subject: Re: [PATCH 3/6] index-pack: remove redundant child field
Date: Thu, 10 Oct 2019 12:02:29 -0700
Message-ID: <20191010190229.100750-1-jonathantanmy@google.com> (raw)
In-Reply-To: <67ca2f70-361a-bebd-7590-37337959552c@gmail.com>

> On 10/9/2019 7:44 PM, Jonathan Tan wrote:
> > Instead, recompute ancestry if we ever need to reclaim memory.
> 
> I find this message lacking in important details:
> 
> 1. Where do we recompute ancestry?
> 2. What are the performance implications of this change?
> 3. Why is it important that you construct a stack of deltas in prune_base_data()?

Thanks for taking a look at this. My original plan (as I perhaps badly
explained in the cover letter [1]) was to show the individual small
steps that I took to reach the end goal, each step still passing all
tests, in the hope that small steps are easier to understand than one
big one. Hence why I didn't explain much in this commit message (and
others), because I thought that I might have to squash them later. But
perhaps that is too confusing and I should have just squashed them in
the first place (and explain all the changes in the commit message -
it's +177 -198, which is not too big anyway).

To answer the question anyway, the short answer is that it doesn't
matter because I'm going to replace this mechanism in later patches. But
a longer answer:

 1. In prune_base_delta() (the stack of deltas you mention in question
    3).
 2. Slightly fewer pointer management during the normal course of
    operation, but an allocation if we ever need to reclaim memory.
 3. To recompute the ancestry. We have ancestry using the "base"
    pointer, but I need to iterate from the oldest to newest, so I
    create an array of all the "base" pointers and iterate in reverse.

[1] https://public-inbox.org/git/cover.1570663470.git.jonathantanmy@google.com/

> >  static void link_base_data(struct base_data *base, struct base_data *c)
> >  {
> > -	if (base)
> > -		base->child = c;
> > -	else
> > -		get_thread_data()->base_cache = c;
> > -
> >  	c->base = base;
> > -	c->child = NULL;
> >  	if (c->data)
> >  		get_thread_data()->base_cache_used += c->size;
> >  	prune_base_data(c);
> > @@ -408,11 +416,6 @@ static void link_base_data(struct base_data *base, struct base_data *c)
> >  
> >  static void unlink_base_data(struct base_data *c)
> >  {
> > -	struct base_data *base = c->base;
> > -	if (base)
> > -		base->child = NULL;
> > -	else
> > -		get_thread_data()->base_cache = NULL;
> >  	free_base_data(c);
> >  }
> 
> Seems like this method should be removed and all callers should
> call free_base_data() instead.

I agree, and did it in the next patch. Here I left it to preserve the
{link,unlink}_base_data symmetry.

  reply index

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-09 23:44 [RFC PATCH 0/6] Better threaded delta resolution in index-pack Jonathan Tan
2019-10-09 23:44 ` [PATCH 1/6] index-pack: unify threaded and unthreaded code Jonathan Tan
2019-10-17  6:20   ` Jeff King
2019-10-09 23:44 ` [PATCH 2/6] index-pack: remove redundant parameter Jonathan Tan
2019-10-17  6:21   ` Jeff King
2019-10-09 23:44 ` [PATCH 3/6] index-pack: remove redundant child field Jonathan Tan
2019-10-10 14:45   ` Derrick Stolee
2019-10-10 19:02     ` Jonathan Tan [this message]
2019-10-17  6:24       ` Jeff King
2019-10-17  6:26   ` Jeff King
2019-10-09 23:44 ` [PATCH 4/6] index-pack: calculate {ref,ofs}_{first,last} early Jonathan Tan
2019-10-17  6:30   ` Jeff King
2019-10-09 23:44 ` [PATCH 5/6] index-pack: make resolve_delta() assume base data Jonathan Tan
2019-10-17  6:32   ` Jeff King
2019-10-09 23:44 ` [PATCH 6/6] index-pack: make quantum of work smaller Jonathan Tan
2019-10-17  6:35   ` Jeff King
2019-10-17 20:17 ` [PATCH v2 0/7] Better threaded delta resolution in index-pack Jonathan Tan
2019-10-17 20:17   ` [PATCH v2 1/7] Documentation: deltaBaseCacheLimit is per-thread Jonathan Tan
2019-10-17 20:17   ` [PATCH v2 2/7] index-pack: unify threaded and unthreaded code Jonathan Tan
2019-10-17 20:17   ` [PATCH v2 3/7] index-pack: remove redundant parameter Jonathan Tan
2020-02-28  0:04     ` Josh Steadmon
2020-03-10 21:29       ` Jonathan Tan
2019-10-17 20:17   ` [PATCH v2 4/7] index-pack: remove redundant child field Jonathan Tan
2020-02-28  0:04     ` Josh Steadmon
2019-10-17 20:17   ` [PATCH v2 5/7] index-pack: calculate {ref,ofs}_{first,last} early Jonathan Tan
2019-10-17 20:17   ` [PATCH v2 6/7] index-pack: make resolve_delta() assume base data Jonathan Tan
2019-10-17 20:17   ` [PATCH v2 7/7] index-pack: make quantum of work smaller Jonathan Tan
2020-02-28  0:04     ` Josh Steadmon
2020-03-10 21:42       ` Jonathan Tan
2020-02-28  0:03   ` [PATCH v2 0/7] Better threaded delta resolution in index-pack Josh Steadmon
2020-03-10 21:45     ` Jonathan Tan

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=20191010190229.100750-1-jonathantanmy@google.com \
    --to=jonathantanmy@google.com \
    --cc=git@vger.kernel.org \
    --cc=mh@glandium.org \
    --cc=peff@peff.net \
    --cc=stolee@gmail.com \
    /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

git@vger.kernel.org list mirror (unofficial, one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Example config snippet for mirrors

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git