Skip to content

Commit 4e066dd

Browse files
4e554c4c4e554c4c
committedJan 23, 2025
Fix various performance issues
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<str>` 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<config>`?) 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 squidowl#640
1 parent 4bf0c5d commit 4e066dd

File tree

5 files changed

+120
-144
lines changed

5 files changed

+120
-144
lines changed
 

‎data/Cargo.toml

+4-4
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ flate2 = "1.0"
1717
futures = "0.3.21"
1818
hex = "0.4.3"
1919
iced_core = "0.14.0-dev"
20-
log = "0.4.16"
20+
log = { version = "0.4.16", features = ['std'] }
2121
palette = "0.7.4"
2222
rand = "0.8.4"
2323
rand_chacha = "0.3.0"
@@ -27,8 +27,8 @@ sha2 = "0.10.8"
2727
toml = "0.8.11"
2828
thiserror = "1.0.30"
2929
reqwest = { version = "0.12", features = ["json"] }
30-
tokio = { version = "1.0", features = ["io-util"] }
31-
tokio-stream = { version = "0.1", features = ["time"] }
30+
tokio = { version = "1.0", features = ["io-util", "fs"] }
31+
tokio-stream = { version = "0.1", features = ["time", "fs"] }
3232
itertools = "0.12.1"
3333
timeago = "0.4.2"
3434
url = { version = "2.5.0", features = ["serde"] }
@@ -46,4 +46,4 @@ path = "../irc"
4646

4747
[dependencies.serde]
4848
version = "1.0"
49-
features = ["derive"]
49+
features = ["derive", "rc"]

‎data/src/client.rs

+65-107
Original file line numberDiff line numberDiff line change
@@ -155,6 +155,20 @@ impl fmt::Debug for Client {
155155
f.debug_struct("Client").finish()
156156
}
157157
}
158+
const UNCONDITIONAL_CAPS: &[&str] = &[
159+
"account-notify",
160+
"away-notify",
161+
"batch",
162+
"chghost",
163+
"draft/read-marker",
164+
"extended-monitor",
165+
"invite-notify",
166+
"labeled-response",
167+
"message-tags",
168+
"multi-prefix",
169+
"server-time",
170+
"userhost-in-names",
171+
];
158172

