gossipsub: unsubscribe fixes

[arnetheduck]

• fix KeyError when updating metric of unsubscribed topic
• fix unsubscribe message not being sent to all peers causing them to
  keep thinking we're still subscribed
• release memory earlier in a few places

[dryajov]

I really dislike this because it's using a clever and unintuitive trick to create an alias. It's also masking away potential rase conditions, if this proc becomes async it would be very easy to miss why we suddenly get NPEs or out of bounds access. I would rather we use var sub = addr rpcMsg.subscriptions[i] # do something with sub[] which makes things explicit and hard to miss.

[arnetheduck]

I think you might have missed something here, addr is the construct that disables the safety mechanisms in Nim and permits the kind of issues you're referring to because the resulting pointer can become dangling - it should be avoided when possible.
also, it would not be applicable here since this is not a mutable instance.

template is used throughout the codebase to work around nim 1.2 issues in performance hotspots. gossipsub is the biggest memory consumer in nimbus and this is one of the sources of that consumption.

[dryajov]

addr is the construct that disables the safety mechanisms in Nim

Nope, didn't miss that. What I'm referring to here is explicitness. The template in this case is "pretending" that the element has been placed in a variable - for all intents and purposes it's usage looks and feels like that, like here for example - https://github.com/status-im/nim-libp2p/pull/569/files#diff-a99c4291e04a9bde36fc1088e4d03053d57e0fcb54d659fcbc4b313362cf7278R206, this is misleading.

I don't think that the compiler can deduce the size of the array/sequence in this case, so there are no compile checks for out of bounds access and you effectively getting similar guarantees to addr.

The problem with masking it like this is that someone can innadverdly do something like:

template sub(): untyped = rpcMsg.subscriptions[i]
....
del rpcMsg.subscriptions[i]
...
# uhoh - in the best case you're going to crash, in the worst it's you're pointing to the wrong element
echo sub.topic

It won't happen now, because the proc is not async, but this tend to change over the lifetime of the implementation.

[arnetheduck]

deduce the size

the [] operator checks for out-of-bounds at runtime. the pointer returned by addr is not checked any further.

similar guarantees to addr.

addr returns a ptr T - a pointer can for example be copied into a closure, but when that is done, the data that it points to is not copied - this causes it to become dangling, the very thing you're trying to argue is a problem with the template. addr is one of the memory-unsafe constructs in Nim - it doesn't offer any guarantees at all. If you were to use addr with async, you would be circumventing the memory-safety that the language gives you and crash where a template like this wouldn't (it would take a copy of rpcMsg and keep working, albeit inefficiently)

this is misleading.

Generally, rpcMsg is immutable meaning that it cannot change the way you're suggesting in your snippet - you also cannot use addr on it, for similar reasons: you would now be able to mutate it and cause these kinds of issues.

In cases like this where the source data is immutable, in Nim 1.2 you can either take a copy with let (what the previous code did) or use a template (or type it out) - the language doesn't allow you to use addr for good reasons, and even if it did, it would cause worse problems if the function was refactored or turned into async, as noted above.

We can forgo this optimization of course, but doing so comes at a measurable cost.

[dryajov]

Not sure if this holds. The only local variables are handlers which is a pointer/ref and futs. I would imagine that only futs gets copied because it's the only thing on the stack, the rest get lifted and passed as arguments to the closure. The new waiter closure keeps copying futs into the closure environment just as the previous version did, while obfuscating the flow. Where is the gain?

[arnetheduck]

nimbus does not use the data handlers, and typically, if a handler returns a completed future, it is no longer interesting to pass it to the callSoon machinery. The same reasoning holds here as when writing: everything inside {.async.} is copied into a closure environment to guarantee the lifetime of the data. In this particular example, we know more and can exploit it to conserve memory in a hotspot.

The gain is expressed as the "fast path" in the comments - futs will remain empty and no allocations will happen - waiter will not be called at all in this case.

Before implementing tricks like this, you also need to have a clear understanding of exception handling in Nim - in particular, this is a good example of why functions that return Future must not raise.

[dryajov]

Again not sure where is the gain here, wouldn't this do exactly the same amount of copying as the previous version but in a more convoluted way? What am I missing?

[arnetheduck]

To understand this, you need to look into how the compiler transforms the code into a closure iterator - in particular, it will copy the variables that enter the closure onto a heap instance - the closure environment - which then gets passed around by reference. encoded is such a variable, often having significant size.

The best place to see this is the C code. The comment simplifies things by calling it into the future - this is a mental shortcut that is mostly accurate.

before:

				T9_ = (tyObject_FuturecolonObjectType___GXFSekg1U8JRoedGa2vBSA*)0;				T9_ = writeLp__9c71DJYgEH8rgDWbdnwWECg(T8_, (*(*colonenvP_).colonup_).encoded2->data, ((*(*colonenvP_).colonup_).encoded2 ? (*(*colonenvP_).colonup_).encoded2->Sup.len : 0));				asgnRef((void**) (&(*colonenvP_).chronosInternalTmpFuture4), &T9_->Sup);

after:

	asgnRef((void**) (&(*colonenv_).fut1), writeLp__9c71DJYgEH8rgDWbdnwWECg(T4_, encoded->data, (encoded ? encoded->Sup.len : 0)));

If you want more background on how libp2p uses memory, the easiest place is the metrics - seq[byte], Future[void] and string are the 3 kinds of memory that libp2p allocates a lot of and this was one example: to create the future, it first copied encoded into the closure environment, then passed a reference to that environment to writeLp - because the closure is a single allocation that has roughly the same lifetime as the future, the copy of encoded stayed referenced until the future was resolved. This can take a while, and the result is that we see spikes of memory usage in Nimbus.

By doing it this way, the closure environment still has a significant lifetime, but it's now much smaller - in particular, it doesn't hold on to a copy of encoded.

These kinds of optimizations generally don't make sense unless they've been verified to cause trouble - they are rarely noticeable - but gossipsub, because of the fanout, is a pathological case - every message is copied to each peer and goes through 4-5 layers of abstraction each taking multiple copies of it as part of framing it - the fewer such copies, the less memory libp2p will use and keep referenced.

[dryajov]

LGTM. But I'd really like to understand the reasoning behind the things I pointed out.