Improve IncludeExcludeSet testing
#9071
Conversation
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
jetty-util/src/main/java/org/eclipse/jetty/util/IncludeExcludeSet.java
Outdated
Show resolved
Hide resolved
| if (!_includes.isEmpty()) | ||
| { | ||
| // We have includes defined, if input has no match, return false immediately | ||
| if (!_includePredicate.test(t)) | ||
| return false; | ||
| // We have an include match, let excludes override if needed. | ||
| } |
There was a problem hiding this comment.
I don't find the cascading ifs with hanging comment clearer. How about:
| if (!_includes.isEmpty()) | |
| { | |
| // We have includes defined, if input has no match, return false immediately | |
| if (!_includePredicate.test(t)) | |
| return false; | |
| // We have an include match, let excludes override if needed. | |
| } | |
| // If we have defined includes, but none match then | |
| if (!_includes.isEmpty() && !_includePredicate.test(t)) | |
| { | |
| // return false immediately, no need to check excluded | |
| return false; | |
| } | |
| // We are included by omission or match, so we now check excludes |
There was a problem hiding this comment.
This is actually one of the implementations we tried.
This was the "simplified" algorithm from ...
https://github.com/eclipse/jetty.project/blob/fix/jetty-12.0.x/improve-includeexcludeset/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/IncludeExcludeSet.java#L160-L164
It (surprisingly) is 10% slower in the jmh testing than what we are using now.
I'll be happy to adjust the comments though.
There was a problem hiding this comment.
It should be identical, but it wasn't.
There was a problem hiding this comment.
I'm very dubious of the test if they are not identical!
There was a problem hiding this comment.
I'm very dubious of the test if they are not identical!
You aren't the only one, even @sbordet and @lorban had the same initial reaction, and even with some jmh test tweaking couldn't make it identical.
Even testing both techniques side by side on the same jmh run showed the difference! (consistently the same difference each jmh run)
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
Improve testing and code coverage of
IncludeExcludeSet.Updating
IncludeExcludeSetfor best performing.test(input)method.(jmh testing of different algorithms shows this one to be fastest)
Signed-off-by: Joakim Erdfelt joakim.erdfelt@gmail.com