search

ebkalderon / tower-lsp

Language Server Protocol implementation written in Rust
Apache License 2.0
1.02k stars 57 forks source link

Reduce message reparsing with lspower-based codec #314

Closed ebkalderon closed 2 years ago

ebkalderon commented 2 years ago

I've been reading through the LanguageServerCodec implementation merged in from the lspower fork in #309, and I feel somewhat skeptical of the claim in the README.md that this new codec is more efficient and reduces message reparsing compared to the nom implementation that existed before (source).

The previous implementation used to a remaining_msg_bytes field to return Ok(None) immediately if the message payload wasn't entirely buffered into memory. By contrast, the lspower implementation attempts to blindly reparse the headers with httparse every time. I think should be pretty easy to add in a remaining_msg_bytes field into the new codec so that it truly runs as fast as it claims, though.

Also, I've identified another optimization that would also boost efficiency beyond both the original and lspower codec implementations: it would be nice if we could eagerly advance the src buffer past the HTTP headers once we're done parsing them, so that we don't need to re-run httparse each time the codec is still waiting for the payload to come through.

CC @silvanshade

ebkalderon commented 2 years ago

Perhaps we could also add a criterion.rs bench test to the repo, so we can keep proper tabs on the actual performance of the codec rather than speculating.

silvanshade commented 2 years ago

The previous implementation used to a remaining_msg_bytes field to return Ok(None) immediately if the message payload wasn't entirely buffered into memory. By contrast, the lspower implementation attempts to blindly reparse the headers with httparse every time.

You are right about this. It's strange because I clearly remember doing some debugging and seeing opportunities to minimize the reparsing.

In fact, it looks like at one point I did still use that field but removed it in a refactoring: https://github.com/silvanshade/lspower/commit/c642b2bbada3ece48a3b3d55f8f99e056748677d

But that refactoring actually seems to have done the opposite of what I claimed. I must have been confused or maybe rebased things incorrectly and didn't notice somehow.

I think should be pretty easy to add in a remaining_msg_bytes field into the new codec so that it truly runs as fast as it claims, though.

Indeed it is. You can see how I did it here: https://github.com/silvanshade/lspower/commit/c642b2bbada3ece48a3b3d55f8f99e056748677d#diff-ca14eabd5ab83f72548cd7e44eaea3f56a5d2808e3275c6b2a97622ebe5d1dd0L156-L161

Also, I've identified another optimization that would also boost efficiency beyond both the original and lspower codec implementations: it would be nice if we could eagerly advance the src buffer past the HTTP headers once we're done parsing them, so that we don't need to re-run httparse each time the codec is still waiting for the payload to come through.

Yes, this was in fact the original intention behind what I was trying to do (that and the fast advance with twoway). In fact, it makes more sense to me now why I was storing header_len and content_len in the codec state (which I removed in the PR I sent). I couldn't figure out why I had stored them there, but it was precisely because of this (before I made the incorrect change in the linked commit).

I could either create a PR to go back to this behavior or if you want to just go ahead implement it yourself (or maybe already have) that would be great.