Skip to content

Home

Jump to bottom
Wesley Shillingford edited this page Dec 19, 2018 · 37 revisions

This lists changes I would like to make:

  • Replace std::bind with lambda:

https://stackoverflow.com/questions/15598607/when-should-i-use-stdbind

std::bind will probably be deprecated in the future https://stackoverflow.com/questions/33835922/why-should-bind-be-deprecated

Scott Meyers - Effective Modern C++ Item 34.

I understand the boost asio/beast examples have used it in the past, but it is going to be a deprecated feature soon.

https://youtu.be/zt7ThwVfap0?t=1511

For instance, boost callable traits: https://www.boost.org/doc/libs/1_68_0/libs/callable_traits/doc/html/index.html

https://github.com/nanocurrency/raiblocks/pull/1273/files

  • Wiki "Build instructions" incorrect boost path
  • Use flat_map, sorted vector or custom block allocator, instead of raw std::map. #define BOOST_POOL_NO_MT#include <boost/pool/pool_alloc.hpp>#include int main(){ typedef boost::fast_pool_allocator<int, boost::default_user_allocator_new_delete, boost::details::pool::default_mutex, 64, 128> allocator; std::list<int, allocator> l; for (int i = 0; i < 1000; ++i) l.push_back(i); l.clear(); boost::singleton_pool<boost::fast_pool_allocator_tag, sizeof(int)>:: purge_memory(); }

From https://theboostcpplibraries.com/boost.pool

https://www.appveyor.com/docs/windows-images-software/#boost

It makes sense to remove cache as well, but this causes ccache to not be found in travis when using apt-get. I've left it as is. It is also recommended to call apt-get update when calling apt-get install due to, but the ci probably doesn't. Docker uses intermediate caches for each command which is not available in ci by default in general.

Use intermediate cache with travis? Boost from a repository? TODO figure out best course of action

  • Use more std algorithms
  • Range loops, also boost::range?
  • Line length to clang format? Lines too long. Conditions can be factored out in if statements.
  • Replace std::function with auto, template or another inplaced version. It can have a SBO like std::string for small callables, otherwise otherwise it allocates.

If using callables as a function argument, then declare it as a template parameter.

From the SG14 (gaming) working group have an implementation: https://github.com/WG21-SG14/SG14/blob/master/SG14/inplace_function.h

Or even make your own: https://www.youtube.com/watch?v=VY83afAJUIg

  • Reorder member variables to reduce unnecessary packing and less cache misses by joining frequently used data together. Be careful about shared memory as it locks the whole cache line. https://stackoverflow.com/questions/892767/optimizing-member-variable-order-in-c
  • Common.hpp another place that namespace std is being polluted
  • Investigate if it is possible to run a multi-node (i.e run on both live and beta network at once).
  • .gitmodules references beast
  • Check that Big5 are noexcept where possible (especially move constructors)
  • Remove operator[] being used with std::map/std::unordered_map. See Effective C++ STL Item. Use find() and an InsertOrUpdate() method.
  • Avoid allocations. Consider using jemalloc. ::operator new(size_t) is most likely a wrapper around std::malloc in most implementations, which is good for large blocks (array/vector), but is inefficient for much smaller ones.
  • Backup representative in case your one goes down?
  • Are shared_ptrs abused?
  • Replace polymorphic inheritance, with static inheritance with templates?
  • Any branches which can be removed? branches which can be removed (make testing harder). if (type == UDP => ).
  • Avoid syntax like this: for (auto i (node.wallets.items.begin ()), n (node.wallets.items.end ()); i != n; ++i)

It is the old iterator style for loop, use a ranged for loop (I've seen them elsewhere in the code, but it should be consistent) for (auto wallet :: node.wallets.items) { }

  • Why is a std::deque used in node.cpp line 3271? This data structure is generally only used when it is desirable to insert at the beginning (in addition to the end), otherwise a std::vector is more beneficial.
  • 6 - Raw for loops don't show intent like a standard algorithm does, and I prefer to use an algorithm where possible. In the same area of code, replace the whole loop with something like: std::transform(roots.begin(), roots.end(), std::back_inserter(result), [](const auto& root) { return root->election->status.winner; }

Something like: for (auto & entry : list_probable_rep_weights ()) { result = result + entry.rep_weight.number (); }

Can be replaced with: auto result = std::accumulate(list_probable_rep_weights.begin(), list_probable_rep_weights.end(), static_castrai::uint128_t(0), [](auto count, auto& entry) { return count + entry.rep_weight.number (); }

Or even better with ranges v3 library it becomes: auto result = ranges::accumulate(list_probable_rep_weights, static_castrai::uint128_t(0), [](auto count, auto& entry) { return count + entry.rep_weight.number (); }

(I'm sure there's many other useful algorithms that can be used...).

  • Can precompiled headers speed up build? It's mostly dependencies which take time I think. Need to measure.
  • Too many public variables. Encapsulation, well defined inheritances...
  • rpc.hpp mrai_ should be swapped to be consistent. same with payment & work
  • Order of includes should be grouped by most dependent to least to make sure header files contain all necessary include dependencies to be parsed.
  • common.hpp - make deserialize virtual? seems strange it is not as seems part of the interface. to_bytes could return unique_ptr?

Clone this wiki locally