Personally I find it easier to reason about the correctness of the existing version, and I'm not entirely convinced that the benefit (slight memory saving) justifies the cost (+23 −9) in this case.

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #22782 (Remove unused MaybeSetAddrName by MarcoFalke)
• #22735 ([net] Don't return an optional from TransportDeserializer::GetMessage() by jnewbery)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Interesting idea! I tend to agree with @practicalswift though that probably the memory saving benefit doesn't outweigh the reduced code readability, at least not in the current form when the user part of the data structure suddenly has to take care of maintaining additional internal state that should not be exposed.

Would it be possible to generalize the concept by creating a new container that inherits from std::forward_list and adds the missing parts (tail pointer, push method)? Not sure If its worth it but from a general perspective I would prefer it to the current approach.

Another question, from the perspective of an STL noob: If we want to store a message queue, why we didn't simply choose std::queue? Is it because of the .splice() magic that wouldn't be possible then?

Yeah it would be possible to encapsulate the API to a generic container.

It may be worth doing this IMO because we can replace malloc with a bounded free-list or a circular buffer or something else that can perform a lot better without worrying about the underlying structures.

But for STL magic, yes splicing a list is a bit special so something else would have trouble replicating that.

I think this is interesting. Builds cleanly but lot of the functional tests are failing for me locally. Happy to re-review if they are fixed.

rebased & bug fixed thanks to @joshiaastha, some benchmarks to come soon.

🐙 This pull request conflicts with the target branch and needs rebase.

_{Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".}

• Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
• Is it no longer relevant? ➡️ Please close.
• Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

high level thoughts @mzumsande or @jnewbery ?

i can rebase it if interest -- @joshiaastha was working on some USDT based measures for latency and similar, but I think got stuck before SoB ended.

longer run i'd like for these to be circular buffers with overflow either freezing the net thread or going into a list. I think it'd be really good if a peers memory was bounded.

high level thoughts [...] @jnewbery ?

@fanquake no strong opinion. Saving 8 bytes per network message is extremely low priority for me, but I won't stand in the way if other people think it's worth it.

I think it'd be really good if a peers memory was bounded.

I don't understand why this is a prerequisite or how it fits into a larger project.

@theStack's feedback was to ask if we can encapsulate this messier API, and I said sure, that would also be good towards a longer term more difficult project to implement a circular buffer.

Closing for now, let us know when it should be reopened.