rfc: refactoring Server
#640
Comments
|
Sounds reasonable! I don't even think we need a UUID. Since the server name from the toml config has to be unique, we can simply hash it and use that as the u64 id. |
Yes, but the issue is that since bouncer-networks (#77) would mean multiple servers / connections for a single TOML "server name", we need a better way to identify servers for example, two servers from two different bouncers could be called "libera" |
|
Makes sense 👍 |
|
Ok i'll try to work on this separately from the bouncer-networks stuff (since that's already getting kind of complicated) and we'll see how it looks 🎉 |
|
I think I'm going to try to make it a v5 UUID with a consistent hash function, so that we can use these in the history in lieu of server names |
|
I'm thinking that UUIDs might actually be a bad idea. But a better idea might be putting |
|
I tried making Before: After: So a 4% cpu usage reduction which is pretty nice |
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
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
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
The Problem
When working on #77 I have run into numerous issues with how to represent a "server" in halloy. The
Serverstruct, which is a wrapper aroundString, is currently ubiquitous in the halloy codebase, however it has many issues. SinceServeris aString,&Serveris&String, which is an anti-pattern. Variables of type&Servermust be.clone()d often to satisfy lifetime issues and in order to satisfy lifetime constraints, which leads to the same "server name" being duplicated all over the codebase. For example, it must be cloned every time a message is received by the IRC server to communicate the server updated between the "stream" and "client".In order to implement #77, the way that servers are referenced must be changed as well. In a bouncer-networks setup, several "primary" servers each set up any number of secondary servers. Thus there is a hierarchical relationship between servers, and a single config is shared between many. Furthermore, servers can no longer be identified by their name. Names may not be unique (two secondaries on under two different primaries may match), and are not the canonical way to refer to a server (the canonical identifier for a "secondary" server is its bouncer NETID).
Therefore, however we represent
Server, it cannot also be used as the human-visible name for a server, which causes issues elsewhere in the codebaseA Proposed Solution
Ideally, in order to send a
Serveridentifier with every single irc message, theServertype should beCopy(not justClone!) and cheap to replicate. Furthermore, we should be able to get additional data that aServerrepresents by asking a centralized source of information.My proposal is as follows:
Servershould be aUuid(either v4 or v5). This data can be cheaply copied and sent between tasks without a performance overhead. Then, when it is necessary to look up server information (such as user-visible name) this can be gathered by looking at the aHashMap<Uuid, ServerInfo>which contains server config name, bouncer ID, bouncer name, etc.CCs
@tarkah , @casperstorm let me know what you think. Last time I brought this up you mentioned that I should open an RFC before doing any huge refactors with
Serverand friends 😸The text was updated successfully, but these errors were encountered: