From 138895625de35c2902bc55f035b978a60b544184 Mon Sep 17 00:00:00 2001 From: Calvin Lee Date: Mon, 27 Jan 2025 16:41:24 +0000 Subject: [PATCH] Fix various performance issues MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit includes various fixes that I have found working on bouncer-networks. It includes several performance benefits—such as changing `Server` to be an `Arc` which removes much of the copying overhead. I have also made sure that there is only one copy of the server config in the `Halloy` struct, which was previously duplicated for seemingly no reason. Still, several performance issues go un-addressed. such as the duplication of server configs between threads (should this also be an `Arc`?) and the copying of `client` structs. I have also made some changes to the client. The capability negotiation process has been streamlined, and I've eliminated some branching with preference toward pattern matching. I've also fixed a slight error with halloy, making sure that it detects the end of registration correctly (matching other clients). I have also made changes to the `Cargo.toml` in the `data` crate. This is to fix some compilation issues when building that crate in isolation. I would like this fixed so that it'd be easier to run `data` crate tests in isolation. Ideally I think that each of the sub-crates should be tested individually in CI—but that's out of scope for this PR. In my opinion, this resolves #640 --- data/Cargo.toml | 8 +- data/src/client.rs | 212 ++++++++++++++------------------------- data/src/server.rs | 21 +++- irc/proto/src/command.rs | 13 ++- src/main.rs | 50 ++++----- 5 files changed, 132 insertions(+), 172 deletions(-) diff --git a/data/Cargo.toml b/data/Cargo.toml index e47028cd4..9fd535546 100644 --- a/data/Cargo.toml +++ b/data/Cargo.toml @@ -17,7 +17,7 @@ flate2 = "1.0" futures = "0.3.21" hex = "0.4.3" iced_core = "0.14.0-dev" -log = "0.4.16" +log = { version = "0.4.16", features = ['std'] } palette = "0.7.4" rand = "0.8.4" rand_chacha = "0.3.0" @@ -27,8 +27,8 @@ sha2 = "0.10.8" toml = "0.8.11" thiserror = "1.0.30" reqwest = { version = "0.12", features = ["json"] } -tokio = { version = "1.0", features = ["io-util"] } -tokio-stream = { version = "0.1", features = ["time"] } +tokio = { version = "1.0", features = ["io-util", "fs"] } +tokio-stream = { version = "0.1", features = ["time", "fs"] } itertools = "0.12.1" timeago = "0.4.2" url = { version = "2.5.0", features = ["serde"] } @@ -46,4 +46,4 @@ path = "../irc" [dependencies.serde] version = "1.0" -features = ["derive"] +features = ["derive", "rc"] diff --git a/data/src/client.rs b/data/src/client.rs index 1856021b8..53de8921b 100644 --- a/data/src/client.rs +++ b/data/src/client.rs @@ -159,6 +159,20 @@ impl fmt::Debug for Client { f.debug_struct("Client").finish() } } +const UNCONDITIONAL_CAPS: &[&str] = &[ + "account-notify", + "away-notify", + "batch", + "chghost", + "draft/read-marker", + "extended-monitor", + "invite-notify", + "labeled-response", + "message-tags", + "multi-prefix", + "server-time", + "userhost-in-names", +]; impl Client { pub fn new( @@ -692,92 +706,52 @@ impl Client { )]); } } - Command::CAP(_, sub, a, b) if sub == "LS" => { - let (caps, asterisk) = match (a, b) { - (Some(caps), None) => (caps, None), - (Some(asterisk), Some(caps)) => (caps, Some(asterisk)), - // Unreachable - (None, None) | (None, Some(_)) => return Ok(vec![]), - }; - + Command::CAP(_, sub, Some(_asterisk), Some(caps)) if sub == "LS" => { + self.listed_caps.extend(caps.split(' ').map(String::from)); + } + // Finished with LS + Command::CAP(_, sub, Some(caps), None) if sub == "LS" => { self.listed_caps.extend(caps.split(' ').map(String::from)); - // Finished - if asterisk.is_none() { - let mut requested = vec![]; - - let contains = |s| self.listed_caps.iter().any(|cap| cap == s); + let contains = |s : &str| self.listed_caps.iter().any(|cap| cap == s); - if contains("invite-notify") { - requested.push("invite-notify"); - } - if contains("userhost-in-names") { - requested.push("userhost-in-names"); - } - if contains("away-notify") { - requested.push("away-notify"); - } - if contains("message-tags") { - requested.push("message-tags"); - } - if contains("server-time") { - requested.push("server-time"); - } - if contains("chghost") { - requested.push("chghost"); - } - if contains("extended-monitor") { - requested.push("extended-monitor"); - } - if contains("account-notify") { - requested.push("account-notify"); - - if contains("extended-join") { - requested.push("extended-join"); - } - } - if contains("batch") { - requested.push("batch"); + let mut requested: Vec<&str> = UNCONDITIONAL_CAPS + .iter() + .copied() + .filter(|c| contains(c)) + .collect(); - // We require batch for our chathistory support - if contains("draft/chathistory") { - requested.push("draft/chathistory"); + if contains("account-notify") && contains("extended-join") { + requested.push("extended-join"); + } + if contains("batch") && contains("draft/chathistory") { + // We require batch for our chathistory support + requested.push("draft/chathistory"); - if contains("draft/event-playback") { - requested.push("draft/event-playback"); - } - } + if contains("draft/event-playback") { + requested.push("draft/event-playback"); } - if contains("labeled-response") { - requested.push("labeled-response"); + } + // We require labeled-response so we can properly tag echo-messages + if contains("labeled-response") && contains("echo-message") { + requested.push("echo-message"); + } + if self.listed_caps.iter().any(|cap| cap.starts_with("sasl")) { + requested.push("sasl"); + } - // We require labeled-response so we can properly tag echo-messages - if contains("echo-message") { - requested.push("echo-message"); - } - } - if self.listed_caps.iter().any(|cap| cap.starts_with("sasl")) { - requested.push("sasl"); - } - if contains("multi-prefix") { - requested.push("multi-prefix"); - } - if contains("draft/read-marker") { - requested.push("draft/read-marker"); - } - if !requested.is_empty() { - // Request - self.registration_step = RegistrationStep::Req; + if !requested.is_empty() { + // Request + self.registration_step = RegistrationStep::Req; - for message in group_capability_requests(&requested) { - self.handle.try_send(message)?; - } - } else { - // If none requested, end negotiation - self.registration_step = RegistrationStep::End; - self.handle.try_send(command!("CAP", "END"))?; + for message in group_capability_requests(&requested) { + self.handle.try_send(message)?; } + } else { + // If none requested, end negotiation + self.registration_step = RegistrationStep::End; + self.handle.try_send(command!("CAP", "END"))?; } } Command::CAP(_, sub, a, b) if sub == "ACK" => { @@ -837,71 +811,32 @@ impl Client { let new_caps = caps.split(' ').map(String::from).collect::>(); - let mut requested = vec![]; + let newly_contains = |s: &str| new_caps.iter().any(|cap| cap == s); - let newly_contains = |s| new_caps.iter().any(|cap| cap == s); - - let contains = |s| self.listed_caps.iter().any(|cap| cap == s); - - if newly_contains("invite-notify") { - requested.push("invite-notify"); - } - if newly_contains("userhost-in-names") { - requested.push("userhost-in-names"); - } - if newly_contains("away-notify") { - requested.push("away-notify"); - } - if newly_contains("message-tags") { - requested.push("message-tags"); - } - if newly_contains("server-time") { - requested.push("server-time"); - } - if newly_contains("chghost") { - requested.push("chghost"); - } - if newly_contains("extended-monitor") { - requested.push("extended-monitor"); - } - if contains("account-notify") || newly_contains("account-notify") { - if newly_contains("account-notify") { - requested.push("account-notify"); - } + let mut requested: Vec<&str> = UNCONDITIONAL_CAPS + .iter() + .copied() + .filter(|c| newly_contains(c)) + .collect(); - if newly_contains("extended-join") { - requested.push("extended-join"); - } - } - if contains("batch") || newly_contains("batch") { - if newly_contains("batch") { - requested.push("batch"); - } + let contains = |s: &str| self.listed_caps.iter().any(|cap| cap == s); - // We require batch for our chathistory support - if newly_contains("draft/chathistory") && self.config.chathistory { - requested.push("draft/chathistory"); + let either_contains = |s: &str| contains(s) || newly_contains(s); - if newly_contains("draft/event-playback") { - requested.push("draft/event-playback"); - } - } + if either_contains("account-notify") && newly_contains("extended-join") { + requested.push("extended-join"); } - if contains("labeled-response") || newly_contains("labeled-response") { - if newly_contains("labeled-response") { - requested.push("labeled-response"); - } + // We require batch for our chathistory support + if either_contains("batch") && newly_contains("draft/chathistory") && self.config.chathistory { + requested.push("draft/chathistory"); - // We require labeled-response so we can properly tag echo-messages - if newly_contains("echo-message") { - requested.push("echo-message"); + if newly_contains("draft/event-playback") { + requested.push("draft/event-playback"); } } - if newly_contains("multi-prefix") { - requested.push("multi-prefix"); - } - if newly_contains("draft/read-marker") { - requested.push("draft/read-marker"); + // We require labeled-response so we can properly tag echo-messages + if either_contains("labeled-response") && newly_contains("echo-message") { + requested.push("echo-message"); } if !requested.is_empty() { @@ -1003,7 +938,7 @@ impl Client { )]); } dcc::Command::Unsupported(command) => { - bail!("Unsupported DCC command: {command}",); + bail!("Unsupported DCC command: {command}"); } } } else { @@ -1168,11 +1103,18 @@ impl Client { // Updated actual nick let nick = ok!(args.first()); self.resolved_nick = Some(nick.to_string()); + } + // end of registration (including ISUPPORT) is indicated by either RPL_ENDOFMOTD or + // ERR_NOMOTD: https://modern.ircdocs.horse/#connection-registration + Command::Numeric(RPL_ENDOFMOTD, _args) | Command::Numeric(ERR_NOMOTD, _args) => { + let Some(nick) = self.resolved_nick.as_deref() else { + bail!("Error, registration completed without RPL_WELCOME completed"); + }; // Send nick password & ghost if let Some(nick_pass) = self.config.nick_password.as_ref() { // Try ghost recovery if we couldn't claim our nick - if self.config.should_ghost && nick != &self.config.nickname { + if self.config.should_ghost && nick != self.config.nickname { for sequence in &self.config.ghost_sequence { self.handle.try_send(command!( "PRIVMSG", diff --git a/data/src/server.rs b/data/src/server.rs index 5a44cd0f7..44d114e09 100644 --- a/data/src/server.rs +++ b/data/src/server.rs @@ -1,4 +1,5 @@ use std::collections::BTreeMap; +use std::sync::Arc; use std::{fmt, str}; use tokio::fs; use tokio::process::Command; @@ -14,23 +15,35 @@ use crate::config::Error; pub type Handle = Sender; #[derive(Debug, Clone, Hash, PartialEq, Eq, PartialOrd, Ord, Serialize, Deserialize)] -pub struct Server(String); +#[serde(transparent)] +pub struct Server { + name: Arc, +} + +impl Server { + pub fn name(&self) -> &str { + &self.name + } + +} impl From<&str> for Server { fn from(value: &str) -> Self { - Server(value.to_string()) + Server { + name: Arc::from(value), + } } } impl fmt::Display for Server { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - self.0.fmt(f) + self.name.fmt(f) } } impl AsRef for Server { fn as_ref(&self) -> &str { - &self.0 + self.name() } } diff --git a/irc/proto/src/command.rs b/irc/proto/src/command.rs index 899a8c91b..3d9127ca8 100644 --- a/irc/proto/src/command.rs +++ b/irc/proto/src/command.rs @@ -149,6 +149,9 @@ impl Command { params.next() }; } + macro_rules! remaining { + () => { params.collect() }; + } match tag.as_str() { "CAP" if len > 0 => { @@ -183,7 +186,7 @@ impl Command { "STATS" if len > 0 => STATS(req!(), opt!()), "HELP" => HELP(opt!()), "INFO" => INFO, - "MODE" if len > 0 => MODE(req!(), opt!(), Some(params.collect())), + "MODE" if len > 0 => MODE(req!(), opt!(), Some(remaining!())), "PRIVMSG" if len > 1 => PRIVMSG(req!(), req!()), "NOTICE" if len > 1 => NOTICE(req!(), req!()), "WHO" if len > 0 => WHO(req!(), opt!(), opt!()), @@ -201,11 +204,11 @@ impl Command { "SQUIT" if len > 1 => SQUIT(req!(), req!()), "AWAY" => AWAY(opt!()), "LINKS" => LINKS, - "USERHOST" => USERHOST(params.collect()), + "USERHOST" => USERHOST(remaining!()), "WALLOPS" if len > 0 => WALLOPS(req!()), "ACCOUNT" if len > 0 => ACCOUNT(req!()), - "BATCH" if len > 0 => BATCH(req!(), params.collect()), - "CHATHISTORY" if len > 0 => CHATHISTORY(req!(), params.collect()), + "BATCH" if len > 0 => BATCH(req!(), remaining!()), + "CHATHISTORY" if len > 0 => CHATHISTORY(req!(), remaining!()), "CHGHOST" if len > 1 => CHGHOST(req!(), req!()), "CNOTICE" if len > 2 => CNOTICE(req!(), req!(), req!()), "CPRIVMSG" if len > 2 => CPRIVMSG(req!(), req!(), req!()), @@ -214,7 +217,7 @@ impl Command { "MONITOR" if len > 0 => MONITOR(req!(), opt!()), "TAGMSG" if len > 0 => TAGMSG(req!()), "USERIP" if len > 0 => USERIP(req!()), - _ => Self::Unknown(tag, params.collect()), + _ => Self::Unknown(tag, remaining!()), } } diff --git a/src/main.rs b/src/main.rs index 67671be5b..9055f17e9 100644 --- a/src/main.rs +++ b/src/main.rs @@ -127,7 +127,6 @@ struct Halloy { theme: Theme, config: Config, clients: data::client::Map, - servers: server::Map, modal: Option, main_window: Window, pending_logs: Vec, @@ -190,7 +189,6 @@ impl Halloy { screen, theme: appearance::theme(&config.appearance.selected).into(), clients: Default::default(), - servers: config.servers.clone(), config, modal: None, main_window, @@ -200,6 +198,12 @@ impl Halloy { command, ) } + fn servers_mut(&mut self) -> &mut server::Map { + &mut self.config.servers + } + fn servers(&self) -> &server::Map { + &self.config.servers + } } pub enum Screen { @@ -327,13 +331,12 @@ impl Halloy { match config { Ok(updated) => { let removed_servers = self - .servers + .servers() .keys() .filter(|server| !updated.servers.contains(server)) .cloned() .collect::>(); - self.servers = updated.servers.clone(); self.theme = appearance::theme(&updated.appearance.selected).into(); self.config = updated; @@ -541,8 +544,9 @@ impl Halloy { let statusmsg = self.clients.get_statusmsg(&server); let casemapping = self.clients.get_casemapping(&server); + use data::client::Event; match event { - data::client::Event::Single(encoded, our_nick) => { + Event::Single(encoded, our_nick) => { if let Some(message) = data::Message::received( encoded, our_nick, @@ -560,7 +564,7 @@ impl Halloy { ); } } - data::client::Event::WithTarget(encoded, our_nick, target) => { + Event::WithTarget(encoded, our_nick, target) => { if let Some(message) = data::Message::received( encoded, our_nick, @@ -581,7 +585,7 @@ impl Halloy { ); } } - data::client::Event::Broadcast(broadcast) => match broadcast { + Event::Broadcast(broadcast) => match broadcast { data::client::Broadcast::Quit { user, comment, @@ -675,7 +679,7 @@ impl Halloy { ); } }, - data::client::Event::Notification( + Event::Notification( encoded, our_nick, notification, @@ -698,7 +702,7 @@ impl Halloy { if matches!( notification, - data::client::Notification::Highlight { .. } + Notification::Highlight { .. } ) { commands.extend( message.into_highlight(server.clone()).map( @@ -713,7 +717,7 @@ impl Halloy { } match ¬ification { - data::client::Notification::DirectMessage(user) => { + Notification::DirectMessage(user) => { if let Ok(query) = target::Query::parse( user.as_str(), chantypes, @@ -735,13 +739,13 @@ impl Halloy { } } } - data::client::Notification::Highlight { + Notification::Highlight { enabled: _, user: _, target: _, } - | data::client::Notification::MonitoredOnline(_) - | data::client::Notification::MonitoredOffline(_) => { + | Notification::MonitoredOnline(_) + | Notification::MonitoredOffline(_) => { self.notifications.notify( &self.config.notifications, ¬ification, @@ -751,7 +755,7 @@ impl Halloy { _ => {} } } - data::client::Event::FileTransferRequest(request) => { + Event::FileTransferRequest(request) => { if let Some(command) = dashboard.receive_file_transfer( &server, chantypes, @@ -763,7 +767,7 @@ impl Halloy { commands.push(command.map(Message::Dashboard)); } } - data::client::Event::UpdateReadMarker(target, read_marker) => { + Event::UpdateReadMarker(target, read_marker) => { commands.push( dashboard .update_read_marker( @@ -776,7 +780,7 @@ impl Halloy { .map(Message::Dashboard), ); } - data::client::Event::JoinedChannel(channel, server_time) => { + Event::JoinedChannel(channel, server_time) => { let command = dashboard .load_metadata( &self.clients, @@ -788,7 +792,7 @@ impl Halloy { commands.push(command); } - data::client::Event::ChatHistoryAcknowledged(server_time) => { + Event::ChatHistoryAcknowledged(server_time) => { if let Some(command) = dashboard .load_chathistory_targets_timestamp( &self.clients, @@ -800,7 +804,7 @@ impl Halloy { commands.push(command); } } - data::client::Event::ChatHistoryTargetReceived( + Event::ChatHistoryTargetReceived( target, server_time, ) => { @@ -815,7 +819,7 @@ impl Halloy { commands.push(command); } - data::client::Event::ChatHistoryTargetsReceived( + Event::ChatHistoryTargetsReceived( server_time, ) => { if let Some(command) = dashboard @@ -844,8 +848,6 @@ impl Halloy { } stream::Update::Quit(server, reason) => match &mut self.screen { Screen::Dashboard(dashboard) => { - self.servers.remove(&server); - if let Some(client) = self.clients.remove(&server) { let user = client.nickname().to_owned().into(); @@ -920,7 +922,7 @@ impl Halloy { if let Some(Modal::ServerConnect { server, config, .. }) = self.modal.take() { - let existing_entry = self.servers.entries().find(|entry| { + let existing_entry = self.servers().entries().find(|entry| { entry.server == server || entry.config.server == config.server }); @@ -947,7 +949,7 @@ impl Halloy { .collect::>(), ); } else { - self.servers.insert(server, config); + self.servers_mut().insert(server, config); } } } @@ -1111,7 +1113,7 @@ impl Halloy { let tick = iced::time::every(Duration::from_secs(1)).map(Message::Tick); let streams = Subscription::batch( - self.servers + self.servers() .entries() .map(|entry| stream::run(entry, self.config.proxy.clone())), )