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

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: May 24, 2012, 04:34 UTC
Message-ID: <4FBDBA39.5020101@alum.mit.edu>
In-Reply-To: <4FBD1B36.5060404@lsrfire.ath.cx>

On 05/23/2012 07:15 PM, René Scharfe wrote:

Show 52 quoted lines

> Am 23.05.2012 18:56, schrieb Junio C Hamano:
>> René Scharfe<rene.scharfe@lsrfire.ath.cx>   writes:
>>
>>> Am 23.05.2012 00:18, schrieb Junio C Hamano:
>>>> René Scharfe<rene.scharfe@lsrfire.ath.cx>    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@alum.mit.edu
http://softwareswirl.blogspot.com/