On Fri, Sep 01, 2023 at 01:21:10PM -0700, Junio C Hamano wrote: > Jeff King <peff@xxxxxxxx> writes: > > > But I think that is the wrong way to optimize it. We shouldn't be > > storing any strings per-atom, but rather walking the parse tree to > > produce a single output buffer. And the values should be cheap to fill > > in, because we should parse the object as necessary up front. This is > > more or less the way the pretty.c parser does it. > > I thought "as necessary" may be a bit tricky as populate_value() > were taught to omit doing the whole get_object() thing when the > values for used_atom[] are all computable without parsing the object > at all, but it seems that over time the populate_value() callchain > has degraded sufficiently to unconditionally call get_object() these > days, so I agree that the arrangement does not have much optimization > value, at least in the current code. No, I think we still do that optimization. When parsing the format string, the parser function for each atom sets fields in an object_info struct to indicate what we're interested in. Then for each ref, we call populate_value(). If that object_info doesn't need anything (we byte-wise compare it to an empty dummy struct), then we return early, before calling get_object(). And that optimization is very important to retain; it makes a format like %(refname) an order of magnitude faster. The optimization I was referring to is that if you have a format like: %(contents:body) %(contents:body) then we'll de-duplicate that to a single used_atom struct, and they'll share the same v->s result string. That's much harder to do if you parse into an abstract syntax tree, since the two occupy different parts of the tree. But my contention is that it does not matter if you stop allocating v->s in the first place, and just walk the tree to directly output the result (either to a strbuf or directly to stdout). -Peff
