Fix various performance issues #724

4e554c4c · 2025-01-23T18:43:06Z

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 #640

4e554c4c · 2025-01-23T18:43:50Z

data/src/server.rs

-pub struct Server(String);
+#[serde(transparent)]
+pub struct Server {
+    name: Arc<str>,


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

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

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

data/src/client.rs

casperstorm

Looks good to me, thanks for a great PR @4e554c4c.
I'd like to see if @tarkah has any comments before we merge it.

data/src/client.rs

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

4e554c4c · 2025-01-27T16:43:46Z

updated

I experimented with some other things (in particular using HashSet operations to update caps) that didn't end up so well due to struct field borrowing issues...
Eventually I think it'd be nice to move this into a Cap struct that handles most of the logic, since that would simplify a lot (especially because the logic for CAP NEW and CAP LS should basically be equivalent).

andymandias

Looks good to me, and I didn't find any issues in testing.

andymandias · 2025-01-29T02:54:04Z

data/src/client.rs

@@ -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


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).

tarkah · 2025-01-29T20:08:55Z

src/main.rs

                                let removed_servers = self
-                                    .servers
+                                    .servers()
                                    .keys()
                                    .filter(|server| !updated.servers.contains(server))
                                    .cloned()
                                    .collect::<Vec<_>>();

-                                self.servers = updated.servers.clone();


I'm personally not a fan of this. Config should be immutable and exactly what we loaded from the disk. We add / remove from this map when servers are disconnected or added via irc:// url and is our source of truth for what is currently connected, not what they've configured

Instead, we should change config.server to be a normal HashMap so it can't be conflated with Halloy.servers / server::Map and misused. We can convert to server::Map initializing Halloy from config or updating here.

i would be ok with this if there were only one Config, but server configs are copied everywhere which is a huge pain for keeping things in sync.

Not to mention that the current logic deletes all servers which are not in the config whenever the config updates, including dynamically added servers, so these two structs are essentially treated as equal except in some edge cases

i would be ok with this if there were only one Config, but server configs are copied everywhere which is a huge pain for keeping things in sync.

The only thing accessing Config.servers is us cloning it to Halloy.servers. And the only thing Halloy.servers is used by is logic for diffing when new servers are added (via irc://) or removed (via config refresh) and then it's our source of truth for which server subscriptions we maintain (active IRC connections).

But by using Config.servers as our source of truth, we have to mutate the users configuration and we also lose control of how to manage runtime servers outside the context of "configuration"

tarkah · 2025-01-29T20:12:17Z

data/src/server.rs

-pub struct Server(String);
+#[serde(transparent)]
+pub struct Server {
+    name: Arc<str>,


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

4e554c4c · 2025-02-04T13:04:21Z

closing this until v2

4e554c4c commented Jan 23, 2025

View reviewed changes

andymandias reviewed Jan 23, 2025

View reviewed changes

data/src/client.rs Show resolved Hide resolved

andymandias reviewed Jan 23, 2025

View reviewed changes

data/src/client.rs Outdated Show resolved Hide resolved

4e554c4c force-pushed the perf branch from 2c5e94b to 4e066dd Compare January 23, 2025 21:02

casperstorm approved these changes Jan 24, 2025

View reviewed changes

4e554c4c mentioned this pull request Jan 24, 2025

Bouncer networks #730

Draft

andymandias reviewed Jan 24, 2025

View reviewed changes

data/src/client.rs Outdated Show resolved Hide resolved

4e554c4c force-pushed the perf branch from 4e066dd to 1388956 Compare January 27, 2025 16:42

4e554c4c requested a review from andymandias January 28, 2025 16:36

andymandias approved these changes Jan 29, 2025

View reviewed changes

tarkah requested changes Jan 29, 2025

View reviewed changes

4e554c4c closed this Feb 4, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix various performance issues #724

Fix various performance issues #724

4e554c4c commented Jan 23, 2025

4e554c4c Jan 23, 2025

tarkah Jan 29, 2025

4e554c4c Jan 30, 2025

casperstorm left a comment

4e554c4c commented Jan 27, 2025

andymandias left a comment

andymandias Jan 29, 2025

tarkah Jan 29, 2025

4e554c4c Jan 30, 2025 •

edited

Loading

tarkah Jan 31, 2025 •

edited

Loading

tarkah Jan 29, 2025

4e554c4c commented Feb 4, 2025

Fix various performance issues #724

Fix various performance issues #724

Conversation

4e554c4c commented Jan 23, 2025

4e554c4c Jan 23, 2025

Choose a reason for hiding this comment

tarkah Jan 29, 2025

Choose a reason for hiding this comment

4e554c4c Jan 30, 2025

Choose a reason for hiding this comment

casperstorm left a comment

Choose a reason for hiding this comment

4e554c4c commented Jan 27, 2025

andymandias left a comment

Choose a reason for hiding this comment

andymandias Jan 29, 2025

Choose a reason for hiding this comment

tarkah Jan 29, 2025

Choose a reason for hiding this comment

4e554c4c Jan 30, 2025 • edited Loading

Choose a reason for hiding this comment

tarkah Jan 31, 2025 • edited Loading

Choose a reason for hiding this comment

tarkah Jan 29, 2025

Choose a reason for hiding this comment

4e554c4c commented Feb 4, 2025

4e554c4c Jan 30, 2025 •

edited

Loading

tarkah Jan 31, 2025 •

edited

Loading