net/net_processing: Convert net std::list buffers to std::forward_list #19757

1. 
   JeremyRubin commented at 6:36 am on August 18, 2020: contributor

   Currently we use a std::list for the message queue. This is a doubly-linked list. However, we never insert things in the middle, we only really pull from the front or append to the back. Thus we can replace std::list with std::forward_list, which saves a bit of memory per message (8 bytes, plus having to set the back pointers).

   Not the highest priority PR, but a decent refactor I spotted while looking at this code for an unrelated reason that doesn’t add that much code complexity. Shakes fist at STL for not making std::forward_list store a tail pointer, but it’s easy enough to implement on your own.

2. fanquake added the label P2P on Aug 18, 2020
3. 
   practicalswift commented at 7:14 am on August 18, 2020: contributor

   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.
4. 
   DrahtBot commented at 8:03 pm on August 20, 2020: member

   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.

5. 
   theStack commented at 7:47 pm on September 6, 2020: member

   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?

6. 
   JeremyRubin commented at 7:57 pm on September 6, 2020: contributor

   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.

7. 
   jonatack commented at 10:21 am on September 7, 2020: member

   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.
8. DrahtBot added the label Needs rebase on Sep 29, 2020
9. JeremyRubin force-pushed on Jul 30, 2021
10. JeremyRubin force-pushed on Jul 30, 2021
11. Convert net std::list buffers to std::forward_list and track last element

    Co-authored-by: joshiaastha <joshiaastha6@gmail.com>

    31603fe438
12. JeremyRubin force-pushed on Jul 30, 2021
13. 
    JeremyRubin commented at 4:01 pm on July 30, 2021: contributor

    rebased & bug fixed thanks to @joshiaastha, some benchmarks to come soon.
14. DrahtBot removed the label Needs rebase on Jul 30, 2021
15. DrahtBot added the label Needs rebase on Aug 27, 2021
16. 
    DrahtBot commented at 11:01 am on August 27, 2021: member

    🐙 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”.

17. 
    DrahtBot commented at 12:29 pm on December 22, 2021: member

    • 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.
18. 
    fanquake commented at 6:19 am on December 23, 2021: member

    high level thoughts @mzumsande or @jnewbery ?
19. 
    JeremyRubin commented at 7:42 am on December 23, 2021: contributor

    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.

20. 
    jnewbery commented at 8:15 am on December 23, 2021: member

    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.

21. 
    JeremyRubin commented at 7:14 pm on December 23, 2021: contributor

    @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.
22. 
    MarcoFalke commented at 2:15 pm on April 29, 2022: member

    Closing for now, let us know when it should be reopened.
23. MarcoFalke closed this on Apr 29, 2022

    ---

24. DrahtBot locked this on Apr 29, 2023

Contributors
JeremyRubin practicalswift DrahtBot theStack jonatack fanquake jnewbery MarcoFalke

Labels
P2P Needs rebase