njames93 added a comment. In D93531#2463052 <https://reviews.llvm.org/D93531#2463052>, @sammccall wrote:
> This adds a bit of complexity, making the code here a fair amount harder to > follow and verify the correctness of. > > - Do we have evidence that these allocations are causing a problem? (e.g. do > we observe a significant decrease in RSS after the patch)? Naively I would > expect these allocations to be basically unimportant compared to those of the > JSON objects themselves.(And I don't particularly expect either of them to be > significant - the comment on the other review was really just "0 probably > isn't the right arg to malloc_trim if there's any allocation going on"). It's not causing problems per say. but given the incoming json messages can contain a whole file plus things like escape chars. its wise to allocate a buffer that will grow to the largest json it receives but never shrink. > - there seem to be simpler ways to structure this avoiding allocations. > JSONLineBuffer is effectively statically bounded, and can be > `SmallString<32>` or so. The content buffer could simply be passed in if I'm > reading right: `bool readRawMessage(std::string&)`? OutputBuffer probably > does need to be a member variable though. Its not statically bounded unfortunately, the length is how ever long a line in the json is, which can be infinite. the read line function uses a buffer size of 1024 as an upperbound. However that can easily be exceeded as I think the contents of files are escaped so they will be read as 1 long line. It may be slightly more readable to make the function take a reference to the buffer though. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93531/new/ https://reviews.llvm.org/D93531 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
