Jetty 12 recycle servlet channel #8909
Conversation
...0/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextHandler.java
Outdated
Show resolved
Hide resolved
jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java
Outdated
Show resolved
Hide resolved
jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java
Show resolved
Hide resolved
...ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/AsyncServletIOTest.java
Outdated
Show resolved
Hide resolved
|
@gregw Looks like there was some failure with this PR on Jenkins. |
| @@ -47,9 +47,10 @@ class AsyncContentProducer implements ContentProducer | |||
| private long _firstByteNanoTime = Long.MIN_VALUE; | |||
| private long _rawBytesArrived; | |||
|
|
|||
| AsyncContentProducer(ServletChannel servletChannel) | |||
| AsyncContentProducer(ServletChannel servletChannel, AutoLock lock) | |||
There was a problem hiding this comment.
This constructor should have a comment to explain why locking is now being controlled externally.
...jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/BlockingContentProducer.java
Outdated
Show resolved
Hide resolved
jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/HttpOutput.java
Outdated
Show resolved
Hide resolved
jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java
Outdated
Show resolved
Hide resolved
jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java
Outdated
Show resolved
Hide resolved
|
|
||
| public ServletChannel(ServletContextHandler servletContextHandler, Request request) | ||
| { | ||
| _state = new ServletRequestState(this); | ||
| _context = servletContextHandler.getContext(); | ||
| _servletContextApi = _context.getServletContext(); |
There was a problem hiding this comment.
Couldn't this be recycled and go through a different ServletContextHandler which would have a different ServletContextApi?
If so then this would need to be done in associate().
There was a problem hiding this comment.
No, see the test when we reuse a ServletChannel that it is for the same context, else we create a new one.
There was a problem hiding this comment.
Wouldn't it be better to have a ServletChannel which could be re-used for any context instead of always remaking it if it goes to a different context.
There was a problem hiding this comment.
Yeah maybe, but as @lorban would say, that is optimizing the slow path. Cross context requests are rare and many deployments only have 1 context per server.
Having these big objects with lots of final fields increases the benefit of re-using them, as all those final fields don't need to be re-enabled.
So I'm happy with it as is for now. Perhaps if we see cross context slowness as a problem in future we invest in a reusable instance per context?
There was a problem hiding this comment.
Ok, at least its better than before so I'll approve.
I can't see that there would be much extra cost to making it fully re-usable though.
There was a problem hiding this comment.
Maybe, but I don't really know the cost of creating new ServletContextAPI instances for each request. I'd much rather reuse them most of the time and then make a new one only if changing contexts.
|
@janbartel @lachlan-roberts re-review please |
Implement TODO's in ee10-servlet regarding Jetty 12 recycling of
ServletChannel