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

Fix WrapWords excessive memory utilization #205

Merged
merged 1 commit into from
Sep 25, 2023

Conversation

horkhe
Copy link
Contributor

@horkhe horkhe commented Aug 25, 2022

When feeding production data to this library we discovered that function WordWrap had O(n*n) memory complexity, that resulted in huge amounts of memory allocated when processing even relatively small strings. We are talking Gigabytes that made our production services killed with OOM. This PR implements 2 optimisations:

  • Function splitWords was introduced to efficiently break a line into words. Note that unlike original implementation it does not consider empty spaces to be words.
  • Function WrapWords was rewritten to be O(n) on memory complexity. A benchmark that comes with this PR shows great improvement:
BenchmarkWrapString-16
Before:     1	2490331031 ns/op	2535184104 B/op	50905550 allocs/op
After:   1652	    658098 ns/op	    230223 B/op	    5176 allocs/op

@horkhe
Copy link
Contributor Author

horkhe commented Sep 12, 2022

@olekukonko any chance to get this PR looked at?

@olekukonko
Copy link
Owner

@horkhe sorry for the late response, this is wonderful, will take my time to look at this next week so that it can be merged

@olekukonko
Copy link
Owner

@horkhe can you kindly update your PR and resolve merge conflicts, I Would really like to merge this performance improvement. thank you

@horkhe
Copy link
Contributor Author

horkhe commented Jun 15, 2023

@olekukonko sorry for the late response. I have rebased this PR, please take a look.

@horkhe
Copy link
Contributor Author

horkhe commented Jul 20, 2023

@olekukonko any chance to get this merged?

1 similar comment
@horkhe
Copy link
Contributor Author

horkhe commented Sep 15, 2023

@olekukonko any chance to get this merged?

@olekukonko
Copy link
Owner

@horkhe will look at this this weekend and revert. Thank you for being consistent.

@horkhe
Copy link
Contributor Author

horkhe commented Sep 25, 2023

@olekukonko a friendly reminder.

@olekukonko olekukonko merged commit df64c4b into olekukonko:master Sep 25, 2023
@horkhe
Copy link
Contributor Author

horkhe commented Sep 28, 2023

Thanks a lot! Could you please also tag the master with v0.0.6, so it can be picked up by go mod.

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.

2 participants