Issue #6328 - avoid binding WebSocket MethodHandles #12181
Conversation
Signed-off-by: Lachlan Roberts <[email protected]>
Signed-off-by: Lachlan Roberts <[email protected]>
…websocketMethodHolder
...-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/util/MethodHolder.java
Outdated
Show resolved
Hide resolved
| */ | ||
| public interface MethodHolder | ||
| { | ||
| String METHOD_HOLDER_BINDING_PROPERTY = "jetty.websocket.methodholder.binding"; |
There was a problem hiding this comment.
Rather than system property configuration, could this be done as a LOG.isDebugEnabled instead? The primary motivation for using this mode is to make flame graphs look better, so it is a kind of logging? @sbordet thoughts?
There was a problem hiding this comment.
Its hard to define a logger in the interface because it will need to be public and then I think we need to export it with JPMS.
...-core-common/src/main/java/org/eclipse/jetty/websocket/core/util/NonBindingMethodHolder.java
Outdated
Show resolved
Hide resolved
...ket-core-common/src/main/java/org/eclipse/jetty/websocket/core/util/BindingMethodHolder.java
Outdated
Show resolved
Hide resolved
| @@ -51,9 +51,9 @@ public abstract class DispatchedMessageSink extends AbstractMessageSink | |||
| private volatile CompletableFuture<Void> dispatchComplete; | |||
| private MessageSink typeSink; | |||
|
|
|||
| public DispatchedMessageSink(CoreSession session, MethodHandle methodHandle, boolean autoDemand) | |||
| public DispatchedMessageSink(CoreSession session, MethodHolder methodHolder, boolean autoDemand) | |||
There was a problem hiding this comment.
Do we need to provide all the old APIs deprecated?
| public DispatchedMessageSink(CoreSession session, MethodHolder methodHolder, boolean autoDemand) | |
| @Deprecated | |
| public DispatchedMessageSink(CoreSession session, MethodHandle methodHandle, boolean autoDemand) | |
| { | |
| this(session, MethodHolder.from(methodHandle), autoDemand); | |
| } | |
| public DispatchedMessageSink(CoreSession session, MethodHolder methodHolder, boolean autoDemand) |
There was a problem hiding this comment.
I don't think so, this is websocket-core, and this is really not supposed to be used by the application more of an implementation class.
There was a problem hiding this comment.
yeah but it is public and not internal.... @sbordet thoughts?
There was a problem hiding this comment.
I think this probably should be internal.
Its to reduce having code duplication from having this in our jakarta and jetty websocket implementations. But making it internal makes things difficult with JPMS.
But there is no API to actually use a MessageSink directly.
There was a problem hiding this comment.
OK, leave out and we can add back deprecated if anybody complains.... although it is against our new deprecated policy :)
...-websocket-core-common/src/main/java/org/eclipse/jetty/websocket/core/util/MethodHolder.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Lachlan Roberts <[email protected]>
…websocketMethodHolder
lachlan-roberts
force-pushed
the
jetty-12.1.x-websocketMethodHolder
branch
from
ba97660 to
e923380
Compare
Use a new MethodHolder 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.