(Resending as plain text) On Mar 28, 2024, at 01:21, Patrick Steinhardt <ps@xxxxxx> wrote: > I was about to propose that we should likely also change all of these > static variables to be local instead. I don't think that we use the > variables after the function calls. But now that I see that we do it > like this in all of these helpers I think what's going on is that this > is a memory optimization to avoid reallocating buffers all the time. > > Ugly, but so be it. We could refactor the code to pass in scratch > buffers from the outside to remove those static variables. But that > certainly would be a bigger change and thus likely outside of the scope > of this patch series. > Oh, now you get to my comment in the preceding patch. With this patch > we're now in a somewhat weird in-between state where the buffers are > still static, but we release their memory after each call. So we kind of > get the worst of both worlds: static variables without being able to > reuse the buffer's memory. > > If we were to change this then we should definitely mark the buffers as > non-static. If so, it would be great to demonstrate that this does not > significantly impact performance. > > The same is true for all the other instances. I had glossed that they're `static`, since I've grown accustomed to Rust, where this sort of non-reentrant code is discouraged. However, this pattern is great for fast-import, because all of its data is simply freed when it exits at the end of the stream. I dropped this patch in v2. I don't think it's worth hoisting these `strbuf`s out. It would only reduce it from 5 to 2 total static `strbuf`s for paths, but would make ownership less clear. Thalia
