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

WIP: Allocation/borrowing optimizations, part 1 #179

Closed
wants to merge 14 commits into from

Conversation

agausmann
Copy link
Contributor

@agausmann agausmann commented Jun 10, 2019

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

  • Re-implement prefix parsing.
  • Add parser tests.
  • Integrate the new Message type with the other APIs.
  • Add more builders/constructors for Message.

Unresolved questions

  • How do we want to implement this in the broader scale? Command and Prefix are also going to have to be rewritten to allow zero-allocation as they liberally use String, but we still haven't really discussed our options here.

  • Do we want to split these other changes into different PRs, or would it be best to review and merge together?

@aatxe
Copy link
Owner

aatxe commented Jun 10, 2019

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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 agausmann force-pushed the optimized_message branch 2 times, most recently from 39b39dc to aa4f08a Compare June 13, 2019 16:49
agausmann added 14 commits June 13, 2019 11:51
- Made some parts of the documentation less ambiguous about borrows.
- Updated parser documentation to reflect new parameters.
- Removed the 15-parameter restriction, replacing it with a
parser-iterator (even though servers/clients SHOULD NOT send more than
15, they MUST be able to handle such messages).
- Combined suffix/trailing parameter with middle parameters, as it MUST
be treated the same as if it were a final parameter ("PRIVMSG #foo
:Hello" and "PRIVMSG #foo Hello" are semantically equivalent).
@agausmann
Copy link
Contributor Author

agausmann commented Jun 13, 2019

Rebased on latest 0.14.

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