perf(gossip): reduce gossip table memory usage #526

dnut · 2025-01-31T03:04:55Z

The gossip table was previously using a regular hashmap to hold up to 1 million instances of GossipData, which is a 732 byte tagged union due to ContactInfo being 728 bytes. This means the hashmap would be at least 732 MB when full. Running on mainnet I witnessed it growing to over 1 GB.

The bulk of this data is unused due to the union being so large. Most of the union fields are actually much smaller, around 50-100 bytes. There are only around ~5000 contact infos being stored, but each of the ~1 million other elements are being allocated 732 bytes since they are instances of the same union.

The solution for this is to store each union variant's unwrapped type in a separate array. So if an item is 50 bytes, it only uses 50 bytes in the array, instead of using 732 bytes as union. This is implemented in SplitUnionList, which contains a separate ArrayList for every union field. GossipMap is a new struct that wraps this, mapping from GossipKey to the items within SplitUnionList.

I still need to do more rigorous benchmarking, but initial tests are showing that the gossip table is about 20% of its previous size.

when overwriting, there was a bug (recently introduced) where the overwritten item is supposed to be returned to the caller, and they deinit it. instead, the *new* item was being returned as if it was the overwritten item. the caller would deinit the new item, leaving corrupted memory in the gossip table (which would eventually be double freed). the overwritten item would never be freed, so it leaked also adds a test and adds a stack trace to an error log

…ciently

…ayList to save space

…usize index, just using tagged index this simplificaiton means you can no longer index with an integer over all items. this is a limitation, but this extra indexing makes the data structure use a surprising amount of memory. instead i've opted to make the data structure a simpler building block, and the user of the struct can implement a wrapper with this indexing if needed. in this case, its internal indexing was redundant with indexing that GossipMap is already doing. so by stripping out this indexing, GossipMap doesn't need any extra logic to handle it. i've just simplified the code and reduced the memory usage.

0xNineteen

few nits - otherwise lgtm

0xNineteen · 2025-02-05T17:41:32Z

src/gossip/data.zig

