-
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
PathMappings optimizations #9055
Conversation
Avoid iterations if only ServletPathSpec instances Avoid tests for empty mappings. Better reset implementation
Avoid iterations if only ServletPathSpec instances Avoid tests for empty mappings. Better reset implementation
This is a more comprehensive alternative to #9053 |
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.
Needs more tests
jetty-http/src/main/java/org/eclipse/jetty/http/pathmap/PathMappings.java
Show resolved
Hide resolved
Improve suffix matching
jetty-http/src/main/java/org/eclipse/jetty/http/pathmap/PathMappings.java
Show resolved
Hide resolved
Improve exact matching
It feels like a set of micro benchmarks would help now that this class' perf appeared on the radar. |
Maybe, but these changes are primarily removing work that was never actually necessary, so it is more of a bug fix than an optimization. I don't think we should hold up a merge of this whilst over analyzing it. I'd like to get this merged and only if we then see it on flame graphs should we drill down with micro benchmarks. |
jetty-http/src/main/java/org/eclipse/jetty/http/pathmap/PathMappings.java
Show resolved
Hide resolved
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.
Approved.
Should probably merge PR #9071 first (or revert your IncludeExcludeSet change in this PR)
Avoid iterations if only ServletPathSpec instances
Avoid tests for empty mappings.
Better reset implementation