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

Optimize some string handling #170

Merged
merged 2 commits into from
Jun 2, 2017
Merged

Conversation

moorereason
Copy link
Contributor

@moorereason moorereason commented May 31, 2017

I've added an additional commit to this PR that further optimizes this area of code:

benchmark                       old ns/op     new ns/op     delta
BenchmarkTreeToTomlString-2     49028         18817         -61.62%

benchmark                       old allocs     new allocs     delta
BenchmarkTreeToTomlString-2     485            78             -83.92%

benchmark                       old bytes     new bytes     delta
BenchmarkTreeToTomlString-2     9749          4961          -49.11%

benchmark                       old ns/op     new ns/op     delta
BenchmarkTreeToTomlString-2     49028         43234         -11.82%

benchmark                       old allocs     new allocs     delta
BenchmarkTreeToTomlString-2     485            453            -6.60%

benchmark                       old bytes     new bytes     delta
BenchmarkTreeToTomlString-2     9749          9187          -5.76%
@@ -111,7 +111,7 @@ func (t *Tree) writeTo(w io.Writer, indent, keyspace string, bytesCount int64) (
return bytesCount, err
}

kvRepr := fmt.Sprintf("%s%s = %s\n", indent, k, repr)
kvRepr := indent + k + " = " + repr + "\n"
Copy link
Contributor

Choose a reason for hiding this comment

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

You could probably speed this up even further by creating a bytes.Buffer outside of the loop range, writing to it inside the for loop, then calling w.Write(buf.Bytes()), which would also skip the string conversion below this line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried your suggestion, but it only added allocs. Perhaps I'm doing it wrong.

Compared to prior commit:

benchmark                       old ns/op     new ns/op     delta
BenchmarkTreeToTomlString-2     43234         18817         -56.48%

benchmark                       old allocs     new allocs     delta
BenchmarkTreeToTomlString-2     453            78             -82.78%

benchmark                       old bytes     new bytes     delta
BenchmarkTreeToTomlString-2     9187          4961          -46.00%
@moorereason moorereason changed the title Don't use fmt.Sprintf on simple strings Optimize some string handling May 31, 2017
@kevinburke
Copy link
Contributor

Here's what I had in mind. It makes the code a lot simpler, since we know that b.WriteString and company never return an error, we don't need to check them constantly.

b49b944

@pelletier
Copy link
Owner

@moorereason nice!
@kevinburke I like your proposition (makes the code much cleaner), but the +14% allocs increase isn't great. What happens with a larger preallocated buffer?

I think I'm going to merge this as a first step. Then a second step we should figure a way to make Kevin's solution use some less allocs, as it makes the code much more readable.

@pelletier pelletier merged commit a60c713 into pelletier:master Jun 2, 2017
@pelletier
Copy link
Owner

Ref #167

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