Re: [PATCH 3/3] refs: use strings directly in find_containing_dir()

[Michael Haggerty <mhagger@xxxxxxxxxxxx>]

On 05/23/2012 07:15 PM, René Scharfe wrote:
> Am 23.05.2012 18:56, schrieb Junio C Hamano:
> > René Scharfe<rene.scharfe@xxxxxxxxxxxxxx>   writes:
> >
> > > Am 23.05.2012 00:18, schrieb Junio C Hamano:
> > > > René Scharfe<rene.scharfe@xxxxxxxxxxxxxx>    writes:
> > > >
> > > > > What has git grep to do with refs?  It checks if the path in the command
> > > > > above is a ref, which makes it iterate over all of them..
> > > >
> > > > Do you mean:
> > > >
> > > > 	/* Is it a rev? */
> > > >            get_sha1()
> > > >            ->    ...
> > > >              ->    get_sha1_basic()
> > > >                ->    dwim_ref()
> > > >
> > > > callpath?
> > >
> > > Yes, indeed.  Hmm, this is done even if the paths come after a
> > > double-dash.  Anyway, I don't consider the check to be a performance
> > > issue, just a quick way to test the allocation count that i stumbled
> > > upon while working on the recent grep patches.
> >
> > I was merely reacting "iterate over all of them"; dwim_ref() only checks
> > if .git/blah, .git/refs/heads/blah, .git/refs/tags/blah, etc.  exists and
> > the number of checks do not depend on the number of refs you have, so I
> > was wondering if I overlooked something that does for_each_ref() of
> > everything.
>
> Yeah, for loose refs that's true. However, I have 470 packed refs, and
> this command:
>
> 	$ valgrind --tool=exp-dhat ./git grep guess xdiff/xutils.c
>
> reports (among other findings):
>
> ==28255== max-live:    30,334 in 470 blocks
> ==28255== tot-alloc:   30,334 in 470 blocks (avg size 64.54)
> ==28255== deaths:      none (none of these blocks were freed)
> ==28255== acc-ratios:  7.76 rd, 0.95 wr  (235,582 b-read, 28,924 b-written)
> ==28255==    at 0x402AEE8: malloc (in /usr/lib/valgrind/vgpreload_exp-dhat-x86-linux.so)
> ==28255==    by 0x813691D: xmalloc (wrapper.c:50)
> ==28255==    by 0x8106B1A: create_ref_entry.constprop.8 (refs.c:250)
> ==28255==    by 0x8107761: read_packed_refs (refs.c:817)
> ==28255==    by 0x810785F: get_packed_refs (refs.c:843)
> ==28255==    by 0x8107BE7: resolve_ref_unsafe (refs.c:1028)
> ==28255==    by 0x81090AA: dwim_ref (refs.c:1549)
> ==28255==    by 0x8122E06: get_sha1_1 (sha1_name.c:304)
> ==28255==    by 0x81237EF: get_sha1_with_context_1 (sha1_name.c:1044)
> ==28255==    by 0x8124016: get_sha1 (cache.h:795)
> ==28255==    by 0x75782F65: ???

You are seeing all of the packed refs being *read*, which is triggered 
in this case by resolve_ref_unsafe() -> get_packed_refs().  This stack 
trace does not imply that all references are being iterated through as 
part of a search.  IOW, if dwim_ref() were called again, there would not 
be another 470 allocations.

This is similar to the old code, which also read all of the packed refs 
whenever any of them were accessed.

Michael

--
Michael Haggerty
mhagger@xxxxxxxxxxxx
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html