Re: [PATCH] process_{tree,blob}: Remove useless xstrdup calls

From: Björn Steinbrink <b.steinbrink@gmx.de>
Date: Apr 11, 2009, 19:40 UTC
Message-ID: <20090411194000.GB21300@atjola.homenet>
In-Reply-To: <alpine.LFD.2.00.0904111115210.4583@localhost.localdomain>

On 2009.04.11 11:19:01 -0700, Linus Torvalds wrote:

Show 24 quoted lines

> On Sat, 11 Apr 2009, Björn Steinbrink wrote:
> > On 2009.04.11 15:41:12 +0200, Björn Steinbrink wrote:
> > > On 2009.04.10 18:15:26 -0700, Linus Torvalds wrote:
> > > > It obviously goes on top of my previous patch.
> > > 
> > > Gives some nice results for the "rev-list --all --objects" test on the
> > > gentoo repo says (with the old pack):
> > >      | With my patch | With your patch on top
> > > -----|---------------|-----------------------
> > > VSZ  |       1667952 | 1319324
> > > RSS  |       1388408 | 1126080
> > 
> > linux-2.6.git:
> > 
> >      | With my patch | With your patch on top
> > -----|---------------|-----------------------
> > VSZ  |        460376 | 407900
> > RSS  |        292996 | 239760
> 
> Interesting. That's a 18+% reduction in RSS in both cases. Much bigger 
> than I expected, or what I saw in my limited testing. Is this in 32-bit 
> mode, where the pointers are cheaper, and thus the non-pointer data 
> relatively more expensive and a bigger percentage of the total? We really 
> wasted a _lot_ of memory on those names.

No, this is x86-64, 8 byte pointers. But the savings are trivially explained I think. The struct object_array things are 20 bytes here (per object overhead!), so that's about 5M * 20 = 100M. And the average name length for the objects was 19 bytes, which means about another 100M. Both, the object_array stuff as well as the path names, were allocated and never freed. Your patch removed the object_array stuff, and it made the memory allocations for the names temporary. Right?

Had you moved just the path_name() calls, that would have meant that we had needed to keep the name_path stuff around, which is also 20 bytes here (two pointers, one int). And that would have meant that anything that has a leading-up path shorter than 20 bytes (64 bit pointers) would have seen increased memory usage (64bit pointers), but with 32 pointers, the limit would have been 12 bytes.

So for the "just move path_name() call" solution, 32bit vs. 64bit would have made a difference, but with your actual patches, you just turned everything into temporary allocations. So the 4byte overhead on 64bit platforms is just once linear with the directory-depth of the current object, instead of with the number of objects in total.

Right?

Björn