Re: [PATCH 07/14] tree-diff: drop path_appendnew() alloc optimization

From: Patrick Steinhardt <ps@pks.im>
Date: Jan 13, 2025, 15:40 UTC
Message-ID: <Z4Uz7B4J89NphNF6@pks.im>
In-Reply-To: <20250109084649.GG2748836@coredump.intra.peff.net>

On Thu, Jan 09, 2025 at 03:46:49AM -0500, Jeff King wrote:

Show 7 quoted lines

> So my conclusion is that it probably does help a little, but it's mostly
> lost in the noise. I could see an argument for keeping it, as the
> complexity is hidden away in functions that do not often need to be
> touched. But it does make them more confusing than necessary (despite
> some detailed explanations from the author of that commit; it just took
> me a while to wrap my head around what was going on) and prevents
> further refactoring of the combine_diff_path struct. So let's drop it.

A 1% performance speedup does not feel like a good argument to me, so I'm perfectly fine with dropping the code, even if most of it is actually in the form of comments. But that already shows that it needs quite a bit of explanation.

I wonder though: did you also use e.g. Valgrind to compare the number of allocations? glibc tends to be heavily optimized with regards to small allocations, so you typically don't notice the performance impact caused by them even when the number of saved allocations is significant. So the effect might be more pronounced with other libcs that aren't optimized for such usecases, like e.g. musl libc.

Patrick