I noticed that inside sn_node we pass around the struct NodeInfo a lot, and make many copies redundantly, deep into struct hierarchies. Here’s just a handful of places I found it.
That’s not all of them, but I started following the thread and it goes really, really deep. My question is, what is the reasoning behind the high redundancy of the copying here? Surely, a single <'a> reference would reduce the memory footprint, no?
I get that we’re passing around snapshots, but why not just pass around references? Or even an Arc<RwLock> if the copying is for simple thread safety? I’m wondering because I want to change a field in this struct… at runtime… After initially attempting, I quickly realized that tracking down all of these copies is damn-near futile.
Is there a way to change these all at once? If not, is there anything stopping me from turning them all into references to a “master” NodeInfo? They seem to all come from the top level NodeDuties, but it seems overkill to reconstruct the entire NodeDuties for the sake of changing a struct field…
Well, I have my (somewhat unsatisfactory) answer. The short answer is that I can’t find a good reason. I don’t think this is possible as of right now to change the NodeInfo at runtime because no one piece of the program seems to have ownership of it.
Update
So I put together a patch that centralizes access to NodeInfo and removes some redundant information. Now NodeInfo only holds information particular to this node that isn’t already handled by routing. That includes the used_space, the root_dir, reward_key, and similar such things. Then AdultDuties and ElderDuties are held alongside the NodeInfo that provide information that is only relevant to adults and to elders respectively.
The net effect is that I think it achieves much better separation of responsibility, reduces redundancy, and allows for better state access patterns (e.g. updating the reward_key, which is what I was originally aiming to do…).
Would be awesome to see this as a PR, @Scorch . Could you fire one up?
This is something we’ve been wanting to get to, just haven’t had time on @Scorch. There’s definitely a lot of scope for improvements across the whole codebase, so this sort of thing is great to see
The NodeInfo is basically static info that so far has not had any reason to change at runtime. So allowing for a single instance/centralized access is definitely good.
For allowing e.g. reward_key to be updated by a node operator at runtime, all of that would probably have changed anyway (needs an API for it etc.). If you want to account for that / have that in mind in the PR, feel free to do so!
The PR actually wraps up another fix where UsedSpace was getting constructed twice and the global used_value wasn’t syncing between chunk stores from what I coud tell. Since the former affected the implementation of the latter, they got a bit interleaved.
Ah ok thanks, that’s eventually what I figured must’ve been the case
As for an API for changing reward key, that’s pretty much the context. I’m currently working on a pet project to start implement just such a thing, which was the original motivation for this thread.
Just FYI, there is a branch coming in where the clippy stuff will be fixed alongside. But looks like it wasn’t finalised before EOD. Will ping you monday when that’s in for the rebase
Ah whoops, my bad, looks like I jumped the gun on that one. I submitted a quick PR to clear the clippy stuff before seeing your post I can keep it open if the other one isn’t too far along and there’s no special requirements, otherwise feel free to ignore it and I’ll close out the GitHub issue.
I can keep it open if the other one isn’t too far along and there’s no special requirements, otherwise feel free to ignore it and I’ll close out the GitHub issue.