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.

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

Avoid StringBuilder.append(Object) in ContentDisposition #25056

Merged
merged 1 commit into from
May 12, 2020

Conversation

stsypanov
Copy link
Contributor

@stsypanov stsypanov commented May 12, 2020

The return type of expression (c == '"' && !escaped) ? "\\\"" : c is Object, which implies a call to StringBuilder.append(Object).

The implementation of the following means that lots of garbage objects are allocated as a result of String.valueOf().

@Override
public StringBuilder append(Object obj) {
    return append(String.valueOf(obj));
}

Instead of covariant append we can use exact signatures, i.e. one taking char and one taking String which allows us to avoid unnecessary allocations.

This benchmark demonstrates significant improvement even for filename of length = 10:

JDK 8

Benchmark                              (latin)  (length)  Mode  Cnt     Score     Error   Units
appendCovariant                           true        10  avgt   50   180.230 ±  10.346   ns/op
appendExact                               true        10  avgt   50    68.517 ±   1.479   ns/op

appendCovariant                          false        10  avgt   50   177.713 ±   4.438   ns/op
appendExact                              false        10  avgt   50    67.798 ±   1.364   ns/op

appendCovariant:·gc.alloc.rate.norm       true        10  avgt   50   688.000 ±   0.001    B/op
appendExact:·gc.alloc.rate.norm           true        10  avgt   50   112.000 ±   0.001    B/op

appendCovariant:·gc.alloc.rate.norm      false        10  avgt   50   816.000 ±   0.001    B/op
appendExact:·gc.alloc.rate.norm          false        10  avgt   50   112.000 ±   0.001    B/op

JDK 14

Benchmark                              (latin)  (length)  Mode  Cnt     Score     Error   Units
appendCovariant                           true        10  avgt   50   228.858 ±  18.627   ns/op
appendExact                               true        10  avgt   50    57.950 ±   2.660   ns/op

appendCovariant                          false        10  avgt   50   292.879 ±  12.408   ns/op
appendExact                              false        10  avgt   50    90.228 ±   2.277   ns/op

appendCovariant:·gc.alloc.rate.norm       true        10  avgt   50   688.026 ±   0.002    B/op
appendExact:·gc.alloc.rate.norm           true        10  avgt   50   112.004 ±   0.001    B/op

appendCovariant:·gc.alloc.rate.norm      false        10  avgt   50  1096.040 ±   0.002    B/op
appendExact:·gc.alloc.rate.norm          false        10  avgt   50   200.008 ±   0.001    B/op

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label May 12, 2020
@sbrannen sbrannen self-assigned this May 12, 2020
@sbrannen sbrannen added type: task A general task and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels May 12, 2020
@sbrannen sbrannen added this to the 5.3 M1 milestone May 12, 2020
@sbrannen sbrannen changed the title Rid covariant StringBuilder.append(Object) in favour of exact method call Avoid covariant StringBuilder.append(Object) in favour of exact method call May 12, 2020
@sbrannen sbrannen changed the title Avoid covariant StringBuilder.append(Object) in favour of exact method call Avoid StringBuilder.append(Object) in ContentDisposition May 12, 2020
@sbrannen sbrannen merged commit 6305a69 into spring-projects:master May 12, 2020
@sbrannen
Copy link
Member

This has been merged into master.

Thanks

@stsypanov stsypanov deleted the covar branch May 12, 2020 16:44
@rstoyanchev
Copy link
Contributor

Good catch!

kenny5he pushed a commit to kenny5he/spring-framework that referenced this pull request Jun 21, 2020
This commit avoids invoking StringBuilder.append(Object) in favor
of explicit method calls to append(String) and append(char) in
ContentDisposition.escapeQuotationsInFilename(String).

Closes spring-projectsgh-25056
izeye added a commit to izeye/samples-jmh-gradle that referenced this pull request Feb 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: task A general task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants