Skip to content

Fix various performance issues #724

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Fix various performance issues #724

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions data/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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"] }
Expand All @@ -46,4 +46,4 @@ path = "../irc"

[dependencies.serde]
version = "1.0"
features = ["derive"]
features = ["derive", "rc"]
212 changes: 77 additions & 135 deletions data/src/client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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" => {
Expand Down Expand Up @@ -837,71 +811,32 @@ impl Client {

let new_caps = caps.split(' ').map(String::from).collect::<Vec<String>>();

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() {
Expand Down Expand Up @@ -1003,7 +938,7 @@ impl Client {
)]);
}
dcc::Command::Unsupported(command) => {
bail!("Unsupported DCC command: {command}",);
bail!("Unsupported DCC command: {command}");
}
}
} else {
Expand Down Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the link on the next line it reads as though RPL_WELCOME through RPL_ENDOFMOTD or ERR_NOMOTD is technically all post-registration. It doesn't really matter what that series of messages is called though, this update is correct; we were utilizing RPL_ISUPPORT parameters in channel parsing for sending JOINs, but feature advertisements come after RPL_WELCOME (and before RPL_ENDOFMOTD or ERR_NOMOTD).

// 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",
Expand Down
21 changes: 17 additions & 4 deletions data/src/server.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use std::collections::BTreeMap;
use std::sync::Arc;
use std::{fmt, str};
use tokio::fs;
use tokio::process::Command;
Expand All @@ -14,23 +15,35 @@ use crate::config::Error;
pub type Handle = Sender<proto::Message>;

#[derive(Debug, Clone, Hash, PartialEq, Eq, PartialOrd, Ord, Serialize, Deserialize)]
pub struct Server(String);
#[serde(transparent)]
pub struct Server {
name: Arc<str>,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to change this to a named struct instead of a tuple struct, since future PRs will add fields here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't implement Serialize / Deserialize for this if we expect it to become stateful / hold additional data as this will break out serialization format in a few places.

I'd prefer we keep it a newtype for now (with the arc is fine), and if in the future if we want to add fields, we can rename this to server::Name and have some new Server struct that takes over that role

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately servers and server names are traeted as the same thing basically all throughout the codebase. The config is treated as the de-facto location where servers are stored (with everything else being cloned from there) so we kind of have to start changing things here unless we change the entire codebase

Doing things this way makes the bouncer networks change a fairly modest ~100 patch before bouncer-networks specific logic which i thought would be better than totally refactoring everything

}

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<str> for Server {
fn as_ref(&self) -> &str {
&self.0
self.name()
}
}

Expand Down
13 changes: 8 additions & 5 deletions irc/proto/src/command.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,9 @@ impl Command {
params.next()
};
}
macro_rules! remaining {
() => { params.collect() };
}

match tag.as_str() {
"CAP" if len > 0 => {
Expand Down Expand Up @@ -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!()),
Expand All @@ -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!()),
Expand All @@ -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!()),
}
}

Expand Down
Loading
Loading