chore!: refactor PackageURL by moving String functions to StringUtil #220
Conversation
jeremylong
changed the title
|
➕ 1 for refactoring |
| * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE | ||
| * SOFTWARE. | ||
| */ | ||
| package com.github.packageurl.utils; |
There was a problem hiding this comment.
Since these classes are not meant to be used by third-party libraries, I would suggest:
- Using
com.github.packageurl.internalor similar as package name. - Documenting that the package is internal in
package-info.java.
Due to #201, this package will not be exported through JPMS, since package-info.java is not annotated with @Export.
Sorry, I just saw the change to |
|
This PR also made minor changes to the Benchmark of updated percent decode/encode:
|
|
I figured out the problem with the benchmark and I'm re-running it now. |
The |
|
I can update the benchmark as part of this PR. I'd push the code - but I'm running the benchmark now and I'd like to see the results in another 2 hours after it runs on both pre and post my updates. |
I fixed and extended the benchmark in #222. |
|
Since if (source.indexOf(PERCENT_CHAR) == -1) {
return source;
}I am not sure if |
|
After updating to use the new benchmark. You'll notice that there isn't a lot of change in the With my changes:
Legacy version:
|
JMH tests have a duration expressed in seconds and do not depend on the machine. 😉 |
|
In #224 I expanded on this PR by optimizing the case when no percent encoding is needed. Profiling has shown that |
jeremylong
force-pushed
the
scratch/refactor-stringutils
branch
from
006e58a to
8f17eee
Compare
* feat: Improve benchmark (#222) Fixes a bug in the benchmark initialization and adds a `toLowerCase` benchmark. * fix: Benchmark initialization The benchmark **must** be initialized in a `@Setup` method, otherwise `nonAsciiProb` will always be `0.0`. * fix: Improve encoding/decoding performance for ASCII strings Since strings that don't require **any** percent encoding are in practice the rule, the encoding/decoding code should be optimized for this case.
|
@ppkarwasz I think this is good to go. I don't think we can get much more optimization out of the encode/decode and moving the string functions to their own class helps clean up the |
There was a problem hiding this comment.
LGTM
Around 100 ns per operation on a 256 character long String looks good enough to me.
Maybe we could split the toLowerCase and toLowerCaseJre benchmark method to a separate benchmark class: right now these methods use the test strings for encoding, so there are no favorable test string (e.g. a string with only lowercase characters).
StringUtilclasspublic @Nullable String uriDecode(final @Nullable String source)@since 1.6.0to@since 2.0.0(this was missed in build: bump major version #219)