Skip to content
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.

Sign up for GitHub

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

Jump to bottom

StringBuilderPool #2

Closed
wants to merge 5 commits into from
Closed

StringBuilderPool #2

wants to merge 5 commits into from

Conversation

igr
Copy link
Member

@igr igr commented Sep 27, 2020

No description provided.

@igr igr mentioned this pull request Sep 27, 2020
Closed
slandelle
slandelle suggested changes Sep 27, 2020
View reviewed changes
Copy link
Contributor

@slandelle slandelle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stupid me: forgot to reset pooled StringBuilders...

src/main/java/jodd/util/StringBuilderPool.java Outdated Show resolved Hide resolved
igr and others added 2 commits September 27, 2020 17:59
@igr @slandelle
Update src/main/java/jodd/util/StringBuilderPool.java
9575785 
Co-authored-by: Stephane Landelle <slandelle@gatling.io>
@igr @slandelle
Update src/main/java/jodd/net/HtmlDecoder.java
7865127 
Co-authored-by: Stephane Landelle <slandelle@gatling.io>
@igr igr marked this pull request as ready for review September 27, 2020 15:59
@codecov
Copy link

codecov bot commented Sep 27, 2020 •
edited
Loading

Codecov Report

Merging #2 into master will decrease coverage by 0.08%.
The diff coverage is 93.05%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master       #2      +/-   ##
============================================
- Coverage     74.11%   74.03%   -0.09%     
- Complexity     2878     2886       +8     
============================================
  Files           122      123       +1     
  Lines          9179     9212      +33     
  Branches       1920     1923       +3     
============================================
+ Hits           6803     6820      +17     
- Misses         1881     1896      +15     
- Partials        495      496       +1
Impacted Files Coverage Δ Complexity Δ
src/main/java/jodd/net/HtmlEncoder.java 93.18% <71.42%> (ø) 12.00 <2.00> (+1.00)
src/main/java/jodd/util/StringBuilderPool.java 94.11% <94.11%> (ø) 6.00 <6.00> (?)
src/main/java/jodd/net/HtmlDecoder.java 87.00% <94.44%> (+0.13%) 19.00 <8.00> (+1.00)
src/main/java/jodd/util/StringUtil.java 96.14% <100.00%> (ø) 517.00 <4.00> (ø)
src/main/java/jodd/io/StreamGobbler.java 0.00% <0.00%> (ø) 0.00% <0.00%> (ø%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 159a2a9...c03d01f. Read the comment docs.

slandelle and others added 2 commits September 28, 2020 14:13
@slandelle @igr
StringUtil#remove(char): copy chars by chunk instead of one by one
99ad746 
Motivation:

Chars are currently copied one by one in the StringBuilder.

Modification:

Use String#indexOf to locate char position and then StringBuilder#append(string, start, end) to copy chars by chunk.

Result:

Better performance.
@igr
JMH results added
c03d01f
@igr
Copy link
Member Author

igr commented Sep 28, 2020

@slandelle please see my benchmark

StringUtil_removeBenchmark.removeNew                a  thrpt    5  29089.686 ± 222.621  ops/ms
StringUtil_removeBenchmark.removeNew               ba  thrpt    5  24278.007 ± 145.268  ops/ms
StringUtil_removeBenchmark.removeNew              cab  thrpt    5  22196.510 ± 212.803  ops/ms
StringUtil_removeBenchmark.removeNew            abcde  thrpt    5  23367.545 ± 430.193  ops/ms
StringUtil_removeBenchmark.removeNew  bcdefghaijklman  thrpt    5  17268.509 ± 163.351  ops/ms
StringUtil_removeBenchmark.removeOld                a  thrpt    5  57116.090 ± 881.963  ops/ms
StringUtil_removeBenchmark.removeOld               ba  thrpt    5  40605.504 ± 463.230  ops/ms
StringUtil_removeBenchmark.removeOld              cab  thrpt    5  40103.663 ± 609.040  ops/ms
StringUtil_removeBenchmark.removeOld            abcde  thrpt    5  35670.546 ± 535.558  ops/ms
StringUtil_removeBenchmark.removeOld  bcdefghaijklman  thrpt    5  29990.477 ± 348.026  ops/ms

Am i doing something wrong?

@slandelle
Copy link
Contributor

@igr Sadly, it looks like the new strategy is only a win for large Strings. I think you can revert latest commit.

@igr igr closed this Sep 30, 2020
@slandelle
Copy link
Contributor

@igr Maybe I wasn't clear. The StringBuilderPool is definitely a good thing. Only this commit 99ad746 wasn't.

@igr igr reopened this Sep 30, 2020
@slandelle
Copy link
Contributor

I don't own the PR, otherwise I would have remove the wrong commit.

@igr
Copy link
Member Author

igr commented Oct 4, 2020

Closing this one and opening: #4

@igr igr closed this Oct 4, 2020
@igr igr mentioned this pull request Dec 11, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@slandelle slandelle slandelle requested changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@igr @slandelle