[clangd] Reuse buffer for JSONTransport::sendMessage

[njames93]

Summary

Allocate a Buffer in the JSONTransport to be used when sending messages to the client.
This gets reused each time a message is sent, reducing in fewer malloc, which is always a bonus.

[njames93]

100% sure that failure is from trunk

[njames93]

Extend buffer behaviour to sendMessage.

Calls to this function seem to be guarded(up the call stack) by a mutex so again we shouldn't worry about races.
Besides there is no synchronization in the function currently when writing to the output stream.

[sammccall]

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").
• 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.

[njames93]

In 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.

[njames93]

Moved a few members out of the class.

[sammccall]

In D93531#2463126, @njames93 wrote:

In 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.

Well, it's also wise to measure before optimizing. The large requests (ones containing a whole file) typically cause us to parse a C++ TU. After we read this JSON message, we will immediately:

• allocate a std::string to hold the decoded file content payload as part of the json::Value
• copy that string into the DraftManager
• allocate a MemoryBuffer and copy the file into it, for parsing
• read all of the thousands of transitively included headers into newly-allocated MemoryBuffers
• run the clang parser and then the clangd indexer

I think the most likely case is that this change has no observable impact on performance or resource usage. If that's true, I don't want to land it - it makes the code harder to reason about for no detectable benefit.

It's definitely *plausible* there's something surprising going on with allocation patterns such that this makes a difference. The impact of malloc_trim shows that clangd's overall allocation patterns have surprising effects. But right now I don't see any reason to think that these allocations matter.

• 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.

JSONLineBuffer is not used for reading JSON, but rather HTTP-style headers. The only headers supported are Content-Length and Content-Type, and I don't believe clients send Content-Type in practice (there's only one supported). In any case, you'll never see a line beyond 256 chars from a well-behaved client. ("statically bounded" is too strong, but from a performance-tuning perspective it's enough).

[njames93]

Remove input buffers, but keep output as its easy to reason about.

[sammccall]

Thanks, this version seems just as clear as the original to me, and fewer allocations is nice :-)

[sammccall]

I've put a version of the reading part in D93653. This seems a bit less invasive to me - I'm still not sure if it's worth the readability hit though...

clang-tools-extra/clangd/JSONTransport.cpp

llvm::Error loop(MessageHandler &Handler) override {
     return llvm::errorCodeToError(std::make_error_code(std::errc::io_error));
   }
 
 private:
   // Dispatches incoming message to Handler onNotify/onCall/onReply.
   bool handleMessage(llvm::json::Value Message, MessageHandler &Handler);
   // Writes outgoing message to Out stream.
   void sendMessage(llvm::json::Value Message) {
-    std::string S;
-    llvm::raw_string_ostream OS(S);
+    OutputBuffer.clear();
+    llvm::raw_svector_ostream OS(OutputBuffer);
     OS << llvm::formatv(Pretty ? "{0:2}" : "{0}", Message);
-    OS.flush();
-    Out << "Content-Length: " << S.size() << "\r\n\r\n" << S;
+    Out << "Content-Length: " << OutputBuffer.size() << "\r\n\r\n"
+        << OutputBuffer;
     Out.flush();
-    vlog(">>> {0}\n", S);
+    vlog(">>> {0}\n", OutputBuffer);
   }
 
   // Read raw string messages from input stream.
   llvm::Optional<std::string> readRawMessage() {
     return Style == JSONStreamStyle::Delimited ? readDelimitedMessage()
                                                : readStandardMessage();
   }
   llvm::Optional<std::string> readDelimitedMessage();
   llvm::Optional<std::string> readStandardMessage();
 
+  llvm::SmallVector<char, 0> OutputBuffer;
   std::FILE *In;
   llvm::raw_ostream &Out;
   llvm::raw_ostream &InMirror;
   bool Pretty;
   JSONStreamStyle Style;
 };
 
 bool JSONTransport::handleMessage(llvm::json::Value Message,