Branko Čibej wrote: > On 21.04.2014 19:05, Ivan Zhakov wrote: >>> trunk/subversion/libsvn_delta/text_delta.c:923 >>> trunk/subversion/libsvn_subr/io.c:805 >> >> [...] It's good time to make consistent in either way. Where temporary >> read buffers should be allocated: on stack or in heap (pool)? I'm fine >> with both approaches, but I would like to make it consistent. What do >> you think? > > Why do you want the code to be consistent? There are good arguments > for both approaches. Consistency for its own sake makes no sense.
It's not obvious to me, at first sight, why we might choose the stack in some places and the heap in others, so it would be helpful to discuss it briefly. After a few minutes' thought I came up with the following differences. I'm thinking of a 64 KB temporary buffer. Advantages on the stack: * Quicker to allocate * Memory guaranteed to be freed by any kind of return from function * Allocates slightly less memory (no overhead) Advantages on the heap: * Stack stays smaller, allowing svn to run on platforms with restricted stack space * Stack stays smaller, improving locality of references to other stack data, which may help the function to run faster after the initial allocation What else is important? The "restricted stack space" concern may be the most important. I don't know whether this concerns us in practice, for buffers in the 64 KB size range, on the platforms we target. If stack space is not a problem, then the other points above suggest to me that allocating on the stack is likely to be a better choice usually. A function that does a lot of tight, CPU-intensive processing could be an exception, if profiling shows it would be significantly faster with a buffer on the heap. But that's just my speculation. An example: the buffer in subversion/libsvn_fs_fs/transaction.c:3578 fnv1a_checksum_on_file_range() is allocated on the heap (in its caller's iterpool) but not needed after the function returns. The function overall may be a CPU load hot spot, but not in its own code but rather inside the call to svn_checksum_update. Would it be better to put this buffer on the stack? - Julian
