New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Avoid temporary String object creation in StreamMessageProducer #816
Conversation
b650ae6
to
446a98a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - I have trouble imagining it will fully solving your OOM, but saving the 160M is definitely worthwhile.
I'll leave it open for now for others to have a look. But assuming no dissenting views it can be merged soonish.
No, it won't solve the issue but it allows for processing of larger messages without getting an OOM. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sebthom Thank you for the enhancement!
LGTM. Just a couple of nitpicks, which may be ignored :-)
...clipse.lsp4j.jsonrpc/src/main/java/org/eclipse/lsp4j/jsonrpc/json/StreamMessageProducer.java
Outdated
Show resolved
Hide resolved
org.eclipse.lsp4j.jsonrpc/src/main/java/org/eclipse/lsp4j/jsonrpc/util/LimitedInputStream.java
Outdated
Show resolved
Hide resolved
Thanks to @pisv we have a positive review - even better than simply no dissenting views! Waiting to hear from @sebthom if this commit or #817 should be submitted first (as I assume some amount of merging conflict resolution may be required) |
Rebasing this PR is easier, so I would prefer if #817 gets merged first. |
This can reduce the likeliness of OOMs when dealing with large response messages from LS servers. See discussion at #815