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

(WIP) Optimize String#gsub with block #6397

Conversation

straight-shoota
Copy link
Member

@straight-shoota straight-shoota commented Jul 16, 2018

When replacing characters in a string, typically only a minority of characters are changed like most most escaping or encoding use-cases. Currently, String#gsub loops through the string, yields each character and writes the return to an IO. However, these single-character writes are not as efficient as writing a larger slice.

The best example for this is a string with no replacement. In the previous implementation, it would be copied byte-by-byte (or rather char-by-char) to a new string. If some characters need to be replaced, the size of the slices that can be copied as whole is smaller, but it's still considerably faster than writing byte-by-byte.

An algorithm that only writes replaced chars and copies a series of unchanged characters in bulk can improve performance considerably.
This approach was previously used by a custom implementation in JSON::Builder#string.

In this PR, the proposed implementation of String#gsub yields each char and only writes the returned value if it is not nil. When using this method, returning nil signals that the current character can be copied as is and multiple nil returns are combined to a larger slice copy.

I also added a method variant receiving an io for better integration. And there is a similar Slice#gsub method, which is utilized for ascii-only strings (ore replacements). This method is part of the public API of Slice because it could be useful for any slice to replace some elements. It requires some sort of builder with an IO API, so currently it's only effectively usable for Slice(UInt8).

replace 6/49 chars: "Alle meine Entchen schwimmen auf dem Sä".gsub(' ', '-')

                   old   1.15M (865.84ns) (±12.39%)  192 B/op   1.36× slower
                   new 883.68k (  1.13µs) (±14.82%)  160 B/op   1.77× slower
new (ascii_only: true)   1.57M (637.87ns) (±10.70%)  160 B/op        fastest
replace all: "äääääääääääääääää".gsub('ä', '-')

                   old   1.03M (971.22ns) (±11.46%)  144 B/op   1.02× slower
                   new 739.62k (  1.35µs) (± 9.79%)  144 B/op   1.42× slower
new (ascii_only: true)   1.05M (948.91ns) (± 9.81%)  144 B/op        fastest
replace none: "Alle-meine-Entchen-schwimmen-auf-dem-Sä".gsub(' ', '-')

                   old   1.57M (638.41ns) (±15.57%)  176 B/op   1.60× slower
                   new   1.17M (852.01ns) (±10.92%)  160 B/op   2.13× slower
new (ascii_only: true)   2.51M (399.19ns) (±15.46%)  160 B/op        fastest

I also tested JSON::Builder#string and the new gsub is even slightly faster than the previous implementation because it doesn't need to decode multibyte chars (they won't be replaced anyway).

simple string
old   4.75M (210.51ns) (±20.85%)  12 B/op   1.15× slower
new   5.45M (183.33ns) (±29.58%)  13 B/op        fastest
many replacements
old 648.03k (  1.54µs) (±18.61%)  149 B/op   1.05× slower
new 683.31k (  1.46µs) (±20.73%)  203 B/op        fastest
normal replacements
old 812.58k (  1.23µs) (±25.99%)  253 B/op   1.25× slower
new   1.01M (985.81ns) (±25.16%)  282 B/op        fastest

Obviously, there are cases where the previous, simple implementation is equal or even more performant then the proposed batch copy. This is typically the case when there is a high amount and equal distribution of replaced chars, especially in ascii-only strings. For some use cases, a customized replace loop can achieve more performance than this generalized String#gsub.

In total, I expect this to be an improvement (backed by the benchmarks) because replacements are usually less frequent in most applications.

@@ -354,6 +354,26 @@ struct Slice(T)
source.move_to(self)
end

def gsub(buffer, &block : T -> _)
Copy link
Contributor

Choose a reason for hiding this comment

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

This only compiles if T is UInt8. Are we sure we want this on slice instead of string?

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like a similar problem as in #6389 where we have methods that make sense only for Slice(UInt8), right?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's somewhat similar. But in contrast to #6389, this method conceptually fits for any slice. It's only that the buffer API is currently implemented by IO which is based on UInt8. But you can implement such a receiver for any type.

I'm not entirely sure about it's usefulnes, so maybe it's still better to move it to String as a private method.

@ysbaddaden
Copy link
Contributor

I don't understand your benchmarks: new is always slower than old ?!

@RX14
Copy link
Contributor

RX14 commented Jul 17, 2018

if old is always better than new in the non-ascii-only case, then why not keep the old implementation in that case?

@straight-shoota
Copy link
Member Author

I'm not sure why the non-ascii performance is so bad. In my initial benchmarks, it showed significant improvement. And the implementation is basically the same as for ascii-only. This needs investigation.

@wooster0
Copy link
Contributor

wooster0 commented Nov 5, 2019

@straight-shoota in the past I've also had benchmarking issues, but sometimes it actually helped to clear the cache: sudo rm -rf ~/.cache/crystal/. It can be glitchy sometimes. Maybe that helps.

@RX14 RX14 closed this Nov 13, 2019
@asterite
Copy link
Member

Yeah, there's a problem with how the compiler's cache works, I think in release it doesn't work quite well. I'll have to investigate, but I'll need reproducible code(s).

@straight-shoota straight-shoota deleted the jm/feature/gsub-optimize branch January 10, 2023 19:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants