-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Fix #5562 Improve HTTP Field cache allocation #5565
Conversation
Fix #5562 by initially putting cacheable fields into a inexpensive arraylist. Only create the Trie (with space and complexity costs) if a second request is received.
…62-improve-field-cache
…62-improve-field-cache
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not particularly fond of this change.
I am positive on delaying the creation of the Trie, until we know it's a HTTP/1.1 and we do have cache size > 0.
Seems to me that the chances for caching are the most common case by far, so I would skip the temporary list and go straight to the trie.
jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java
Outdated
Show resolved
Hide resolved
jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java
Outdated
Show resolved
Hide resolved
jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java
Outdated
Show resolved
Hide resolved
jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java
Outdated
Show resolved
Hide resolved
Create `HttpHeader.isPseudo()`` method improved clarity with `createFieldCacheIfNeeded()``
@gregw also, I think we need some test -- something that verifies that the field is |
@sbordet There are cache tests and they were broken until I fixed |
Only defer Trie creation to first cacheable field, not until next request.
@lorban see the emptyTrie method I've added here. Also I wonder why the HttpField cache uses the Terniary Trie rather than just the array Trie. I think this is one of the most important (if not THE important) case to optimise Tries for. We basically have the static and dynamic Tries... it is almost like HTTP/2 Hpack. |
jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java
Outdated
Show resolved
Hide resolved
jetty-util/src/main/java/org/eclipse/jetty/util/AbstractTrie.java
Outdated
Show resolved
Hide resolved
jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java
Outdated
Show resolved
Hide resolved
+ more javadoc + empty set return
@lorban nudge |
Fix #5562 by initially putting cacheable fields into a inexpensive arraylist.
Only create the Trie (with space and complexity costs) if a second request is received.
Edit: the original PR only created the Trie on the second request, but that behaviour was removed in reviews. The cache is now created every request, even for non persistent requests.