Skip to content

strings: use Builder in Map #26304

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
Closed
rsc opened this issue Jul 10, 2018 · 7 comments
Closed

strings: use Builder in Map #26304

rsc opened this issue Jul 10, 2018 · 7 comments
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done. Performance
Milestone
Go1.12

Comments

@rsc
Copy link
Contributor

rsc commented Jul 10, 2018

@robpike and I were just looking at strings.Map. It should use Builder to avoid a copy at the end in the conversion from []byte to string.
@rsc rsc added this to the Go1.12 milestone Jul 10, 2018
@ianlancetaylor ianlancetaylor added NeedsFix The path to resolution is known, but the work has not been done. help wanted labels Jul 10, 2018
@rsc rsc mentioned this issue Jul 10, 2018
Closed
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/122875 mentions this issue: strings: use Builder in Map

@fraenkel
Copy link
Contributor

There is a performance issue that I ran across when trying to use the Builder. The last loop which copies by byte or rune should be able to to just use builder.WriteRune(r), however, it was always ~30% slower to do so.

@zigo101
Copy link

zigo101 commented Jul 11, 2018

Builder is not always faster, in particular for this specified case: https://groups.google.com/forum/#!topic/golang-nuts/I6Ch7hMEQLk

@josharian
Copy link
Contributor

@fraenkel want to file an independent issue?

@fraenkel
Copy link
Contributor

@josharian We did determine its due to inlining. There is also another issue around which is focused on strings vs []bytes, #26264

@iamoryanmoshe
Copy link
Contributor

Is this issue still relevant? Should I check it out?

@tmthrgd tmthrgd mentioned this issue Sep 26, 2018
Closed
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/137575 mentions this issue: strings: use Builder in ToUpper and ToLower

gopherbot pushed a commit that referenced this issue Sep 26, 2018
@tmthrgd @bradfitz
strings: use Builder in ToUpper and ToLower
14e7f17 
Map was optimized to use Builder in 45c7d80, which avoided the []byte
to string converstion. This left the ToUpper and ToLower ASCII fast path
with an extra allocation over Map.

name                                        old time/op    new time/op    delta
ToUpper/#00-12                                3.59ns ± 4%    3.71ns ± 1%     ~     (p=0.056 n=5+5)
ToUpper/ONLYUPPER-12                          11.8ns ± 2%    10.5ns ± 2%  -10.85%  (p=0.008 n=5+5)
ToUpper/abc-12                                31.8ns ± 1%    25.3ns ± 1%  -20.40%  (p=0.008 n=5+5)
ToUpper/AbC123-12                             46.2ns ± 7%    31.9ns ± 8%  -30.89%  (p=0.008 n=5+5)
ToUpper/azAZ09_-12                            47.1ns ± 8%    32.6ns ± 4%  -30.77%  (p=0.008 n=5+5)
ToUpper/longStrinGwitHmixofsmaLLandcAps-12     137ns ±15%     104ns ±11%  -24.11%  (p=0.008 n=5+5)
ToUpper/longɐstringɐwithɐnonasciiⱯchars-12     231ns ± 1%     228ns ± 1%     ~     (p=0.079 n=5+5)
ToUpper/ɐɐɐɐɐ-12                               207ns ± 3%     206ns ± 1%     ~     (p=0.913 n=5+5)
ToUpper/a\u0080\U0010ffff-12                  90.8ns ± 1%    89.6ns ± 1%   -1.30%  (p=0.024 n=5+5)
ToLower/#00-12                                3.59ns ± 1%    4.26ns ± 2%  +18.66%  (p=0.008 n=5+5)
ToLower/abc-12                                6.32ns ± 1%    6.62ns ± 1%   +4.72%  (p=0.008 n=5+5)
ToLower/AbC123-12                             45.0ns ±13%    31.5ns ± 4%  -29.89%  (p=0.008 n=5+5)
ToLower/azAZ09_-12                            48.8ns ± 6%    33.2ns ± 3%  -31.91%  (p=0.008 n=5+5)
ToLower/longStrinGwitHmixofsmaLLandcAps-12     149ns ±13%      98ns ± 8%  -34.30%  (p=0.008 n=5+5)
ToLower/LONGⱯSTRINGⱯWITHⱯNONASCIIⱯCHARS-12     237ns ± 4%     237ns ± 2%     ~     (p=0.635 n=5+5)
ToLower/ⱭⱭⱭⱭⱭ-12                               181ns ± 1%     181ns ± 1%     ~     (p=0.762 n=5+5)
ToLower/A\u0080\U0010ffff-12                  90.6ns ± 1%    92.5ns ± 1%   +2.05%  (p=0.016 n=5+5)

