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

made string splits *much* faster #218

Merged
merged 3 commits into from
Oct 4, 2016
Merged

Conversation

dhiltonp
Copy link
Contributor

@dhiltonp dhiltonp commented Sep 22, 2016

This is in a simple case, not with regexes. I suspect there is a similar optimization available in the neighboring code, but I wanted to get some feedback on this change first.

Before: BenchmarkSplit-8 20000 77864 ns/op
After: BenchmarkSplit-8 200000 8017 ns/op

Test run with go test -bench=BenchmarkString_split

edit:
changed comment to reflect move of test to strings_test.go.

@stevenh
Copy link
Collaborator

stevenh commented Sep 22, 2016

Can you include the benchmark as part of the PR too please.

@dhiltonp
Copy link
Contributor Author

Done.

@dhiltonp
Copy link
Contributor Author

I've done a little benchmarking of the regex sections of string.split().

As is, the same performance improvement cannot be directly applied, as golang's default regex library doesn't include undefined matches in this case: A<B>bold</B>and<CODE>coded</CODE>".split(/<(\/)?([^<>]+)>/).

However, assuming the replacement regex library (as in issues #205 and #215) performs splits resulting in undefined matches correctly, we can remove about 50 LoC from split and gain a 33% speed up in regex splits.

before: BenchmarkString_splitWithRegex-8 30000 40421 ns/op
after: BenchmarkString_splitWithRegex-8 50000 27063 ns/op


I'm currently adding benchmarks for related string operations, which will be useful for determining regressions introduced by #205.

@dhiltonp
Copy link
Contributor Author

Any additional input?

@deoxxa
Copy link
Collaborator

deoxxa commented Oct 4, 2016

Looks great, thanks! Sorry for the delay!

@deoxxa deoxxa merged commit bda76a0 into robertkrimen:master Oct 4, 2016
dhiltonp added a commit to dhiltonp/otto that referenced this pull request Oct 31, 2016
@dhiltonp dhiltonp mentioned this pull request Oct 31, 2016
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.

3 participants