Replace Strings with FastStr #211
Conversation
|
I'm not convinced this is better, since it adds a dependency (code size, compile times) and there's no benchmark to compare before and after. The Not to mention that is this a severely breaking change and will cause headache on the crate's users. |
|
It's okay, I can benchmark this approach later on. I didn't get about breaking change, because public API is same. Can you suggest some scenarios to bench? |
| @@ -248,7 +249,7 @@ impl<T: Transport, L: LoginCredentials> ClientLoopWorker<T, L> { | |||
| /// | |||
| /// The client will make best attempts to stay joined to this channel. I/O errors will be | |||
| /// compensated by retrying the join process. For this reason, this method returns no error. | |||
| fn join(&mut self, channel_login: String) { | |||
There was a problem hiding this comment.
All of these kinds of changes, where the type in a struct or the argument type of a function changes, are breaking changes.
There was a problem hiding this comment.
Oh, looks to be missed. I've planned to rewrite it into impl Into<FastStr>, to save backwards compatibility.
I've just not gotten around to it, but a large sample (maybe 10k-100k) actual messages logged from various twitch channels should give a realistic real-use scenario, run through the message parser (parsing to |
I am not sure, did I patch parser at all. I confirm that's a lot of |
| pub struct IRCMessage { | ||
| /// A map of additional key-value tags on this message. | ||
| pub tags: IRCTags, | ||
| /// The "prefix" of this message, as defined by RFC 2812. Typically specifies the sending | ||
| /// server and/or user. | ||
| pub prefix: Option<IRCPrefix>, | ||
| /// A command like `PRIVMSG` or `001` (see RFC 2812 for the definition). | ||
| pub command: String, | ||
| pub command: FastStr, |
There was a problem hiding this comment.
Of course you did, for example here and approx. 80% of the files changed are the message data types.
|
Thank you for looking into making a benchmark! |
Replaced Strings type with FastStr, removed a lot of
to_owned(), some autoformatting.Pull request checklist:
CHANGELOG.mdwas updated, if applicable