* Re: v1.5.4 plans
From: Kristian Høgsberg @ 2007-12-11 15:24 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vmysijhwq.fsf_-_@gitster.siamese.dyndns.org>
On Mon, 2007-12-10 at 14:37 -0800, Junio C Hamano wrote:
> * We have a handful deprecation notices in the draft release notes, but
> if I recall correctly, Nico wanted to add a few more. We need to
> come up with a wording that is easy to understand for the end users
> to decide which ancient versions will be affected.
Can we deprecate .git/branches?
Kristian
^ permalink raw reply
* Re: [PATCH] Invert numbers and names in the git-shortlog summary mode.
From: Nicolas Pitre @ 2007-12-11 15:34 UTC (permalink / raw)
To: Pierre Habouzit
Cc: Ingo Molnar, Jeff King, Jakub Narebski, Christian Couder, git,
Junio C Hamano
In-Reply-To: <20071211152412.GB15448@artemis.madism.org>
[-- Attachment #1: Type: TEXT/PLAIN, Size: 2011 bytes --]
On Tue, 11 Dec 2007, Pierre Habouzit wrote:
> On Tue, Dec 11, 2007 at 02:57:09PM +0000, Ingo Molnar wrote:
> >
> > * Pierre Habouzit <madcoder@debian.org> wrote:
> >
> > > Agreed, here is the patch that does that, and a sample output is:
> > >
> > > $ git shortlog -n -s -e HEAD -- builtin-commit.c
> > > 11 Junio C Hamano <gitster@pobox.com>
> > > 6 Johannes Schindelin <Johannes.Schindelin@gmx.de>
> > > 6 Kristian Høgsberg <krh@redhat.com>
> > > 2 Jeff King <peff@peff.net>
> > > 1 Alex Riesen <raa.lkml@gmail.com>
> > > 1 Pierre Habouzit <madcoder@debian.org>
> > > 1 Shawn Bohrer <shawn.bohrer@gmail.com>
> > > 1 Wincent Colaiuta <win@wincent.com>
> >
> > great - this looks really neat!
> >
> > btw., stupid question: why are the git-shortlog command line arguments
> > different from git-log? I got used to things like:
> >
> > git-log kernel/
> >
> > so for me it would be natural to just do:
> >
> > git-shortlog -n -s kernel/
> >
> > but this currently produces this output:
> >
> > $ git-shortlog -n -s kernel/
> > (reading log to summarize from standard input)
> >
> > which is quite a bit confusing to someone who'd like to keep as few
> > details of command line arguments in his head as possible :-)
>
> Because git-shortlog insists on you passing a reference first, HEAD is
> not implicit if you pass something that looks like a path first. This is
> arguably wrong. What you meant here is:
>
> $ git-shortlog -n -s HEAD kernel/
>
> The reason IIRC is that git-shortlog once only read things on stdin, and
> this keeps backward compatbility to `git-shortlog` without any
> arguments.
>
> Sometimes history hurts :) I don't think there is much we can do on a
> short timescale. Maybe the old way can be slowly deprecated, and then
> git-shortlog will be able to act like git-log.
At least, HEAD could be assumed by default when stdin is a tty.
Nicolas
^ permalink raw reply
* Re: Something is broken in repack
From: Jon Smirl @ 2007-12-11 15:36 UTC (permalink / raw)
To: Nicolas Pitre; +Cc: Junio C Hamano, gcc, Git Mailing List
In-Reply-To: <alpine.LFD.0.99999.0712110951070.555@xanadu.home>
On 12/11/07, Nicolas Pitre <nico@cam.org> wrote:
> On Tue, 11 Dec 2007, Nicolas Pitre wrote:
>
> > And yet, this is still missing the actual issue. The issue being that
> > the 2.1GB pack as a _source_ doesn't cause as much memory to be
> > allocated even if the _result_ pack ends up being the same.
> >
> > I was able to repack the 2.1GB pack on my machine which has 1GB of ram.
> > Now that it has been repacked, I can't repack it anymore, even when
> > single threaded, as it start crowling into swap fairly quickly. It is
> > really non intuitive and actually senseless that Git would require twice
> > as much RAM to deal with a pack that is 7 times smaller.
>
> OK, here's something else for you to try:
>
> core.deltabasecachelimit=0
> pack.threads=2
> pack.deltacachesize=1
>
> With that I'm able to repack the small gcc pack on my machine with 1GB
> of ram using:
>
> git repack -a -f -d --window=250 --depth=250
>
> and top reports a ~700m virt and ~500m res without hitting swap at all.
> It is only at 25% so far, but I was unable to get that far before.
>
> Would be curious to know what you get with 4 threads on your machine.
Changing those parameters really slowed down counting the objects. I
used to be able to count in 45 seconds now it took 130 seconds. I am
still have the Google allocator linked in.
4 threads, cumulative clock time
25% 200 seconds, 820/627M
55% 510 seconds, 1240/1000M - little late recording
75% 15 minutes, 1658/1500M
90% 22 minutes, 1974/1800M
it's still running but there is no significant change.
Are two types of allocations being mixed?
1) long term, global objects kept until the end of everything
2) volatile, private objects allocated only while the object is being
compressed and then freed
Separating these would make a big difference to the fragmentation
problem. Single threading probably wouldn't see a fragmentation
problem from mixing the allocation types.
When a thread is created it could allocated a private 20MB (or
whatever) pool. The volatile, private objects would come from that
pool. Long term objects would stay in the global pool. Since they are
long term they will just get laid down sequentially in memory.
Separating these allocation types make things way easier for malloc.
CPU time would be helped by removing some of the locking if possible.
--
Jon Smirl
jonsmirl@gmail.com
^ permalink raw reply
* Re: [PATCH] Invert numbers and names in the git-shortlog summary mode.
From: Ingo Molnar @ 2007-12-11 15:48 UTC (permalink / raw)
To: Pierre Habouzit, Jeff King, Jakub Narebski, Christian Couder, git,
Junio C Hamano <g
In-Reply-To: <20071211152412.GB15448@artemis.madism.org>
* Pierre Habouzit <madcoder@debian.org> wrote:
> > which is quite a bit confusing to someone who'd like to keep as few
> > details of command line arguments in his head as possible :-)
>
> Because git-shortlog insists on you passing a reference first, HEAD is
> not implicit if you pass something that looks like a path first. This
> is arguably wrong. What you meant here is:
>
> $ git-shortlog -n -s HEAD kernel/
>
> The reason IIRC is that git-shortlog once only read things on stdin,
> and this keeps backward compatbility to `git-shortlog` without any
> arguments.
>
> Sometimes history hurts :) I don't think there is much we can do on a
> short timescale. Maybe the old way can be slowly deprecated, and then
> git-shortlog will be able to act like git-log.
if we are growing legacies in git this fast it will turn itself into CVS
very quickly, give or take 20 years ;-) I think a straightforward usage
model is paramount, so phasing out such inconsistencies as early as
possible in the project's lifetime should be a priority IMHO. Git has a
very, very 'refreshing' approach to information management, and that
should permeate it all across. It's easy to be "fresh" in the beginning
of a project - maintaining freshness for years is a lot harder. (i dont
suggest to break compatibility, but to be aware of such inconsitencies
and make it a priority to get rid of them. It does not help that such
inconsistencies are only apparent to git newbies.)
another thing i noticed that bites git newbies is that there are
commands that do not do the "obvious" thing (to a newbie) and there are
commands that just dont tell what they are doing. It would be very
helpful if git was just a little bit more verbose about what it does.
I.e. use the standard output for ... actual output, and there will be an
instant and pleasant feedback loop between newbie and tool.
It should basically "teach" the user along the way about what is being
done, so that it quickly becomes an automatism. Especially where
concepts differ from legacy SCMs. When is the index updated, when do
files get modified, how branching works, etc.
for example, if i type "git-checkout" in a Linux kernel tree, it just
sits there for up to a minute, and "does nothing". That is totally
wrong, human-interaction wise. Then after a minute it just returns. What
happened? Why? Where? A newbie would then try "git-checkout -v", using
the well-established "verbose" flag, but that gives:
Usage: /usr/bin/git-checkout [-q] [-f] [-b <new_branch>] [-m] [<branch>] [<paths>...]
That's what a newbie asks. We are trying to turn the world of SCM's
upside down with git, and we should do that by teaching things gradually
and by being very explicit what happens and why, when a command is typed
;-)
i could go on and on with other examples. (should I ? ;-)
It's small, little details like that and if we fix a few dozen of them
it will do _wonders_ to user education, really. The best tools are the
ones that emit just about information to essentially give a tutorial to
the newbie about the philosophy and workings of the tool - without being
annoying to advanced users. It flattens the learning curve and increases
adoption rate enormously. It also helps people learn their way out of
the mental deformations that CVS causes ;-)
And we could do this by adding a -q "quiet" flag, to restore the current
'silent' behavior of git. I could then do this in my .bashrc:
alias git='git -q'
to get the old, quiet behavior. This issue is pretty much the only
conceptual advantage i can see of the competition like hg/mercurial ;-)
Ok, enough ranting for today i guess =B-)
Ingo
^ permalink raw reply
* Re: [PATCH] Invert numbers and names in the git-shortlog summary mode.
From: Pierre Habouzit @ 2007-12-11 16:07 UTC (permalink / raw)
To: Ingo Molnar
Cc: Jeff King, Jakub Narebski, Christian Couder, git, Junio C Hamano
In-Reply-To: <20071211154841.GA29805@elte.hu>
[-- Attachment #1: Type: text/plain, Size: 2289 bytes --]
On Tue, Dec 11, 2007 at 03:48:41PM +0000, Ingo Molnar wrote:
> if we are growing legacies in git this fast it will turn itself into CVS
> very quickly, give or take 20 years ;-) I think a straightforward usage
> model is paramount, so phasing out such inconsistencies as early as
> possible in the project's lifetime should be a priority IMHO. Git has a
> very, very 'refreshing' approach to information management, and that
> should permeate it all across. It's easy to be "fresh" in the beginning
> of a project - maintaining freshness for years is a lot harder. (i dont
> suggest to break compatibility, but to be aware of such inconsitencies
> and make it a priority to get rid of them. It does not help that such
> inconsistencies are only apparent to git newbies.)
Well that's what deprecation is for, but you cannot do that on short
timeframes.
> for example, if i type "git-checkout" in a Linux kernel tree, it just
> sits there for up to a minute, and "does nothing". That is totally
> wrong, human-interaction wise. Then after a minute it just returns. What
> happened? Why? Where? A newbie would then try "git-checkout -v", using
> the well-established "verbose" flag, but that gives:
>
> Usage: /usr/bin/git-checkout [-q] [-f] [-b <new_branch>] [-m] [<branch>] [<paths>...]
not anymore:
$ git checkout -v
error: unknown switch `v'
usage: git-branch [options] [<branch>] [<paths>...]
-b ... create a new branch started at <branch>
-l create the new branchs reflog
--track tells if the new branch should track the remote branch
-f proceed even if the index or working tree is not HEAD
-m performa three-way merge on local modifications if needed
-q, --quiet be quiet
Not all commands are migrated to this new scheme though.
The next git has a _lot_ of things done better wrt UI and such issues.
Though some backward incompatible changes must be introduced with the
proper deprecation warnings, so that people can adapt.
--
·O· Pierre Habouzit
··O madcoder@debian.org
OOO http://www.madism.org
[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply
* [PATCH] stupid typo in git-checkout.sh
From: Pierre Habouzit @ 2007-12-11 16:09 UTC (permalink / raw)
To: Git ML, Junio C Hamano
[-- Attachment #1: Type: text/plain, Size: 688 bytes --]
Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---
thanks to Ingo Molnar that made me spot this one while answering to
him eagerly that git-checkout had a fancy usage now ...
git-checkout.sh | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/git-checkout.sh b/git-checkout.sh
index f6d58ac..200f9bc 100755
--- a/git-checkout.sh
+++ b/git-checkout.sh
@@ -2,7 +2,7 @@
OPTIONS_KEEPDASHDASH=t
OPTIONS_SPEC="\
-git-branch [options] [<branch>] [<paths>...]
+git-checkout [options] [<branch>] [<paths>...]
--
b= create a new branch started at <branch>
l create the new branchs reflog
--
1.5.3.7.2248.g93a2
[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply related
* Re: [PATCH] Invert numbers and names in the git-shortlog summary mode.
From: Pierre Habouzit @ 2007-12-11 16:11 UTC (permalink / raw)
To: Ingo Molnar, Jeff King, Jakub Narebski, Christian Couder, git,
Junio C Hamano
In-Reply-To: <20071211160744.GE15448@artemis.madism.org>
[-- Attachment #1: Type: text/plain, Size: 1894 bytes --]
On Tue, Dec 11, 2007 at 04:07:45PM +0000, Pierre Habouzit wrote:
> On Tue, Dec 11, 2007 at 03:48:41PM +0000, Ingo Molnar wrote:
> > if we are growing legacies in git this fast it will turn itself into CVS
> > very quickly, give or take 20 years ;-) I think a straightforward usage
> > model is paramount, so phasing out such inconsistencies as early as
> > possible in the project's lifetime should be a priority IMHO. Git has a
> > very, very 'refreshing' approach to information management, and that
> > should permeate it all across. It's easy to be "fresh" in the beginning
> > of a project - maintaining freshness for years is a lot harder. (i dont
> > suggest to break compatibility, but to be aware of such inconsitencies
> > and make it a priority to get rid of them. It does not help that such
> > inconsistencies are only apparent to git newbies.)
>
> Well that's what deprecation is for, but you cannot do that on short
> timeframes.
>
> > for example, if i type "git-checkout" in a Linux kernel tree, it just
> > sits there for up to a minute, and "does nothing". That is totally
> > wrong, human-interaction wise. Then after a minute it just returns. What
> > happened? Why? Where? A newbie would then try "git-checkout -v", using
> > the well-established "verbose" flag, but that gives:
> >
> > Usage: /usr/bin/git-checkout [-q] [-f] [-b <new_branch>] [-m] [<branch>] [<paths>...]
>
> not anymore:
>
> $ git checkout -v
> error: unknown switch `v'
> usage: git-branch [options] [<branch>] [<paths>...]
^^^^^^
okay I felt stupid while reading this on list, a patch fixing this
stupid lapsus has just been sent. Doh.
--
·O· Pierre Habouzit
··O madcoder@debian.org
OOO http://www.madism.org
[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply
* [PATCH Resend] Stupid typos in git-checkout.sh
From: Pierre Habouzit @ 2007-12-11 16:13 UTC (permalink / raw)
To: Git ML, Junio C Hamano
In-Reply-To: <20071211160925.GF15448@artemis.madism.org>
Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---
Woops there was another typo in the usage, fix it at the same time.
git-checkout.sh | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/git-checkout.sh b/git-checkout.sh
index f6d58ac..d3e1bd0 100755
--- a/git-checkout.sh
+++ b/git-checkout.sh
@@ -2,13 +2,13 @@
OPTIONS_KEEPDASHDASH=t
OPTIONS_SPEC="\
-git-branch [options] [<branch>] [<paths>...]
+git-checkout [options] [<branch>] [<paths>...]
--
b= create a new branch started at <branch>
l create the new branchs reflog
track tells if the new branch should track the remote branch
f proceed even if the index or working tree is not HEAD
-m performa three-way merge on local modifications if needed
+m perform a three-way merge on local modifications if needed
q,quiet be quiet
"
SUBDIRECTORY_OK=Sometimes
--
1.5.3.7.2248.g93a2
^ permalink raw reply related
* Re: Something is broken in repack
From: Nicolas Pitre @ 2007-12-11 16:20 UTC (permalink / raw)
To: Jon Smirl; +Cc: Junio C Hamano, gcc, Git Mailing List
In-Reply-To: <alpine.LFD.0.99999.0712110951070.555@xanadu.home>
On Tue, 11 Dec 2007, Nicolas Pitre wrote:
> OK, here's something else for you to try:
>
> core.deltabasecachelimit=0
> pack.threads=2
> pack.deltacachesize=1
>
> With that I'm able to repack the small gcc pack on my machine with 1GB
> of ram using:
>
> git repack -a -f -d --window=250 --depth=250
>
> and top reports a ~700m virt and ~500m res without hitting swap at all.
> It is only at 25% so far, but I was unable to get that far before.
Well, around 55% memory usage skyrocketed to 1.6GB and the system went
deep into swap. So I restarted it with no threads.
Nicolas (even more puzzled)
^ permalink raw reply
* Re: Something is broken in repack
From: Jon Smirl @ 2007-12-11 16:21 UTC (permalink / raw)
To: Nicolas Pitre; +Cc: Junio C Hamano, gcc, Git Mailing List
In-Reply-To: <alpine.LFD.0.99999.0712111117440.555@xanadu.home>
On 12/11/07, Nicolas Pitre <nico@cam.org> wrote:
> On Tue, 11 Dec 2007, Nicolas Pitre wrote:
>
> > OK, here's something else for you to try:
> >
> > core.deltabasecachelimit=0
> > pack.threads=2
> > pack.deltacachesize=1
> >
> > With that I'm able to repack the small gcc pack on my machine with 1GB
> > of ram using:
> >
> > git repack -a -f -d --window=250 --depth=250
> >
> > and top reports a ~700m virt and ~500m res without hitting swap at all.
> > It is only at 25% so far, but I was unable to get that far before.
>
> Well, around 55% memory usage skyrocketed to 1.6GB and the system went
> deep into swap. So I restarted it with no threads.
>
> Nicolas (even more puzzled)
On the plus side you are seeing what I see, so it proves I am not imagining it.
--
Jon Smirl
jonsmirl@gmail.com
^ permalink raw reply
* Re: [PATCH] Adding menu for Emacs git.el
From: Remi Vanicat @ 2007-12-11 16:27 UTC (permalink / raw)
To: Alexandre Julliard; +Cc: Junio C Hamano, git
In-Reply-To: <878x41idvb.fsf@wine.dyndns.org>
2007/12/11, Alexandre Julliard <julliard@winehq.org>:
> Junio C Hamano <gitster@pobox.com> writes:
>
> > Alex, what is the current status of this patch?
>
> Patch v3 looked good but the patch file was corrupted. Here's the latest
> version, I have moved things around a bit to use only one menu and to be
> more similar to pcl-cvs.
>
It look good
^ permalink raw reply
* Re: Something is broken in repack
From: Linus Torvalds @ 2007-12-11 16:33 UTC (permalink / raw)
To: Jon Smirl; +Cc: Nicolas Pitre, Junio C Hamano, gcc, Git Mailing List
In-Reply-To: <9e4733910712102301p5e6c4165v6afb32d157478828@mail.gmail.com>
On Tue, 11 Dec 2007, Jon Smirl wrote:
>
> So why does our threaded code take 20 CPU minutes longer (12%) to run
> than the same code with a single thread?
Threaded code *always* takes more CPU time. The only thing you can hope
for is a wall-clock reduction. You're seeing probably a combination of
(a) more cache misses
(b) bigger dataset active at a time
and a probably fairly miniscule
(c) threading itself tends to have some overheads.
> Q6600 is just two E6600s in the same package, the caches are not shared.
Sure they are shared. They're just not *entirely* shared. But they are
shared between each two cores, so each thread essentially has only half
the cache they had with the non-threaded version.
Threading is *not* a magic solution to all problems. It gives you
potentially twice the CPU power, but there are real downsides that you
should keep in mind.
> Why does the threaded code need 2.24GB (google allocator, 2.85GB gcc)
> with 4 threads? But only need 950MB with one thread? Where's the extra
> gigabyte going?
I suspect that it's really simple: you have a few rather big files in the
gcc history, with deep delta chains. And what happens when you have four
threads running at the same time is that they all need to keep all those
objects that they are working on - and their hash state - in memory at the
same time!
So if you want to use more threads, that _forces_ you to have a bigger
memory footprint, simply because you have more "live" objects that you
work on. Normally, that isn't much of a problem, since most source files
are small, but if you have a few deep delta chains on big files, both the
delta chain itself is going to use memory (you may have limited the size
of the cache, but it's still needed for the actual delta generation, so
it's not like the memory usage went away).
That said, I suspect there are a few things fighting you:
- threading is hard. I haven't looked a lot at the changes Nico did to do
a threaded object packer, but what I've seen does not convince me it is
correct. The "trg_entry" accesses are *mostly* protected with
"cache_lock", but nothing else really seems to be, so quite frankly, I
wouldn't trust the threaded version very much. It's off by default, and
for a good reason, I think.
For example: the packing code does this:
if (!src->data) {
read_lock();
src->data = read_sha1_file(src_entry->idx.sha1, &type, &sz);
read_unlock();
...
and that's racy. If two threads come in at roughly the same time and
see a NULL src->data, theÿ́'ll both get the lock, and they'll both
(serially) try to fill it in. It will all *work*, but one of them will
have done unnecessary work, and one of them will have their result
thrown away and leaked.
Are you hitting issues like this? I dunno. The object sorting means
that different threads normally shouldn't look at the same objects (not
even the sources), so probably not, but basically, I wouldn't trust the
threading 100%. It needs work, and it needs to stay off by default.
- you're working on a problem that isn't really even worth optimizing
that much. The *normal* case is to re-use old deltas, which makes all
of the issues you are fighting basically go away (because you only have
a few _incremental_ objects that need deltaing).
In other words: the _real_ optimizations have already been done, and
are done elsewhere, and are much smarter (the best way to optimize X is
not to make X run fast, but to avoid doing X in the first place!). The
thing you are trying to work with is the one-time-only case where you
explicitly disable that big and important optimization, and then you
complain about the end result being slow!
It's like saying that you're compiling with extreme debugging and no
optimizations, and then complaining that the end result doesn't run as
fast as if you used -O2. Except this is a hundred times worse, because
you literally asked git to do the really expensive thing that it really
really doesn't want to do ;)
> Is there another allocator to try? One that combines Google's
> efficiency with gcc's speed?
See above: I'd look around at threading-related bugs and check the way we
lock (or don't) accesses.
Linus
^ permalink raw reply
* [PATCH] Fix a typo in checkout.sh and cleanup one-line help messages
From: Johannes Sixt @ 2007-12-11 16:36 UTC (permalink / raw)
To: Pierre Habouzit; +Cc: Git ML, Junio C Hamano
In-Reply-To: <20071211161305.GH15448@artemis.madism.org>
This also shortens option descriptions to fit in 80 columns.
Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
Pierre Habouzit schrieb:
> Signed-off-by: Pierre Habouzit <madcoder@debian.org>
> ---
>
> Woops there was another typo in the usage, fix it at the same time.
>
> git-checkout.sh | 4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
Funny, I had this in my tree since a few days, although embarassingly,
I hadn't spotted the "git-branch" typo :-/
-- Hannes
git-checkout.sh | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/git-checkout.sh b/git-checkout.sh
index f6d58ac..5621c69 100755
--- a/git-checkout.sh
+++ b/git-checkout.sh
@@ -2,13 +2,13 @@
OPTIONS_KEEPDASHDASH=t
OPTIONS_SPEC="\
-git-branch [options] [<branch>] [<paths>...]
+git-checkout [options] [<branch>] [<paths>...]
--
b= create a new branch started at <branch>
-l create the new branchs reflog
-track tells if the new branch should track the remote branch
+l create the new branch's reflog
+track arrange that the new branch tracks the remote branch
f proceed even if the index or working tree is not HEAD
-m performa three-way merge on local modifications if needed
+m merge local modifications into the new branch
q,quiet be quiet
"
SUBDIRECTORY_OK=Sometimes
--
1.5.3.7.1079.gecca
^ permalink raw reply related
* Re: [PATCH 2/2] pack-objects: fix threaded load balancing
From: Johannes Sixt @ 2007-12-11 17:02 UTC (permalink / raw)
To: Nicolas Pitre; +Cc: Junio C Hamano, git, Jon Smirl
In-Reply-To: <alpine.LFD.0.99999.0712080000120.555@xanadu.home>
Nicolas Pitre schrieb:
> @@ -1612,10 +1620,10 @@ static void *threaded_find_deltas(void *arg)
> pthread_mutex_lock(&data_ready);
> pthread_mutex_unlock(&data_request);
>
> - if (!me->list_size)
> + if (!me->remaining)
> return NULL;
>
> - find_deltas(me->list, me->list_size,
> + find_deltas(me->list, &me->remaining,
> me->window, me->depth, me->processed);
> }
> }
This hunk caught my attention. &data_ready is locked, but not released in
this function.
Looking more closely at the code surrounding this hunk, it seems that the
lock is released in a *different* thread than the one that locked it. This
works on Linux, but is not portable. We will have to use condition variables
like every one else does in a producer-consumer-like scenario.
-- Hannes
^ permalink raw reply
* Re: Something is broken in repack
From: Nicolas Pitre @ 2007-12-11 17:21 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Jon Smirl, Junio C Hamano, gcc, Git Mailing List
In-Reply-To: <alpine.LFD.0.9999.0712110806540.25032@woody.linux-foundation.org>
[-- Attachment #1: Type: TEXT/PLAIN, Size: 4146 bytes --]
On Tue, 11 Dec 2007, Linus Torvalds wrote:
> That said, I suspect there are a few things fighting you:
>
> - threading is hard. I haven't looked a lot at the changes Nico did to do
> a threaded object packer, but what I've seen does not convince me it is
> correct. The "trg_entry" accesses are *mostly* protected with
> "cache_lock", but nothing else really seems to be, so quite frankly, I
> wouldn't trust the threaded version very much. It's off by default, and
> for a good reason, I think.
I beg to differ (of course, since I always know precisely what I do, and
like you, my code never has bugs).
Seriously though, the trg_entry has not to be protected at all. Why?
Simply because each thread has its own exclusive set of objects which no
other threads ever mess with. They never overlap.
> For example: the packing code does this:
>
> if (!src->data) {
> read_lock();
> src->data = read_sha1_file(src_entry->idx.sha1, &type, &sz);
> read_unlock();
> ...
>
> and that's racy. If two threads come in at roughly the same time and
> see a NULL src->data, theÿ́'ll both get the lock, and they'll both
> (serially) try to fill it in. It will all *work*, but one of them will
> have done unnecessary work, and one of them will have their result
> thrown away and leaked.
No. Once again, it is impossible for two threads to ever see the same
src->data at all. The lock is there simply because read_sha1_file() is
not reentrant.
> Are you hitting issues like this? I dunno. The object sorting means
> that different threads normally shouldn't look at the same objects (not
> even the sources), so probably not, but basically, I wouldn't trust the
> threading 100%. It needs work, and it needs to stay off by default.
For now it is, but I wouldn't say it really needs significant work at
this point. The latest thread patches were more about tuning than
correctness.
What the threading could be doing, though, is uncovering some other
bugs, like in the pack mmap windowing code for example. Although that
code is serialized by the read lock above, the fact that multiple
threads are hammering on it in turns means that the mmap window is
possibly seeking back and forth much more often than otherwise, possibly
leaking something in the process.
> - you're working on a problem that isn't really even worth optimizing
> that much. The *normal* case is to re-use old deltas, which makes all
> of the issues you are fighting basically go away (because you only have
> a few _incremental_ objects that need deltaing).
>
> In other words: the _real_ optimizations have already been done, and
> are done elsewhere, and are much smarter (the best way to optimize X is
> not to make X run fast, but to avoid doing X in the first place!). The
> thing you are trying to work with is the one-time-only case where you
> explicitly disable that big and important optimization, and then you
> complain about the end result being slow!
>
> It's like saying that you're compiling with extreme debugging and no
> optimizations, and then complaining that the end result doesn't run as
> fast as if you used -O2. Except this is a hundred times worse, because
> you literally asked git to do the really expensive thing that it really
> really doesn't want to do ;)
Linus, please pay attention to the _actual_ important issue here.
Sure I've been tuning the threading code in parallel to the attempt to
debug this memory usage issue.
BUT. The point is that repacking the gcc repo using "git repack -a -f
--window=250" has a radically different memory usage profile whether you
do the repack on the earlier 2.1GB pack or the later 300MB pack.
_That_ is the issue. Ironically, it is the 300MB pack that causes the
repack to blow memory usage out of proportion.
And in both cases, the threading code has to do the same
work whether or not the original pack was densely packed or not since -f
throws away every existing deltas anyway.
So something is fishy elsewhere than in the packing code.
Nicolas
^ permalink raw reply
* Re: Something is broken in repack
From: David Miller @ 2007-12-11 17:24 UTC (permalink / raw)
To: nico; +Cc: torvalds, jonsmirl, gitster, gcc, git
In-Reply-To: <alpine.LFD.0.99999.0712111202470.555@xanadu.home>
From: Nicolas Pitre <nico@cam.org>
Date: Tue, 11 Dec 2007 12:21:11 -0500 (EST)
> BUT. The point is that repacking the gcc repo using "git repack -a -f
> --window=250" has a radically different memory usage profile whether you
> do the repack on the earlier 2.1GB pack or the later 300MB pack.
If you repack on the smaller pack file, git has to expand more stuff
internally in order to search the deltas, whereas with the larger pack
file I bet git has to less often undelta'ify to get base objects blobs
for delta search.
In fact that behavior makes perfect sense to me and I don't understand
GIT internals very well :-)
^ permalink raw reply
* Re: [PATCH 2/2] pack-objects: fix threaded load balancing
From: Nicolas Pitre @ 2007-12-11 17:28 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Junio C Hamano, git, Jon Smirl
In-Reply-To: <475EC2AB.60702@viscovery.net>
On Tue, 11 Dec 2007, Johannes Sixt wrote:
> Nicolas Pitre schrieb:
> > @@ -1612,10 +1620,10 @@ static void *threaded_find_deltas(void *arg)
> > pthread_mutex_lock(&data_ready);
> > pthread_mutex_unlock(&data_request);
> >
> > - if (!me->list_size)
> > + if (!me->remaining)
> > return NULL;
> >
> > - find_deltas(me->list, me->list_size,
> > + find_deltas(me->list, &me->remaining,
> > me->window, me->depth, me->processed);
> > }
> > }
>
> This hunk caught my attention. &data_ready is locked, but not released in
> this function.
>
> Looking more closely at the code surrounding this hunk, it seems that the
> lock is released in a *different* thread than the one that locked it. This
> works on Linux, but is not portable. We will have to use condition variables
> like every one else does in a producer-consumer-like scenario.
Are you willing to make a patch for it?
Nicolas
^ permalink raw reply
* Re: Something is broken in repack
From: Daniel Berlin @ 2007-12-11 17:28 UTC (permalink / raw)
To: Jon Smirl; +Cc: Nicolas Pitre, Junio C Hamano, gcc, Git Mailing List
In-Reply-To: <9e4733910712102301p5e6c4165v6afb32d157478828@mail.gmail.com>
On 12/11/07, Jon Smirl <jonsmirl@gmail.com> wrote:
>
> Total CPU time 196 CPU minutes vs 190 for gcc. Google's claims of
> being faster are not true.
Depends on your allocation patterns. For our apps, it certainly is :)
Of course, i don't know if we've updated the external allocator in a
while, i'll bug the people in charge of it.
^ permalink raw reply
* git annotate runs out of memory
From: Daniel Berlin @ 2007-12-11 17:33 UTC (permalink / raw)
To: git
On the gcc repository (which is now a 234 meg pack for me), git
annotate ChangeLog takes > 800 meg of memory (I stopped it at about
1.6 gig, since it started swapping my machine).
I assume it will run out of memory. I stopped it after 2 minutes.
Mercurial, on the same file, takes 50 meg and 30 seconds.
git annotate fold-const.c takes 300 meg of memory and takes > 30 seconds.
Mercurial, on the same file takes 50 meg of memory and 10 seconds.
svn takes 15 seconds and 20 meg of memory.
I have excluded the mmap memory from mmap'ing the pack/file (in
git/mercurial respectively).
Annotate is treasured by gcc developers (this was a key sticking point
in svn conversion).
Having an annotate that is 2x slower and takes 15x memory would not
fly (regardless of how good the results are).
This seems to be a common problem with git. It seems to use a lot of
memory to perform common operations on the gcc repository (even though
it is faster in some cases than hg).
--Dan
^ permalink raw reply
* Re: Something is broken in repack
From: Nicolas Pitre @ 2007-12-11 17:44 UTC (permalink / raw)
To: David Miller; +Cc: Linus Torvalds, jonsmirl, Junio C Hamano, gcc, git
In-Reply-To: <20071211.092402.266823343.davem@davemloft.net>
On Tue, 11 Dec 2007, David Miller wrote:
> From: Nicolas Pitre <nico@cam.org>
> Date: Tue, 11 Dec 2007 12:21:11 -0500 (EST)
>
> > BUT. The point is that repacking the gcc repo using "git repack -a -f
> > --window=250" has a radically different memory usage profile whether you
> > do the repack on the earlier 2.1GB pack or the later 300MB pack.
>
> If you repack on the smaller pack file, git has to expand more stuff
> internally in order to search the deltas, whereas with the larger pack
> file I bet git has to less often undelta'ify to get base objects blobs
> for delta search.
Of course. I came to that conclusion two days ago. And despite being
pretty familiar with the involved code (I wrote part of it myself) I
just can't spot anything wrong with it so far.
But somehow the threading code keep distracting people from that issue
since it gets to do the same work whether or not the source pack is
densely packed or not.
Nicolas
(who wish he had access to a much faster machine to investigate this issue)
^ permalink raw reply
* Re: git annotate runs out of memory
From: Nicolas Pitre @ 2007-12-11 17:47 UTC (permalink / raw)
To: Daniel Berlin; +Cc: git
In-Reply-To: <4aca3dc20712110933i636342fbifb15171d3e3cafb3@mail.gmail.com>
On Tue, 11 Dec 2007, Daniel Berlin wrote:
> On the gcc repository (which is now a 234 meg pack for me), git
> annotate ChangeLog takes > 800 meg of memory (I stopped it at about
> 1.6 gig, since it started swapping my machine).
> I assume it will run out of memory. I stopped it after 2 minutes.
And I bet this is the exact same issue as the repack one.
Do you still have the 2.1GB pack around? I bet annotate would eat much
less memory in that case.
Nicolas
^ permalink raw reply
* Re: git annotate runs out of memory
From: Daniel Berlin @ 2007-12-11 17:53 UTC (permalink / raw)
To: Nicolas Pitre; +Cc: git
In-Reply-To: <alpine.LFD.0.99999.0712111245260.555@xanadu.home>
On 12/11/07, Nicolas Pitre <nico@cam.org> wrote:
> On Tue, 11 Dec 2007, Daniel Berlin wrote:
>
> > On the gcc repository (which is now a 234 meg pack for me), git
> > annotate ChangeLog takes > 800 meg of memory (I stopped it at about
> > 1.6 gig, since it started swapping my machine).
> > I assume it will run out of memory. I stopped it after 2 minutes.
>
> And I bet this is the exact same issue as the repack one.
>
> Do you still have the 2.1GB pack around? I bet annotate would eat much
> less memory in that case.
I do not, but i could remake it in a few days if it would help
^ permalink raw reply
* Re: [PATCH] Invert numbers and names in the git-shortlog summary mode.
From: Junio C Hamano @ 2007-12-11 17:58 UTC (permalink / raw)
To: Pierre Habouzit
Cc: Ingo Molnar, Jeff King, Jakub Narebski, Christian Couder, git
In-Reply-To: <20071211144351.GA15448@artemis.madism.org>
Pierre Habouzit <madcoder@debian.org> writes:
> Also make it `cut` friendly using a tab to separate the numbers and names.
>
> Signed-off-by: Pierre Habouzit <madcoder@debian.org>
> ---
>
> On Tue, Dec 11, 2007 at 02:05:08PM +0000, Ingo Molnar wrote:
> > please switch around the column too so that the commit count comes
> > first, this is way too ugly:
> >
> > Junio C Hamano: 4826
> > Shawn O. Pearce: 1146
> > Linus Torvalds: 950
> [...]
>
> Agreed, here is the patch that does that, and a sample output is:
>
> $ git shortlog -n -s -e HEAD -- builtin-commit.c
> 11 Junio C Hamano <gitster@pobox.com>
> 6 Johannes Schindelin <Johannes.Schindelin@gmx.de>
> 6 Kristian Høgsberg <krh@redhat.com>
> 2 Jeff King <peff@peff.net>
> 1 Alex Riesen <raa.lkml@gmail.com>
Thanks. Looks good.
^ permalink raw reply
* Re: git annotate runs out of memory
From: Nicolas Pitre @ 2007-12-11 18:01 UTC (permalink / raw)
To: Daniel Berlin; +Cc: git
In-Reply-To: <4aca3dc20712110953h13e3c33ftb310609bbac6a0a8@mail.gmail.com>
On Tue, 11 Dec 2007, Daniel Berlin wrote:
> On 12/11/07, Nicolas Pitre <nico@cam.org> wrote:
> > On Tue, 11 Dec 2007, Daniel Berlin wrote:
> >
> > > On the gcc repository (which is now a 234 meg pack for me), git
> > > annotate ChangeLog takes > 800 meg of memory (I stopped it at about
> > > 1.6 gig, since it started swapping my machine).
> > > I assume it will run out of memory. I stopped it after 2 minutes.
> >
> > And I bet this is the exact same issue as the repack one.
> >
> > Do you still have the 2.1GB pack around? I bet annotate would eat much
> > less memory in that case.
>
> I do not, but i could remake it in a few days if it would help
Well, depending on the amount of RAM in your machine, you might even not
be able to remake it at the moment. I currently can't reproduce it
myself due to the same out-of-memory issue.
Nicolas
^ permalink raw reply
* Re: [PATCH] Invert numbers and names in the git-shortlog summary mode.
From: Junio C Hamano @ 2007-12-11 18:09 UTC (permalink / raw)
To: Nicolas Pitre
Cc: Pierre Habouzit, Ingo Molnar, Jeff King, Jakub Narebski,
Christian Couder, git
In-Reply-To: <alpine.LFD.0.99999.0712111032530.555@xanadu.home>
Nicolas Pitre <nico@cam.org> writes:
> On Tue, 11 Dec 2007, Pierre Habouzit wrote:
> ...
>> Sometimes history hurts :) I don't think there is much we can do on a
>> short timescale. Maybe the old way can be slowly deprecated, and then
>> git-shortlog will be able to act like git-log.
>
> At least, HEAD could be assumed by default when stdin is a tty.
I'd say I agree.
-- >8 --
shortlog: default ot HEAD when standard input is a tty
Instead of warning the user that it is expecting git log output from the
standard input (and instructing to type the log from the keyboard),
default to traverse from HEAD when there is no rev parameter given.
This factors out a useful helper "add_head()" from builtin-diff.c to a
more appropriate place revision.c while renaming it to more descriptive
name add_head_to_pending(), as that is what the function is about.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin-shortlog.c | 5 +++--
builtin-diff.c | 14 +-------------
builtin-log.c | 5 +----
revision.c | 12 ++++++++++++
revision.h | 2 ++
5 files changed, 19 insertions(+), 19 deletions(-)
diff --git a/builtin-shortlog.c b/builtin-shortlog.c
index 90666cb..3d8d709 100644
--- a/builtin-shortlog.c
+++ b/builtin-shortlog.c
@@ -249,9 +249,10 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix)
read_mailmap(&mailmap, ".mailmap", &common_repo_prefix);
+ /* assume HEAD if from a tty */
+ if (!rev.pending.nr && isatty(0))
+ add_head_to_pending(&rev);
if (rev.pending.nr == 0) {
- if (isatty(0))
- fprintf(stderr, "(reading log to summarize from standard input)\n");
read_from_stdin(&list);
}
else
diff --git a/builtin-diff.c b/builtin-diff.c
index 1b61599..55fb84c 100644
--- a/builtin-diff.c
+++ b/builtin-diff.c
@@ -176,18 +176,6 @@ static int builtin_diff_combined(struct rev_info *revs,
return 0;
}
-void add_head(struct rev_info *revs)
-{
- unsigned char sha1[20];
- struct object *obj;
- if (get_sha1("HEAD", sha1))
- return;
- obj = parse_object(sha1);
- if (!obj)
- return;
- add_pending_object(revs, obj, "HEAD");
-}
-
static void refresh_index_quietly(void)
{
struct lock_file *lock_file;
@@ -272,7 +260,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
if (!strcmp(arg, "--"))
break;
else if (!strcmp(arg, "--cached")) {
- add_head(&rev);
+ add_head_to_pending(&rev);
if (!rev.pending.nr)
die("No HEAD commit to compare with (yet)");
break;
diff --git a/builtin-log.c b/builtin-log.c
index e1f1cf6..d375c9d 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -18,9 +18,6 @@
static int default_show_root = 1;
static const char *fmt_patch_subject_prefix = "PATCH";
-/* this is in builtin-diff.c */
-void add_head(struct rev_info *revs);
-
static void add_name_decoration(const char *prefix, const char *name, struct object *obj)
{
int plen = strlen(prefix);
@@ -746,7 +743,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
* does not have.
*/
rev.pending.objects[0].item->flags |= UNINTERESTING;
- add_head(&rev);
+ add_head_to_pending(&rev);
}
/*
* Otherwise, it is "format-patch -22 HEAD", and/or
diff --git a/revision.c b/revision.c
index 2a59035..7e2f4f1 100644
--- a/revision.c
+++ b/revision.c
@@ -139,6 +139,18 @@ void add_pending_object(struct rev_info *revs, struct object *obj, const char *n
add_pending_object_with_mode(revs, obj, name, S_IFINVALID);
}
+void add_head_to_pending(struct rev_info *revs)
+{
+ unsigned char sha1[20];
+ struct object *obj;
+ if (get_sha1("HEAD", sha1))
+ return;
+ obj = parse_object(sha1);
+ if (!obj)
+ return;
+ add_pending_object(revs, obj, "HEAD");
+}
+
static struct object *get_reference(struct rev_info *revs, const char *name, const unsigned char *sha1, unsigned int flags)
{
struct object *object;
diff --git a/revision.h b/revision.h
index 992e1e9..8572315 100644
--- a/revision.h
+++ b/revision.h
@@ -130,6 +130,8 @@ extern void add_object(struct object *obj,
extern void add_pending_object(struct rev_info *revs, struct object *obj, const char *name);
+extern void add_head_to_pending(struct rev_info *);
+
enum commit_action {
commit_ignore,
commit_show,
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox