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

Decrease allocations. Don't force caller to allocate. #13

Merged
merged 5 commits into from
Apr 21, 2017

Conversation

insanitybit
Copy link
Contributor

Hey, I've been using dogstatsd, and it's great, but it has forced me to convert some slices to vecs, &str to String, and things like that, incurring copies.

I noticed the internal code also does more copying than is necessary. It also uses format!, which doesn't preallocate buffers.

I went ahead and replaced all of the Vec and String with slices where applicable. I've also replaced all format! macros with string building, and shuffled a few branches around.

The performance improvement is around 5%. It does break the API. S is now Into<&str> instead of Into<String<, which I think is breaking. Vec becoming &[&str] is definitely breaking.

Before:

test bench::bench_decr ... bench: 5,562 ns/iter (+/- 443)
test bench::bench_event ... bench: 5,383 ns/iter (+/- 1,582)
test bench::bench_gauge ... bench: 5,701 ns/iter (+/- 409)
test bench::bench_histogram ... bench: 5,644 ns/iter (+/- 362)
test bench::bench_incr ... bench: 5,545 ns/iter (+/- 323)
test bench::bench_set ... bench: 5,645 ns/iter (+/- 324)
test bench::bench_timing ... bench: 5,544 ns/iter (+/- 292)
test metrics::bench::bench_format_for_send ... bench: 333 ns/iter (+/- 15)
test metrics::bench::bench_set_counter ... bench: 86 ns/iter (+/- 3)
test metrics::bench::bench_set_metric ... bench: 202 ns/iter (+/- 99)

After:

test bench::bench_decr ... bench: 5,123 ns/iter (+/- 482)
test bench::bench_event ... bench: 5,397 ns/iter (+/- 350)
test bench::bench_gauge ... bench: 5,218 ns/iter (+/- 504)
test bench::bench_histogram ... bench: 5,196 ns/iter (+/- 316)
test bench::bench_incr ... bench: 5,099 ns/iter (+/- 332)
test bench::bench_set ... bench: 5,241 ns/iter (+/- 419)
test bench::bench_timing ... bench: 5,264 ns/iter (+/- 447)
test metrics::bench::bench_format_for_send ... bench: 111 ns/iter (+/- 7)
test metrics::bench::bench_set_counter ... bench: 29 ns/iter (+/- 0)
test metrics::bench::bench_set_metric ... bench: 32 ns/iter (+/- 1)

@nastevens
Copy link
Contributor

Hey @insanitybit - sorry to jump in on your PR unasked, but I had been looking at doing something almost identical so I had some ideas on this. Rather than using &str and slices though, I was looking at Into<Cow<'a, str>> as described in From &str to Cow. The API when doing this seems really nice, as it pretty much makes legal all variations of &str, String, etc.

Similarly on the change from Vec<> to slices, I have had really good results having API functions take I: IntoIterator<Item=S>, S: AsRef<str>. This allows any iterable of anything that can be referenced as a str to be used. So Vec<String> is still legal, but so is a static array of &str

@mcasper
Copy link
Owner

mcasper commented Apr 12, 2017

Hey @insanitybit, thanks so much for the contribution! I've been meaning to come back through and clean up allocations, this is awesome!

Don't worry about the breaking changes, we're pre 1.0 so we can do whatever we want 😛

I do like @nastevens comments about making the API accept both forms of each argument, so if you have time to update the PR that would be great

@insanitybit
Copy link
Contributor Author

Ah, yeah I remembered I: IntoIterator<Item=S>, S: AsRef but it was late so I just made the PR :P that is really the right way to do it. I'll look into using Into as well.

@insanitybit
Copy link
Contributor Author

I've updated the code to use Cow's. Not sure exactly how to get the API working with the IntoIterator. I'm running into some borrow errors.

@mcasper
Copy link
Owner

mcasper commented Apr 12, 2017

I started implementing it roughly off this branch, looks like most of the type signatures will change to be like:

pub fn incr<'a, S, I>(&self, stat: S, tags: I) -> DogstatsdResult
    where S: Into<Cow<'a, str>>,
          S: AsRef<str>,
          I: IntoIterator<Item=S>
{

The AsRef<str> and IntoIterator<Item=S> types will basically permeate all the way down to format_for_send, at which point we can consume the iterator and add any tags that might be there

@mcasper mcasper merged commit 833447e into mcasper:master Apr 21, 2017
@mcasper
Copy link
Owner

mcasper commented Apr 21, 2017

Thanks again for the contribution @insanitybit! I went ahead and merged this and then pushed up the IntoIterator change, and published a new version with all this great work

@insanitybit
Copy link
Contributor Author

Awesome, thanks for taking it the rest of the way.

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