On Thu, 10 Aug 2023 at 23:09, Richard Fitzgerald rf@opensource.cirrus.com wrote:
On 10/8/23 15:38, David Gow wrote:
On Wed, 9 Aug 2023 at 23:54, Richard Fitzgerald rf@opensource.cirrus.com wrote:
Add handling to kunit_log_append() for log messages that are longer than a single buffer fragment.
The initial implementation of fragmented buffers did not change the logic of the original kunit_log_append(). A consequence was that it still had the original assumption that a log line will fit into one buffer.
This patch checks for log messages that are larger than one fragment buffer. In that case, kvasprintf() is used to format it into a temporary buffer and that content is then split across as many fragments as necessary.
Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com
I think this looks good (and is a long-overdue addition to the logging functionality).
One thought I have (and I'm kicking myself for not thinking of it earlier) is that this is starting to get very similar to the "string stream" functionality in lib/kunit/string-stream.{h,c}. Now, I actually think this implementation is much more efficient (using larger fragments, whereas the string stream uses variable-sized ones). Particularly since there are a lot of cases where string streams are created, converted to a string, and then logged, there's almost certainly a bunch of redundant work being done here.
My gut feeling is that we should stick with this as-is, and maybe try to either work out some better integration between string streams and logging (to avoid that extra string allocation) or find some way of combining them.
I completely failed to notice string_stream. I could re-implement this to use string_stream. I wonder whether appending newlines gets a bit inefficient with the current string_stream implementation. Could add newline support to string_stream and alloc one extra byte for each fragment just in case we need to add a newline.
The string_stream implementation would waste a lot a memory if you log many short lines. My current code wastes memory if you log lots of lines that don't fit in available space in the current fragment - though it's simple to shuffle the formatted string backwards to fill up the previous fragment (I just haven't done that yet).
Yeah: I think your implementation here is overall better than the string_stream one. string_stream might handle concurrency a bit better, which would be nice to have as people start wanting to try multithreaded tests.
I think the ideal solution is: - Update string_stream to basically use this implementation. - Update the logging code to then use this via the string_stream API (probably with some tweaks to handle newlines) - Optimize the string_stream append implementation to not create a temporary string, as string streams are written to logs often. (If you were prepared to allow string stream fragments to have variable lengths, and do some ownership shenanigans, this could even become O(1), though I suspect it's not worth the added complexity.)
That being said, I don't think we'd need to land all of that at once. Even if we ported to the suboptimal string_stream API now (which would still gain us the extensible log and some concurrency support), and optimized string_stream later if it turned out to be tricky, that'd be fine. (I wouldn't want to hold this up if changing string_stream was regressing the other code which uses it, for example.)
How does that sound?
-- David