159173
impl Client {
160174
pub fn new(
@@ -646,92 +660,52 @@ impl Client {
646660
)]);
647661
}
648662
}
649-
Command::CAP(_, sub, a, b) if sub == "LS" => {
650-
let (caps, asterisk) = match (a, b) {
651-
(Some(caps), None) => (caps, None),
652-
(Some(asterisk), Some(caps)) => (caps, Some(asterisk)),
653-
// Unreachable
654-
(None, None) | (None, Some(_)) => return Ok(vec![]),
655-
};
656-
663+
Command::CAP(_, sub, Some(_asterisk), Some(caps)) if sub == "LS" => {
664+
self.listed_caps.extend(caps.split(' ').map(String::from));
665+
}
666+
// Finished with LS
667+
Command::CAP(_, sub, Some(caps), None) if sub == "LS" => {
657668
self.listed_caps.extend(caps.split(' ').map(String::from));
658669

659-
// Finished
660-
if asterisk.is_none() {
661-
let mut requested = vec![];
670+
let contains = |s : &str| self.listed_caps.iter().any(|cap| cap == s);
662671

663-
let contains = |s| self.listed_caps.iter().any(|cap| cap == s);
672+
let mut requested: Vec<&str> = UNCONDITIONAL_CAPS
673+
.iter()
674+
.copied()
675+
.filter(|c| contains(c))
676+
.collect();
664677

665-
if contains("invite-notify") {
666-
requested.push("invite-notify");
667-
}
668-
if contains("userhost-in-names") {
669-
requested.push("userhost-in-names");
670-
}
671-
if contains("away-notify") {
672-
requested.push("away-notify");
673-
}
674-
if contains("message-tags") {
675-
requested.push("message-tags");
676-
}
677-
if contains("server-time") {
678-
requested.push("server-time");
679-
}
680-
if contains("chghost") {
681-
requested.push("chghost");
682-
}
683-
if contains("extended-monitor") {
684-
requested.push("extended-monitor");
685-
}
686-
if contains("account-notify") {
687-
requested.push("account-notify");
678+
if contains("account-notify") && contains("extended-join") {
679+
requested.push("extended-join");
680+
}
681+
if contains("batch") && contains("draft/chathistory") {
682+
// We require batch for our chathistory support
683+
requested.push("draft/chathistory");
688684

689-
if contains("extended-join") {
690-
requested.push("extended-join");
691-
}
685+
if contains("draft/event-playback") {
686+
requested.push("draft/event-playback");
692687
}
693-
if contains("batch") {
694-
requested.push("batch");
688+
}
689+
// We require labeled-response so we can properly tag echo-messages
690+
if contains("labeled-response") && contains("echo-message") {
691+
requested.push("echo-message");
692+
}
693+
if self.listed_caps.iter().any(|cap| cap.starts_with("sasl")) {
694+
requested.push("sasl");
695+
}
695696

696-
// We require batch for our chathistory support
697-
if contains("draft/chathistory") {
698-
requested.push("draft/chathistory");
699697

700-
if contains("draft/event-playback") {
701-
requested.push("draft/event-playback");
702-
}
703-
}
704-
}
705-
if contains("labeled-response") {
706-
requested.push("labeled-response");
707-
708-
// We require labeled-response so we can properly tag echo-messages
709-
if contains("echo-message") {
710-
requested.push("echo-message");
711-
}
712-
}
713-
if self.listed_caps.iter().any(|cap| cap.starts_with("sasl")) {
714-
requested.push("sasl");
715-
}
716-
if contains("multi-prefix") {
717-
requested.push("multi-prefix");
718-
}
719-
if contains("draft/read-marker") {
720-
requested.push("draft/read-marker");
721-
}
722-
723-
if !requested.is_empty() {
724-
// Request
725-
self.registration_step = RegistrationStep::Req;
698+
if !requested.is_empty() {
699+
// Request
700+
self.registration_step = RegistrationStep::Req;
726701

727-
for message in group_capability_requests(&requested) {
728-
self.handle.try_send(message)?;
729-
}
730-
} else {
731-
// If none requested, end negotiation
732-
self.registration_step = RegistrationStep::End;
733-
self.handle.try_send(command!("CAP", "END"))?;
702+
for message in group_capability_requests(&requested) {
703+
self.handle.try_send(message)?;
734704
}
705+
} else {
706+
// If none requested, end negotiation
707+
self.registration_step = RegistrationStep::End;
708+
self.handle.try_send(command!("CAP", "END"))?;
735709
}
736710
}
737711
Command::CAP(_, sub, a, b) if sub == "ACK" => {
@@ -791,33 +765,16 @@ impl Client {
791765

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

794-
let mut requested = vec![];
795-
796768
let newly_contains = |s| new_caps.iter().any(|cap| cap == s);
797769

770+
let mut requested: Vec<&str> = UNCONDITIONAL_CAPS
771+
.iter()
772+
.copied()
773+
.filter(|c| newly_contains(*c))
774+
.collect();
775+
798776
let contains = |s| self.listed_caps.iter().any(|cap| cap == s);
799777

800-
if newly_contains("invite-notify") {
801-
requested.push("invite-notify");
802-
}
803-
if newly_contains("userhost-in-names") {
804-
requested.push("userhost-in-names");
805-
}
806-
if newly_contains("away-notify") {
807-
requested.push("away-notify");
808-
}
809-
if newly_contains("message-tags") {
810-
requested.push("message-tags");
811-
}
812-
if newly_contains("server-time") {
813-
requested.push("server-time");
814-
}
815-
if newly_contains("chghost") {
816-
requested.push("chghost");
817-
}
818-
if newly_contains("extended-monitor") {
819-
requested.push("extended-monitor");
820-
}
821778
if contains("account-notify") || newly_contains("account-notify") {
822779
if newly_contains("account-notify") {
823780
requested.push("account-notify");
@@ -851,12 +808,6 @@ impl Client {
851808
requested.push("echo-message");
852809
}
853810
}
854-
if newly_contains("multi-prefix") {
855-
requested.push("multi-prefix");
856-
}
857-
if newly_contains("draft/read-marker") {
858-
requested.push("draft/read-marker");
859-
}
860811

861812
if !requested.is_empty() {
862813
for message in group_capability_requests(&requested) {
@@ -957,7 +908,7 @@ impl Client {
957908
)]);
958909
}
959910
dcc::Command::Unsupported(command) => {
960-
bail!("Unsupported DCC command: {command}",);
911+
bail!("Unsupported DCC command: {command}");
961912
}
962913
}
963914
} else {
@@ -1122,11 +1073,18 @@ impl Client {
11221073
// Updated actual nick
11231074
let nick = ok!(args.first());
11241075
self.resolved_nick = Some(nick.to_string());
1076+
}
1077+
// end of registration (including ISUPPORT) is indicated by either RPL_ENDOFMOTD or
1078+
// ERR_NOMOTD: https://modern.ircdocs.horse/#connection-registration
1079+
Command::Numeric(RPL_ENDOFMOTD, _args) | Command::Numeric(ERR_NOMOTD, _args) => {
1080+
let Some(nick) = self.resolved_nick.as_deref() else {
1081+
bail!("Error, registration completed without RPL_WELCOME completed");
1082+
};
11251083

11261084
// Send nick password & ghost
11271085
if let Some(nick_pass) = self.config.nick_password.as_ref() {
11281086
// Try ghost recovery if we couldn't claim our nick
1129-
if self.config.should_ghost && nick != &self.config.nickname {
1087+
if self.config.should_ghost && nick != self.config.nickname {
11301088
for sequence in &self.config.ghost_sequence {
11311089
self.handle.try_send(command!(
11321090
"PRIVMSG",

‎data/src/server.rs

+17-4
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
use std::collections::BTreeMap;
2+
use std::sync::Arc;
23
use std::{fmt, str};
34
use tokio::fs;
45
use tokio::process::Command;
@@ -14,23 +15,35 @@ use crate::config::Error;
1415
pub type Handle = Sender<proto::Message>;
1516

1617
#[derive(Debug, Clone, Hash, PartialEq, Eq, PartialOrd, Ord, Serialize, Deserialize)]
17-
pub struct Server(String);
18+
#[serde(transparent)]
19+
pub struct Server {
20+
name: Arc<str>,
21+
}
22+
23+
impl Server {
24+
pub fn name(&self) -> &str {
25+
&self.name
26+
}
27+
28+
}
1829

1930
impl From<&str> for Server {
2031
fn from(value: &str) -> Self {
21-
Server(value.to_string())
32+
Server {
33+
name: Arc::from(value),
34+
}
2235
}
2336
}
2437

2538
impl fmt::Display for Server {
2639
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
27-
self.0.fmt(f)
40+
self.name.fmt(f)
2841
}
2942
}
3043

3144
impl AsRef<str> for Server {
3245
fn as_ref(&self) -> &str {
33-
&self.0
46+
self.name()
3447
}
3548
}
3649

‎irc/proto/src/command.rs

+8-5
Original file line numberDiff line numberDiff line change
@@ -149,6 +149,9 @@ impl Command {
149149
params.next()
150150
};
151151
}
152+
macro_rules! remaining {
153+
() => { params.collect() };
154+
}
152155

153156
match tag.as_str() {
154157
"CAP" if len > 0 => {
@@ -183,7 +186,7 @@ impl Command {
183186
"STATS" if len > 0 => STATS(req!(), opt!()),
184187
"HELP" => HELP(opt!()),
185188
"INFO" => INFO,
186-
"MODE" if len > 0 => MODE(req!(), opt!(), Some(params.collect())),
189+
"MODE" if len > 0 => MODE(req!(), opt!(), Some(remaining!())),
187190
"PRIVMSG" if len > 1 => PRIVMSG(req!(), req!()),
188191
"NOTICE" if len > 1 => NOTICE(req!(), req!()),
189192
"WHO" if len > 0 => WHO(req!(), opt!(), opt!()),
@@ -201,11 +204,11 @@ impl Command {
201204
"SQUIT" if len > 1 => SQUIT(req!(), req!()),
202205
"AWAY" => AWAY(opt!()),
203206
"LINKS" => LINKS,
204-
"USERHOST" => USERHOST(params.collect()),
207+
"USERHOST" => USERHOST(remaining!()),
205208
"WALLOPS" if len > 0 => WALLOPS(req!()),
206209
"ACCOUNT" if len > 0 => ACCOUNT(req!()),
207-
"BATCH" if len > 0 => BATCH(req!(), params.collect()),
208-
"CHATHISTORY" if len > 0 => CHATHISTORY(req!(), params.collect()),
210+
"BATCH" if len > 0 => BATCH(req!(), remaining!()),
211+
"CHATHISTORY" if len > 0 => CHATHISTORY(req!(), remaining!()),
209212
"CHGHOST" if len > 1 => CHGHOST(req!(), req!()),
210213
"CNOTICE" if len > 2 => CNOTICE(req!(), req!(), req!()),
211214
"CPRIVMSG" if len > 2 => CPRIVMSG(req!(), req!(), req!()),
@@ -214,7 +217,7 @@ impl Command {
214217
"MONITOR" if len > 0 => MONITOR(req!(), opt!()),
215218
"TAGMSG" if len > 0 => TAGMSG(req!()),
216219
"USERIP" if len > 0 => USERIP(req!()),
217-
_ => Self::Unknown(tag, params.collect()),
220+
_ => Self::Unknown(tag, remaining!()),
218221
}
219222
}
220223

‎src/main.rs

+26-24
Original file line numberDiff line numberDiff line change
@@ -127,7 +127,6 @@ struct Halloy {
127127
theme: Theme,
128128
config: Config,
129129
clients: data::client::Map,
130-
servers: server::Map,
131130
modal: Option<Modal>,
132131
main_window: Window,
133132
pending_logs: Vec<data::log::Record>,
@@ -190,7 +189,6 @@ impl Halloy {
190189
screen,
191190
theme: appearance::theme(&config.appearance.selected).into(),
192191
clients: Default::default(),
193-
servers: config.servers.clone(),
194192
config,
195193
modal: None,
196194
main_window,
@@ -200,6 +198,12 @@ impl Halloy {
200198
command,
201199
)
202200
}
201+
fn servers_mut(&mut self) -> &mut server::Map {
202+
&mut self.config.servers
203+
}
204+
fn servers(&self) -> &server::Map {
205+
&self.config.servers
206+
}
203207
}
204208

205209
pub enum Screen {
@@ -327,13 +331,12 @@ impl Halloy {
327331
match config {
328332
Ok(updated) => {
329333
let removed_servers = self
330-
.servers
334+
.servers()
331335
.keys()
332336
.filter(|server| !updated.servers.contains(server))
333337
.cloned()
334338
.collect::<Vec<_>>();
335339

336-
self.servers = updated.servers.clone();
337340
self.theme = appearance::theme(&updated.appearance.selected).into();
338341
self.config = updated;
339342

@@ -541,8 +544,9 @@ impl Halloy {
541544
let statusmsg = self.clients.get_statusmsg(&server);
542545
let casemapping = self.clients.get_casemapping(&server);
543546

547+
use data::client::Event;
544548
match event {
545-
data::client::Event::Single(encoded, our_nick) => {
549+
Event::Single(encoded, our_nick) => {
546550
if let Some(message) = data::Message::received(
547551
encoded,
548552
our_nick,
@@ -560,7 +564,7 @@ impl Halloy {
560564
);
561565
}
562566
}
563-
data::client::Event::WithTarget(encoded, our_nick, target) => {
567+
Event::WithTarget(encoded, our_nick, target) => {
564568
if let Some(message) = data::Message::received(
565569
encoded,
566570
our_nick,
@@ -581,7 +585,7 @@ impl Halloy {
581585
);
582586
}
583587
}
584-
data::client::Event::Broadcast(broadcast) => match broadcast {
588+
Event::Broadcast(broadcast) => match broadcast {
585589
data::client::Broadcast::Quit {
586590
user,
587591
comment,
@@ -675,7 +679,7 @@ impl Halloy {
675679
);
676680
}
677681
},
678-
data::client::Event::Notification(
682+
Event::Notification(
679683
encoded,
680684
our_nick,
681685
notification,
@@ -698,7 +702,7 @@ impl Halloy {
698702

699703
if matches!(
700704
notification,
701-
data::client::Notification::Highlight { .. }
705+
Notification::Highlight { .. }
702706
) {
703707
commands.extend(
704708
message.into_highlight(server.clone()).map(
@@ -713,7 +717,7 @@ impl Halloy {
713717
}
714718

715719
match &notification {
716-
data::client::Notification::DirectMessage(user) => {
720+
Notification::DirectMessage(user) => {
717721
if let Ok(query) = target::Query::parse(
718722
user.as_str(),
719723
chantypes,
@@ -735,13 +739,13 @@ impl Halloy {
735739
}
736740
}
737741
}
738-
data::client::Notification::Highlight {
742+
Notification::Highlight {
739743
enabled: _,
740744
user: _,
741745
target: _,
742746
}
743-
| data::client::Notification::MonitoredOnline(_)
744-
| data::client::Notification::MonitoredOffline(_) => {
747+
| Notification::MonitoredOnline(_)
748+
| Notification::MonitoredOffline(_) => {
745749
self.notifications.notify(
746750
&self.config.notifications,
747751
&notification,
@@ -751,7 +755,7 @@ impl Halloy {
751755
_ => {}
752756
}
753757
}
754-
data::client::Event::FileTransferRequest(request) => {
758+
Event::FileTransferRequest(request) => {
755759
if let Some(command) = dashboard.receive_file_transfer(
756760
&server,
757761
chantypes,
@@ -763,7 +767,7 @@ impl Halloy {
763767
commands.push(command.map(Message::Dashboard));
764768
}
765769
}
766-
data::client::Event::UpdateReadMarker(target, read_marker) => {
770+
Event::UpdateReadMarker(target, read_marker) => {
767771
commands.push(
768772
dashboard
769773
.update_read_marker(
@@ -776,7 +780,7 @@ impl Halloy {
776780
.map(Message::Dashboard),
777781
);
778782
}
779-
data::client::Event::JoinedChannel(channel, server_time) => {
783+
Event::JoinedChannel(channel, server_time) => {
780784
let command = dashboard
781785
.load_metadata(
782786
&self.clients,
@@ -788,7 +792,7 @@ impl Halloy {
788792

789793
commands.push(command);
790794
}
791-
data::client::Event::ChatHistoryAcknowledged(server_time) => {
795+
Event::ChatHistoryAcknowledged(server_time) => {
792796
if let Some(command) = dashboard
793797
.load_chathistory_targets_timestamp(
794798
&self.clients,
@@ -800,7 +804,7 @@ impl Halloy {
800804
commands.push(command);
801805
}
802806
}
803-
data::client::Event::ChatHistoryTargetReceived(
807+
Event::ChatHistoryTargetReceived(
804808
target,
805809
server_time,
806810
) => {
@@ -815,7 +819,7 @@ impl Halloy {
815819

816820
commands.push(command);
817821
}
818-
data::client::Event::ChatHistoryTargetsReceived(
822+
Event::ChatHistoryTargetsReceived(
819823
server_time,
820824
) => {
821825
if let Some(command) = dashboard
@@ -844,8 +848,6 @@ impl Halloy {
844848
}
845849
stream::Update::Quit(server, reason) => match &mut self.screen {
846850
Screen::Dashboard(dashboard) => {
847-
self.servers.remove(&server);
848-
849851
if let Some(client) = self.clients.remove(&server) {
850852
let user = client.nickname().to_owned().into();
851853

@@ -920,7 +922,7 @@ impl Halloy {
920922
if let Some(Modal::ServerConnect { server, config, .. }) =
921923
self.modal.take()
922924
{
923-
let existing_entry = self.servers.entries().find(|entry| {
925+
let existing_entry = self.servers().entries().find(|entry| {
924926
entry.server == server || entry.config.server == config.server
925927
});
926928

@@ -947,7 +949,7 @@ impl Halloy {
947949
.collect::<Vec<_>>(),
948950
);
949951
} else {
950-
self.servers.insert(server, config);
952+
self.servers_mut().insert(server, config);
951953
}
952954
}
953955
}
@@ -1111,7 +1113,7 @@ impl Halloy {
11111113
let tick = iced::time::every(Duration::from_secs(1)).map(Message::Tick);
11121114

11131115
let streams = Subscription::batch(
1114-
self.servers
1116+
self.servers()
11151117
.entries()
11161118
.map(|entry| stream::run(entry, self.config.proxy.clone())),
11171119
)

0 commit comments

Comments
 (0)
Please sign in to comment.