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.

Sign up for GitHub

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

Jump to bottom

Issue #6328 - avoid binding WebSocket MethodHandles #8087

Closed
wants to merge 1 commit into from
Closed

Issue #6328 - avoid binding WebSocket MethodHandles #8087

wants to merge 1 commit into from

Conversation

lachlan-roberts
Copy link
Contributor

@lachlan-roberts lachlan-roberts commented Jun 1, 2022 •
edited by joakime

Fixes #6328

Use a new JettyMethodHandle interface so that implementations can be used which avoid binding the MethodHandles for each WebSocket connection. This will avoid the multiple spikes seen on the flamegraph from #6328.

For Javax endpoints we are creating new method handles each time because we cannot have the same MetaData cache as used in the JettyWebSocketFrameHandlerFactory because it depends on both the endpointClass and the endpointConfig. However javax.websocket.MessageHandler does not actually use method handles at all and has a special implementation of JettyMethodHandle.

gregw
gregw requested changes Jun 2, 2022
View reviewed changes
Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty good.... but naming needs improvement.
Also I question the need for sparse binding of args. It would be more efficient to only support the same binding style as MethodHandle

...e-common/src/main/java/org/eclipse/jetty/websocket/core/internal/util/JettyMethodHandle.java Outdated Show resolved Hide resolved
.../src/main/java/org/eclipse/jetty/websocket/javax/common/JavaxMessagePartialMethodHandle.java Outdated Show resolved Hide resolved
...rc/main/java/org/eclipse/jetty/websocket/core/internal/util/NonBindingJettyMethodHandle.java Outdated Show resolved Hide resolved
...rc/main/java/org/eclipse/jetty/websocket/core/internal/util/NonBindingJettyMethodHandle.java Outdated Show resolved Hide resolved
...rc/main/java/org/eclipse/jetty/websocket/core/internal/util/NonBindingJettyMethodHandle.java Outdated Show resolved Hide resolved
sbordet
sbordet requested changes Jun 3, 2022
View reviewed changes
...on/src/main/java/org/eclipse/jetty/websocket/core/internal/messages/AbstractMessageSink.java Outdated Show resolved Hide resolved
...src/main/java/org/eclipse/jetty/websocket/core/internal/messages/InputStreamMessageSink.java Outdated Show resolved Hide resolved
...mmon/src/main/java/org/eclipse/jetty/websocket/core/internal/messages/ReaderMessageSink.java Outdated Show resolved Hide resolved
...mmon/src/main/java/org/eclipse/jetty/websocket/core/internal/messages/StringMessageSink.java Outdated Show resolved Hide resolved
.../src/main/java/org/eclipse/jetty/websocket/javax/common/JavaxMessagePartialMethodHolder.java Outdated Show resolved Hide resolved
.../main/java/org/eclipse/jetty/websocket/javax/common/messages/AbstractDecodedMessageSink.java Show resolved Hide resolved
...rc/main/java/org/eclipse/jetty/websocket/javax/common/messages/DecodedBinaryMessageSink.java Show resolved Hide resolved
...n/java/org/eclipse/jetty/websocket/javax/common/messages/DecodedBinaryStreamMessageSink.java Show resolved Hide resolved
.../src/main/java/org/eclipse/jetty/websocket/javax/common/messages/DecodedTextMessageSink.java Show resolved Hide resolved
...ain/java/org/eclipse/jetty/websocket/javax/common/messages/DecodedTextStreamMessageSink.java Show resolved Hide resolved
gregw
gregw requested changes Jun 3, 2022
View reviewed changes
Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Getting there...

...t-core-common/src/main/java/org/eclipse/jetty/websocket/core/internal/util/MethodHolder.java Outdated Show resolved Hide resolved
...t-core-common/src/main/java/org/eclipse/jetty/websocket/core/internal/util/MethodHolder.java Outdated Show resolved Hide resolved
...mon/src/main/java/org/eclipse/jetty/websocket/core/internal/util/NonBindingMethodHolder.java Outdated Show resolved Hide resolved
...mon/src/main/java/org/eclipse/jetty/websocket/core/internal/util/NonBindingMethodHolder.java Outdated Show resolved Hide resolved
...mon/src/main/java/org/eclipse/jetty/websocket/core/internal/util/NonBindingMethodHolder.java Outdated Show resolved Hide resolved
...mon/src/main/java/org/eclipse/jetty/websocket/core/internal/util/NonBindingMethodHolder.java Outdated Show resolved Hide resolved
...mon/src/main/java/org/eclipse/jetty/websocket/core/internal/util/NonBindingMethodHolder.java Outdated Show resolved Hide resolved
gregw
gregw requested changes Jun 6, 2022
View reviewed changes
Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm down to tiny quibbles now.

