gossip(gossip,net,sync,tvu): Channel(Managed -> Unmanaged) overhaul #165
Conversation
| peers: []ContactInfo, | ||
| ) error{OutOfMemory}!struct { valid_peers: std.ArrayList(usize), pings: std.ArrayList(PingAndSocketAddr) } { | ||
| peers: []const ContactInfo, | ||
| out: struct { |
There was a problem hiding this comment.
why did you change this to pass in the arraylist's as a parameter vs return them? seems like everywhere we use it we init&deinit so we dont really take advantage of this approach
There was a problem hiding this comment.
Passing it in like this avoids the anonymous struct return, which has to always be deconstructed; another route would be to return a tuple which we could easily deconstruct at the call-site, but it'd be two arraylists. Obviously they're different types, but I dislike the visual ambiguity, so I opted for turning these into out parameters; another consideration is that it would allow us to more easily re-use a buffer in the future if the opportunity ever came about.
| @@ -345,14 +357,6 @@ pub const GossipService = struct { | |||
| exitAndJoin(self.exit, thread); | |||
| }; | |||
|
|
|||
| const responder_thread = try Thread.spawn(.{}, socket_utils.sendSocket, .{ | |||
There was a problem hiding this comment.
i can see the advantages of grouping the socket threads together - but i prefer the order we had it before because it shows the flow of data better
| @@ -56,7 +56,7 @@ const globalRegistry = @import("../prometheus/registry.zig").globalRegistry; | |||
| const GetMetricError = @import("../prometheus/registry.zig").GetMetricError; | |||
| const Counter = @import("../prometheus/counter.zig").Counter; | |||
|
|
|||
| const PacketBatch = ArrayList(Packet); | |||
| const PacketBatch = std.ArrayListUnmanaged(Packet); | |||
There was a problem hiding this comment.
im concerned removing the alloc out of the arraylist could lead to freeing the data with the wrong allocator at some point in time - we want to reduce the amount of dev overhead/thinking required to make changes to the codebase - and it is nice to have but i dont see any significant improvement from this change
There was a problem hiding this comment.
It is my opinion that this actually reduces the mental overhead when you consider the fact that the allocator used to alloc/free the packet batches going through a channel can now be centralized in one specific place, instead of needing to track down the origin of each packet batch through the multithreaded access of a given channel. This also has the effect that the lifetime of any allocator used for this purpose is now more obvious, which will allow us to avoid accidental UAF of allocator interfaces.
Edit: also, just to note, freeing with the wrong allocator is not a huge concern when you consider the fact that GPA actually catches invalid frees.
Send unmanaged arraylists over channels instead of managed ones, reducing redundant bytes used for duplicate allocator interfaces. Also use some out-parameters instead of returning arraylists in some cases.
InKryption
force-pushed
the
ink/managed-to-unmanaged-over-channels
branch
from
598547e to
d1167ef
Compare
|
Closing due to some more complex incompatibilities of this change with the recent repair_service PR (#136). We can revisit this idea & conversation at a later date. |
Send unmanaged arraylists over channels instead of managed ones, reducing redundant bytes used for duplicate allocator interfaces. Also use some out-parameters instead of returning arraylists in some cases.