Issue #5022 Filter Cache cleanup #5271
Conversation
Issue #5022 Filter Cache cleanup: + Fixed many compiler warnings + removed old LazyList leftovers + Don't create holder string for source unless required + Only have a single type of chain, so it can be wrapped regardless of cache + Reverse mappings lists to make filter chain creation easier + build chain directly rather than build a list then a chain Signed-off-by: Greg Wilkins <gregw@webtide.com>
|
|
||
| // Servlet name filters | ||
| if (servletHolder != null && _filterNameMappings != null && !_filterNameMappings.isEmpty()) |
There was a problem hiding this comment.
The Servlet Spec requires that the first filters in the chain are the url style mappings, and then the servlet name mappings. This does it the other way around, doesn't it?
There was a problem hiding this comment.
I would hope we fail some of our unit tests then.
We should have basic tests that validate these kinds of requirements from the servlet spec.
Unless Greg has done some kind of trickery and his "Reverse mappings" statement is really "mappings done in reverse" making the change he made valid again.
There was a problem hiding this comment.
@janbartel The lists are in reverse so we apply the inner onion skin first. The order the filters are executed is not changed.
Signed-off-by: gregw <gregw@webtide.com>
|
Test failure on "JDK8 / testHTTP2 – org.eclipse.jetty.osgi.test.TestJettyOSGiBootHTTP2" ? |
turn off debug in OSGI test
|
thanks @janbartel .... but as I was about to merge, I've got cold feet!!!! I think this should be faster and better than the cache we currently have.... but I don't really know. So I'm not going to merge for this release cycle. @lorban @sbordet when you have time, we should chat about the possible performance impact of this change... it is on the main path to a Servlet and many big users have many many filters, so we need to get this right. |
# Conflicts: # jetty-servlet/src/main/java/org/eclipse/jetty/servlet/FilterHolder.java # jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletHandler.java
|
@lorban @sbordet can you find time to look at this and comment on possible performance impacts before I merge. Previously we either build a one time filter chain class that used an iterator OR a cacheable chain that was put into both a the cache and a LRU list, so full cache could pop off single entries. Now I build a filter chain the same way no matter what: a nested set of filter chain wrappers, that can be cached or used and discarded. This avoids creating a temporary array, but there can be more wrappers. The cache doesn't maintain an LRU and we just flush the entire cache if it fills. |
|
I don't expect any perf impact from the lower-level changes, but these two higher-level changes may have some:
If those two don't bother you (I assume they don't) then this LGTM. |
|
@lorban my thinking goes that for a webapp with few unique paths, then the cost of the LRU will be wasted as the cache will never fill up. So I thought let's make it work as best we can for the many webapps that only have a few unique URIs and then double the cache size just to capture more webapps in that category. |
Issue #5022 Filter Cache cleanup:
Signed-off-by: Greg Wilkins gregw@webtide.com