aatxe / irc

the irc crate – usable, async IRC for Rust
Mozilla Public License 2.0
533 stars 99 forks source link

WIP: Allocation/borrowing optimizations, part 1 #179

Closed agausmann closed 4 years ago

agausmann commented 5 years ago

This is part of an effort to resolve #32 by replacing owned strings with borrows, Cows, and generics where possible. The most straightforward part is the Message type which now references a single buffer containing the message's serialization. It avoids additional allocations by using iterators for parameters/tags instead of storing them in collections.

This PR also resolves #172 as part of the re-implementation of the message type. delayed

TODO

Unresolved questions

aatxe commented 5 years ago

Thank you for starting the ball rolling on this!

re: your unresolved questions:

  1. Off the top of my head, I think there are two reasonable possibilities here. We could follow the same kind of pattern you've done with Message or we can go the interning approach since prefixes and especially commands will be coming from a relatively small pool consistently.

  2. I think my preference would be based on how isolated each change set is. Since it seems like you can do them separately, I think it's not unreasonable to do them as separate pull requests. But if that's not tractable for some reason, that's alright too.

agausmann commented 5 years ago

Re:

  1. I don't have much experience with interning. At first glance, it might not help much; it seems easier and more straightforward to base most of the zero-allocation on borrows. It may still be useful if at some point we need to extend the lifetime of something.

  2. Yes, we certainly can isolate the changes to different parts of the API, and I think I would prefer it that way too. In the past I have accidentally created massive pull requests by solving one issue and realizing I don't really like the way other things integrate with it, so I solve it by redesigning them all at the same time. Just trying to avoid that :)

agausmann commented 5 years ago

As I'm talking about mega-PRs, I'm realizing that combining suffix into args is a big change that I might want to separate from this...

agausmann commented 5 years ago

Rebased on latest 0.14.