@@ -191,6 +191,25 @@ const GossipDataTag = enum(u32) {
    ContactInfo,
    RestartLastVotedForkSlots,
    RestartHeaviestFork,
+
+    pub fn Value(self: GossipDataTag) type {


value lowercase methods?

types are conventionally capitalized

it being capital is correct

0xNineteen · 2025-02-05T17:42:14Z

src/gossip/map.zig

+
+const assert = std.debug.assert;
+
+pub const GossipMap = struct {


would be good to add a doc comment about this struct with more context on why we have it/the pros of it

0xNineteen · 2025-02-05T21:36:48Z

src/gossip/map.zig

+        };
+    }
+
+    pub const Entry = struct {


all these nested structs make sense but also feel off and make it difficult to read - feel like most of these structs can be outside of the struct - since all the logic is already enclosed in map.zig - so the context/space is already defined okay i think

0xNineteen · 2025-02-06T16:58:44Z

src/gossip/service.zig

-                self.logger.err().logf("failed to generate push messages: {any}", .{e});
+                self.logger.err().logf(
+                    "failed to generate push messages: {any}\n{any}",
+                    .{ e, @errorReturnTrace() },


is there a reason for this change?

I encountered this error message and it wasn't very helpful for debugging the issue. Adding the stack trace enabled me to figure out the problem. I'd actually like to move towards this being a standard pattern any time we log an error. I was considering a few options like:

service manager can do this whenever any a service exits with an error

the logger could have a method that's specifically for logging errors and capturing their stack trace (not sure if this would work)

It's an interesting idea. I understand how the error return trace could be helpful here, but I also find it very noisy and distracting. If this is an error message that could happen, but isn't fatal, and happens somewhat regularly (example: "NoPeers found"), I'd rather not commit code with the error return trace printing.

This is like leaving random debug prints in the code, it can be helpful for debugging, but it's annoying to others using the code. Unless I'm specifically trying to debug why this error message happened, I wouldn't like to see the trace.

0xNineteen · 2025-02-06T17:42:58Z

src/utils/collections.zig

@@ -64,6 +64,188 @@ pub fn RecyclingList(
    };
 }

+pub fn SplitUnionList(TaggedUnion: type) type {


would be good to add a doc comment explaining the high level of this helper

0xNineteen · 2025-02-06T17:43:45Z

src/utils/collections.zig

+            }
+        }
+
+        pub const Entry = struct {


maybe move this to be above getEntry

0xNineteen · 2025-02-06T17:48:22Z

src/gossip/map.zig

+        pub fn next(self: *Iterator) ?Entry {
+            defer self.cursor += 1;
+            return if (self.cursor < self.keys.len) .{
+                .key_ptr = &self.keys[self.cursor],


why not just give iterator self and then call getEntryByIndex ?

0xNineteen · 2025-02-06T17:50:47Z

src/gossip/table.zig

+
+        // replace data with newly swapped value
+        const entry = self.store.getEntryByIndex(entry_index);
+        // gossip_data now points to the element which was swapped in and needs updating


think this comment is off

Rexicon226 · 2025-02-08T11:17:32Z

src/gossip/map.zig

+
+    var keys = std.ArrayList(GossipKey).init(std.testing.allocator);
+    defer keys.deinit();
+    // var data = std.ArrayList(GossipVersionedData).init(std.testing.allocator);


Still need this commented out code?

Rexicon226 · 2025-02-08T11:18:56Z

src/gossip/pull_request.zig

-    for (0..num_items) |i| {
-        const versioned_value = gossip_values[i];
+    var gossip_values = gossip_table.store.iterator();
+    for (0..num_items) |_| {


Why not:

Suggested change

for (0..num_items) |_| {

while (gossip_values.next()) |versioned_value|

?

Rexicon226 · 2025-02-08T11:25:06Z

src/gossip/table.zig

@@ -46,7 +47,7 @@ pub fn AutoArrayHashSet(comptime T: type) type {
 ///
 /// Analogous to [Crds](https://github.com/solana-labs/solana/blob/e0203f22dc83cb792fa97f91dbe6e924cbd08af1/gossip/src/crds.rs#L68)
 pub const GossipTable = struct {
-    store: AutoArrayHashMap(GossipKey, GossipVersionedData),
+    store: GossipMap = .{},


Why did you add a default value here and not initialize in the init function instead? I'd rather us use the init function if it already exists, default values are a bit of a smell.

Rexicon226 · 2025-02-08T11:26:34Z

src/gossip/table.zig

+                const metadata = self.map.metadata.items[index];
+                if (metadata.timestamp_on_insertion >= self.minimum_insertion_timestamp) {
+                    switch (self.map.tagOfIndex(index)) {
+                        inline .ContactInfo => |tag| return self.map.getTypedPtr(tag, index),


It is a useless inline keyword, same thing on the next line. I'd rather you just use getTypedPtr(.ContactInfo, no need to be fancy as it's just harder to read.

Rexicon226 · 2025-02-08T11:29:59Z

src/gossip/table.zig

+            .ContactInfo => {
+                const did_remove = self.contact_infos.swapRemove(table_len);
+                std.debug.assert(did_remove);
+                self.contact_infos.put(entry_index, {}) catch unreachable;


I don't understand why these can't fail.

Rexicon226 · 2025-02-08T11:32:14Z

src/gossip/table.zig

-                .ContactInfo => |ci| if (ci.getSocket(.gossip)) |addr| {
-                    if (addr.eql(&gossip_addr)) return try ci.clone();
+            switch (self.store.tagOfIndex(index)) {
+                inline .ContactInfo => |tag| {


Same thing again, useless inline and just complicates the code.

Rexicon226 · 2025-02-08T11:32:53Z

src/utils/collections.zig

+
+        pub const Index = struct {
+            tag: Tag,
+            index: usize,


What do you think about making the sub-index a non-exhaustive enum for type safety?

Rexicon226 · 2025-02-08T11:35:05Z

src/utils/types.zig

@@ -77,6 +77,28 @@ pub fn ReturnType(comptime FnPtr: type) type {
    };
 }

+/// Same as std.EnumFieldStruct, except every field may be a different type
+pub fn EnumStruct(comptime E: type, comptime Data: fn (E) type) type {
+    @setEvalBranchQuota(@typeInfo(E).Enum.fields.len + 5);


Where does the + 5 come from?

Rexicon226 · 2025-02-08T11:35:18Z

src/utils/types.zig

+    for (&struct_fields, @typeInfo(E).Enum.fields) |*struct_field, enum_field| {
+        const T = Data(@field(E, enum_field.name));
+        struct_field.* = .{
+            .name = enum_field.name ++ "",


Why ++ ""?

Rexicon226 · 2025-02-08T11:36:50Z

src/utils/types.zig

+            .type = T,
+            .default_value = null,
+            .is_comptime = false,
+            .alignment = if (@sizeOf(T) > 0) @alignOf(T) else 0,


Why special case @sizeOf(T) == 0? I don't think align(0) does what you think it does. It selects the default alignment for the target, which we don't care about here. (it also doesn't exist in Zig 0.14 anymore).

kprotty · 2025-02-13T14:45:26Z

src/utils/collections.zig

+    const Tag = @typeInfo(TaggedUnion).Union.tag_type.?;
+
+    return struct {
+        lists: sig.utils.types.EnumStruct(Tag, List),


Would it be better to have an ArrayList(u8) thats bump allocated from? Where each item is struct { Tag, DataType } (user reads Tag first, then fieldParentPtr the struct to get the Data ptr).

dnut added 5 commits January 28, 2025 22:00

perf(gossip): reduce gossip table memory usage

577305c

feat(collections): implement SplitUnionList to store unions more effi…

3ee82d6

…ciently

refactor(gossip): use SplitUnionList in GossipMap instead of MultiArr…

e004385

…ayList to save space

dnut changed the title ~~perf(gossip): reduce memory usage~~ perf(gossip): reduce gossip table memory usage Jan 31, 2025

dnut added 3 commits January 30, 2025 20:08

Merge branch 'master' into dnut/perf/gossip/table-mem-usage

3a8723e

fix: style

8134fbf

fix: style

e69ba1a

dnut marked this pull request as ready for review January 31, 2025 03:15

0xNineteen assigned dnut Jan 31, 2025

dnut added 2 commits February 4, 2025 12:36

Merge branch 'master' into dnut/perf/gossip/table-mem-usage

50d85a3

docs(collections): note implicit assertion in SplitUnionList

376e7f4

0xNineteen requested review from 0xNineteen and Rexicon226 February 5, 2025 21:46

0xNineteen requested changes Feb 6, 2025

View reviewed changes

Rexicon226 requested changes Feb 8, 2025

View reviewed changes

kprotty reviewed Feb 13, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf(gossip): reduce gossip table memory usage #526

perf(gossip): reduce gossip table memory usage #526

dnut commented Jan 31, 2025 •

edited

Loading

0xNineteen left a comment •

edited

Loading

0xNineteen Feb 5, 2025

dnut Feb 7, 2025

Rexicon226 Feb 7, 2025

0xNineteen Feb 5, 2025

0xNineteen Feb 5, 2025

0xNineteen Feb 6, 2025

dnut Feb 7, 2025

Rexicon226 Feb 8, 2025

0xNineteen Feb 6, 2025

0xNineteen Feb 6, 2025

0xNineteen Feb 6, 2025

0xNineteen Feb 6, 2025

Rexicon226 Feb 8, 2025

Rexicon226 Feb 8, 2025

Rexicon226 Feb 8, 2025

Rexicon226 Feb 8, 2025

Rexicon226 Feb 8, 2025

Rexicon226 Feb 8, 2025

Rexicon226 Feb 8, 2025

Rexicon226 Feb 8, 2025

Rexicon226 Feb 8, 2025

Rexicon226 Feb 8, 2025

kprotty Feb 13, 2025


		const assert = std.debug.assert;

		pub const GossipMap = struct {

	for (0..num_items) \|_\| {
	while (gossip_values.next()) \|versioned_value\|

perf(gossip): reduce gossip table memory usage #526

Are you sure you want to change the base?

perf(gossip): reduce gossip table memory usage #526

Conversation

dnut commented Jan 31, 2025 • edited Loading

0xNineteen left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnut commented Jan 31, 2025 •

edited

Loading

0xNineteen left a comment •

edited

Loading