Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace dependencies on aged_containers #3337

Open
scottschurr opened this issue Apr 7, 2020 · 6 comments
Open

Replace dependencies on aged_containers #3337

scottschurr opened this issue Apr 7, 2020 · 6 comments
Labels
Tech Debt Non-urgent improvements

Comments

@scottschurr
Copy link
Collaborator

Issue Description

The collection of aged_containers (see src/ripple/beast/container) are STL-like containers that track the relative age of their contents. The aged_containers are specific to rippled, which means that they are maintained locally. The fact that they are STL-like carries with it all of the associated complexity.

The amount of complexity and maintenance risk that the aged_containers carries is not justified by the role they fill in the rippled code base. We should find std:: (preferable) or boost:: replacements for all uses of the aged_containers in the rippled code base, even if that means incurring some performance penalties. Once all uses of the aged_containers have been replaced, then we should remove the aged_container source code.

@nbougalis nbougalis added the Tech Debt Non-urgent improvements label Apr 9, 2020
@nbougalis
Copy link
Contributor

Alternatively, we should consider whether submitting them for inclusion in Boost makes sense. I know that this would require extra effort on our part, unless someone else is willing to adopt them and push them through Boost review.

@scottschurr
Copy link
Collaborator Author

If Mr. V wants to adopt them I'd be fine with that. But I doubt anyone on this team wants to adopt them thoroughly enough to get them through the Boost review process. I tried doing that once years ago with a small utility that I wrote (and was not accepted). The Boost review process can be brutal.

@intelliot
Copy link
Collaborator

@scottschurr do you still think this is worth doing?

@scottschurr
Copy link
Collaborator Author

Yes, removing the aged_containers from the code base is still very much worth doing. They are very complicated. They have already broken a couple of times as we have moved from one C++ Standard to the next. Fortunately @HowardHinnant was able to figure out fixes. They should be replaced with standard boost (or other library) functionality that someone (other than XRP Ledger developers) is maintaining.

The aged_containers are technical debt at its finest.

@scottschurr
Copy link
Collaborator Author

Case in point: #4486

@intelliot
Copy link
Collaborator

intelliot commented Apr 12, 2023

  • there may be a boost option to consider - Boost Multi-index?
    • would have to do the work of switching it over in order to confirm
    • may need 1-2 container solutions - and it will harm performance. but maybe the perf cost isn't too high.
  • is there a folly or abseil option?
    • we have an indirect dependency on abseil (but google sometimes doesn't maintain old versions of things)
      • google only maintains recipes for ~2 yrs or so
    • the folly available in conan is too old (and there's likewise no guarantee facebook will maintain it going forward)
      • could easily fork the recipe like we are with snappy
      • if folly hasn't changed their build process, it may be as easy as setting the commit hash you want
  • conan makes it easier to add dependencies and update them
  • may be useful to delete the oldest data without doing an expensive search
  • should try to find a solution to the problem that has a maintainer
  • no one has an objection to finding a replacement.
    • but for the time being, if issues crop up with the current implementation, @seelabs can maintain

@intelliot intelliot changed the title Remove dependencies on aged_containers Replace dependencies on aged_containers Apr 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Tech Debt Non-urgent improvements
Projects
None yet
Development

No branches or pull requests

3 participants