Skip to content
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 unnecessary string conversion #1148

Merged
merged 1 commit into from
Mar 4, 2020

Conversation

brharrington
Copy link
Contributor

When parsing the LWC messages converting to a string first
is unnecessary. Avoiding this results in about 30% improvement
in the throughput.

When parsing the LWC messages converting to a string first
is unnecessary. Avoiding this results in about 30% improvement
in the throughput.
@brharrington brharrington added this to the 1.7.0 milestone Mar 3, 2020
@brharrington brharrington requested a review from jfz March 3, 2020 22:29
@jfz
Copy link
Contributor

jfz commented Mar 4, 2020

The data from Websocket API is actually TextMessage, and got converted to ByteString, so I think, to minimize convertion, we can just fold as String from Websocket output and parse as String here? Another option is having Websocket API output ByteString, but that make manual test a bit harder.

See: EvaluatorImpl.createWebSocketFlow

@brharrington
Copy link
Contributor Author

Parsing directly from the ByteString is a bit faster. From a performance perspective I think we are probably better off keeping it as ByteString as much as possible. The ByteString also gives us more flexibility to use a binary JSON encoding later on for further improvement if we want to.

With this change though, the parsing can accept either giving us more flexibility to refine more over time.

@brharrington brharrington merged commit 55cc0c1 into Netflix:master Mar 4, 2020
@brharrington brharrington deleted the lwc-string branch March 4, 2020 21:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants