On Thu, Jun 6, 2024 at 9:25 AM Nathan Hartman <hartman.nat...@gmail.com> wrote:
> I haven't had a chance to really study this yet (or build or test), > but the main part of the patch is in ra.c, where > svn_client__get_youngest_common_ancestor() was calling > svn_client__get_history_as_mergeinfo(). The patch wraps those calls in > a new function get_history_from_cache_or_as_mergeinfo(). > > Conceptually a local in-memory cache seems reasonable, though I need > to study this patch some more (and build and test). > > I'd also wonder about possible unbounded memory usage if there should > be a very large number of items to cache, but I suppose the cache size > could be limited to some reasonable value. > > This part of the OP's message (at [4]) is relevant, especially the > questions about context and the public API: > > > On Thu, May 30, 2024 at 5:09 PM Sands, Daniel N. via users < > us...@subversion.apache.org> >> I came up with a patch for this issue. > It cuts the resolve time down > >> from literal hours in my case, to less than a minute. I can't say it's > >> production ready, but it's a template at least. > >> > >> It attacks the core of the problem, where every time it comes up with a > >> candidate pair to check, it downloads the history from the repo on each > >> file. This happened for the same left-side file hundreds of times > >> while it tried each candidate right-side file. > >> > >> The patch leaves in (commented-out) printfs to show the problem in > >> action. The other part is, now that I have to persist data as long as > >> the client context, do the temporary results pools get used for > >> anything at all? Finally, there is one change to a public API that > >> would need to be fixed. > As you said, conceptually there should be no problem here. The patch reads easily enough. I spotted a few new or meaningfully modified functions, and then a bunch of places where some bit of context is passed through the call stack so it's available for use with the aforementioned functions. Maybe I misunderstood or overlooked it, but I did NOT spot the "one change to a public API" -- I see only changes to private header files (one in include/private/; one local to libsvn_client itself). My review was cursory at best, but it was enough for me to be of the opinion that this contribution is worth a closer examination. -- Mike
