refactor(iroh-net): Store DerpNodes as Arcs inside DerpMap #1379
Conversation
The DerpMap is now immutable, storing the DerpNodes inside it as Arc'ed structs directly saves a lot of copying and moving them into Arcs at runtime for a bit more heap allocations up front.
| @@ -137,7 +147,7 @@ pub struct DerpRegion { | |||
| /// A unique integer for a geographic region | |||
| pub region_id: u16, | |||
| /// A list of [`DerpNode`]s in this region | |||
| pub nodes: Vec<DerpNode>, | |||
| pub nodes: Vec<Arc<DerpNode>>, | |||
There was a problem hiding this comment.
Any reason to have DerpNode be a standalone type in this case? Can we just move the Arc into DerpNode instead?
There was a problem hiding this comment.
You mean wrapping the DerpNode field inside an "inner"? Currently all the DerpNode fields (and DerpPregion and DerpMap) and pub and accessed directly everywhere. Making this change here means you only have to touch the creation of this but not the access. Doing it the other way around involves touching all the access places, probably changing the fields to methods.
I'm not strongly in favour of either, though I also don't think this version is so bad. The explicit Arc is pretty reasonable. Though if you have a strong preference I can change it.
There was a problem hiding this comment.
good enough for now
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Description
The DerpMap is now immutable, storing the DerpNodes inside it as
Arc'ed structs directly saves a lot of copying and moving them into
Arcs at runtime for a bit more heap allocations up front.
Notes & open questions
Change checklist
Documentation updates if relevant.Tests if relevant.