On Mon, 2007-09-10 at 11:04 +0200, Pierre Habouzit wrote: > On Sun, Sep 09, 2007 at 10:38:55PM +0000, Junio C Hamano wrote: > > Pierre Habouzit <madcoder@xxxxxxxxxx> writes: > > > > > This brings builtin-stripspace, builtin-tag and mktag to use strbufs. > > > > One worry I have about this is that strbuf_read() only does the > > default allocation growth, while all of these callers used to > > have some guess as to what the initial allocation to avoid > > realloc should be. > > > > Perhaps you would want to pre-grow the buffer between strbuf_init() > > and strbuf_read() calls? > > Well, I was pondering about extending strbuf_read() to take an extra > signed argument that would be the initial growth strbuf_read should > perform, used as a size hint. negative values would mean to deafault to > the default mechanism. > > Would you be more comfortable if I introduce such a change first ? > That way, if we have stat'ed the file we try to read first, we can > directly allocate the correct size. > > I also think about extending strbuf_init the same way (this time with > a size_t), and if the second argument is non zero, then it performs a > strbuf_grow(...) of that size, because we often have: > > strbuf_init(&sb); > strbuf_grow(&sb, size_hint); > > and it's definitely better to read: > > strbuf_init(&sb, size_hint); > > > I think I'll do that soon. Could you try to benchmark it against the current set of changes? I really doubt this allocation hint is going to make a different, but I know people like to micro-manage their buffers :) My concern is that it's a complication of the interface and I'm skeptical that it will show up in a benchmark. Alternatively, could we just use strbuf_grow() for this? If it turns out that there is a noticeable improvement from pre-allocating, you can just do stat(), strbuf_grow(sb, st.st_size), no need for cluttering the strbuf api. cheers, Kristian - To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