...mon/src/main/java/org/eclipse/jetty/websocket/core/internal/util/NonBindingMethodHolder.java Outdated Show resolved Hide resolved
...mon/src/main/java/org/eclipse/jetty/websocket/core/internal/util/NonBindingMethodHolder.java Outdated Show resolved Hide resolved
...mon/src/main/java/org/eclipse/jetty/websocket/core/internal/util/NonBindingMethodHolder.java Outdated Show resolved Hide resolved
...mon/src/main/java/org/eclipse/jetty/websocket/core/internal/util/NonBindingMethodHolder.java Outdated Show resolved Hide resolved
sbordet
sbordet previously approved these changes Jun 8, 2022
View reviewed changes
@joakime
Copy link
Contributor

joakime commented Jun 8, 2022

Do we want this in Jetty 10.0.10 release?

@lachlan-roberts
Copy link
Contributor Author

@joakime no this does not need to be in the 10.0.10 release.

I think it also needs some performance testing to assess the impact of this.

@lachlan-roberts lachlan-roberts added this to In progress in Jetty 10.0.11/11.0.11 - 🧊 FROZEN 🥶 via automation Jun 8, 2022
gregw
gregw previously approved these changes Jun 8, 2022
View reviewed changes
Jetty 10.0.11/11.0.11 - 🧊 FROZEN 🥶 automation moved this from In progress to Reviewer approved Jun 8, 2022
@joakime joakime removed this from Reviewer approved in Jetty 10.0.11/11.0.11 - 🧊 FROZEN 🥶 Jun 21, 2022
@joakime joakime added this to In progress in Jetty 10.0.12 / 11.0.12 via automation Jun 21, 2022
@joakime
Copy link
Contributor

joakime commented Jun 21, 2022

Skipping this release, moving to 10.0.12 / 11.0.12 release.

@joakime
Copy link
Contributor

joakime commented Aug 2, 2022

@lachlan-roberts you can merge this one now.

@joakime
Copy link
Contributor

joakime commented Nov 10, 2022

@lachlan-roberts can this be merged?

@joakime joakime added the Bug For general bugs on Jetty side label Nov 10, 2022
@lachlan-roberts
Copy link
Contributor Author

@joakime no, the benchmarks indicated that using the wrapper around methodhandle is significantly slower. So we need to further assess the performance impact this change would have.

I don't think this should be classified as a bug, the current usage of MethodHandles is not wrong.

@lachlan-roberts lachlan-roberts added Enhancement and removed Bug For general bugs on Jetty side labels Nov 11, 2022
@joakime
Copy link
Contributor

joakime commented Nov 14, 2022

Moving this to 10.0.14 then.

@joakime joakime added this to the 10.0.x milestone Dec 8, 2022
@janbartel
Copy link
Contributor

@lachlan-roberts what's the status of this one? Should it be moved back to Draft status?

@lachlan-roberts lachlan-roberts marked this pull request as draft July 3, 2023 23:11
@lachlan-roberts
Copy link
Contributor Author

@janbartel yes I converted it back to a draft.

Maybe this PR is something we want to consider for Jetty 12.

@gregw
Copy link
Contributor

gregw commented Jul 5, 2023

@lachlan-roberts I've asked the OP what their status is.
Can you update the branch against latest 10.0.x and repeat performance tests to check impact.

I would think this is something we want to do for 10/11/12 or not at all. If there is a net benefit, then best to keep the code similar in all branches.

@lachlan-roberts lachlan-roberts dismissed stale reviews from sbordet and gregw via 68e6e9c October 4, 2023 01:34
@lachlan-roberts
add MethodHolder abstraction to avoid binding WebSocket MethodHandles
e07b8b4 
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
@lachlan-roberts lachlan-roberts force-pushed the jetty-10.0.x-6328-JettyMethodHandlesExperiment branch from 68e6e9c to e07b8b4 Compare October 4, 2023 01:38
@lachlan-roberts
Copy link
Contributor Author

closing in favor of #10750 for Jetty 12

@lachlan-roberts lachlan-roberts deleted the jetty-10.0.x-6328-JettyMethodHandlesExperiment branch October 18, 2023 20:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gregw gregw gregw left review comments

@sbordet sbordet sbordet left review comments

Assignees

@lachlan-roberts lachlan-roberts

Labels
Enhancement Performance
Projects
No open projects
Jetty 10.0.12 / 11.0.12
In progress
Milestone
10.0.x
Development

Successfully merging this pull request may close these issues.

High CPU usage of method handle invocations in Jetty 10
5 participants
@lachlan-roberts @joakime @janbartel @gregw @sbordet