name                                        old alloc/op   new alloc/op   delta
ToUpper/#00-12                                 0.00B          0.00B          ~     (all equal)
ToUpper/ONLYUPPER-12                           0.00B          0.00B          ~     (all equal)
ToUpper/abc-12                                 6.00B ± 0%     3.00B ± 0%  -50.00%  (p=0.008 n=5+5)
ToUpper/AbC123-12                              16.0B ± 0%      8.0B ± 0%  -50.00%  (p=0.008 n=5+5)
ToUpper/azAZ09_-12                             16.0B ± 0%      8.0B ± 0%  -50.00%  (p=0.008 n=5+5)
ToUpper/longStrinGwitHmixofsmaLLandcAps-12     64.0B ± 0%     32.0B ± 0%  -50.00%  (p=0.008 n=5+5)
ToUpper/longɐstringɐwithɐnonasciiⱯchars-12     48.0B ± 0%     48.0B ± 0%     ~     (all equal)
ToUpper/ɐɐɐɐɐ-12                               48.0B ± 0%     48.0B ± 0%     ~     (all equal)
ToUpper/a\u0080\U0010ffff-12                   16.0B ± 0%     16.0B ± 0%     ~     (all equal)
ToLower/#00-12                                 0.00B          0.00B          ~     (all equal)
ToLower/abc-12                                 0.00B          0.00B          ~     (all equal)
ToLower/AbC123-12                              16.0B ± 0%      8.0B ± 0%  -50.00%  (p=0.008 n=5+5)
ToLower/azAZ09_-12                             16.0B ± 0%      8.0B ± 0%  -50.00%  (p=0.008 n=5+5)
ToLower/longStrinGwitHmixofsmaLLandcAps-12     64.0B ± 0%     32.0B ± 0%  -50.00%  (p=0.008 n=5+5)
ToLower/LONGⱯSTRINGⱯWITHⱯNONASCIIⱯCHARS-12     48.0B ± 0%     48.0B ± 0%     ~     (all equal)
ToLower/ⱭⱭⱭⱭⱭ-12                               32.0B ± 0%     32.0B ± 0%     ~     (all equal)
ToLower/A\u0080\U0010ffff-12                   16.0B ± 0%     16.0B ± 0%     ~     (all equal)

name                                        old allocs/op  new allocs/op  delta
ToUpper/#00-12                                  0.00           0.00          ~     (all equal)
ToUpper/ONLYUPPER-12                            0.00           0.00          ~     (all equal)
ToUpper/abc-12                                  2.00 ± 0%      1.00 ± 0%  -50.00%  (p=0.008 n=5+5)
ToUpper/AbC123-12                               2.00 ± 0%      1.00 ± 0%  -50.00%  (p=0.008 n=5+5)
ToUpper/azAZ09_-12                              2.00 ± 0%      1.00 ± 0%  -50.00%  (p=0.008 n=5+5)
ToUpper/longStrinGwitHmixofsmaLLandcAps-12      2.00 ± 0%      1.00 ± 0%  -50.00%  (p=0.008 n=5+5)
ToUpper/longɐstringɐwithɐnonasciiⱯchars-12      1.00 ± 0%      1.00 ± 0%     ~     (all equal)
ToUpper/ɐɐɐɐɐ-12                                2.00 ± 0%      2.00 ± 0%     ~     (all equal)
ToUpper/a\u0080\U0010ffff-12                    1.00 ± 0%      1.00 ± 0%     ~     (all equal)
ToLower/#00-12                                  0.00           0.00          ~     (all equal)
ToLower/abc-12                                  0.00           0.00          ~     (all equal)
ToLower/AbC123-12                               2.00 ± 0%      1.00 ± 0%  -50.00%  (p=0.008 n=5+5)
ToLower/azAZ09_-12                              2.00 ± 0%      1.00 ± 0%  -50.00%  (p=0.008 n=5+5)
ToLower/longStrinGwitHmixofsmaLLandcAps-12      2.00 ± 0%      1.00 ± 0%  -50.00%  (p=0.008 n=5+5)
ToLower/LONGⱯSTRINGⱯWITHⱯNONASCIIⱯCHARS-12      1.00 ± 0%      1.00 ± 0%     ~     (all equal)
ToLower/ⱭⱭⱭⱭⱭ-12                                1.00 ± 0%      1.00 ± 0%     ~     (all equal)
ToLower/A\u0080\U0010ffff-12                    1.00 ± 0%      1.00 ± 0%     ~     (all equal)

Updates #26304

Change-Id: I4179e21d5e60d950b925fe3ffc74b376b82812d2
GitHub-Last-Rev: 2c7c3bb
GitHub-Pull-Request: #27872
Reviewed-on: https://go-review.googlesource.com/137575
Run-TryBot: Ian Lance Taylor <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Brad Fitzpatrick <[email protected]>
@golang golang locked and limited conversation to collaborators Sep 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done. Performance
Projects
None yet
Milestone
Go1.12
Development

No branches or pull requests
8 participants
@bradfitz @josharian @rsc @fraenkel @ianlancetaylor @gopherbot @zigo101 @iamoryanmoshe