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

Improve performance of small websocket messages [jetty-10.0.x] #4561

Closed
wants to merge 1 commit into from
Closed

Improve performance of small websocket messages [jetty-10.0.x] #4561

wants to merge 1 commit into from

Conversation

SerCeMan
Copy link
Contributor

@SerCeMan SerCeMan commented Feb 9, 2020

Hi, Jetty Maintainers!

We've noticed in our tests based on jetty 9.4.26.v20200117 that a significant amount of CPU time is spent in SimpleBinaryMessage#appendFrame, and after running an allocation profiler, that the method is responsible for the majority of allocations (65% of all in our case).
Screen Shot 2020-02-09 at 7 29 03 pm

After having a closer look, it seems like even in the case of very small messages, the underlying method always allocates 65535 bytes. In our usage profile, the majority of the messages are very small, often than 100 bytes, and it seems like it's possible only to allocate the amount of memory needed in that case.

Since websocket classes were changed in jetty 10, this PR modifies ByteArrayMessageSink.java rather than the current SimpleBinaryMessage.java. However, if the PR looks good to you, then I'd be happy to send a separate PR for a backport.

Thanks :)

@lachlan-roberts
Copy link
Contributor

Thanks for the PR, I would suggest you first put up a PR for a fix in the 9.4.x branch first and you might get it in for a 9.4.27 release. These changes in jetty-10 get a bit more complicated, this class is duplicated for use in the jetty or javax ws APIs, this is planned to be fixed with PR #4549.

@SerCeMan SerCeMan changed the title Improve performance of small websocket messages Improve performance of small websocket messages [jetty-10.0.x] Feb 10, 2020
@joakime
Copy link
Contributor

joakime commented Feb 11, 2020

@lachlan-roberts now that the 9.4.x version is merged, are you going to merge this Jetty 10.0.x version?

@lachlan-roberts
Copy link
Contributor

lachlan-roberts commented Feb 11, 2020

@joakime I think this should wait until #4549 is all done to reduce the duplication and merge conflicts. I also think we are in need of a review of the performance of all the MessageSink classes. In jetty-10 we should also have access to the ByteBufferPool here, so it might be better if somehow we could use that instead of the BAOS.

@lachlan-roberts
Copy link
Contributor

Closing in favour of a larger review of all the MessageSink classes for jetty-10 (#4571).

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.

3 participants