ray-project
[core] Remove const data members and const return by value in core cpp code for performance
What happened + What you expected to happen
-
Core cpp guideline to avoid const or ref member variables. https://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines/avoid-const-or-ref-data-members.html For this issue will only do const data members as most ref data members seem to be of non-copyable/movable objects anyways. Const member variables cannot be moved and results in a generated parent object move constructor that is not noexcept. And then all classes that have a data member with a move constructor without noexcept will also have a move constructor without noexcept. This hurts performance on std container resizes significantly as they will copy instead of move when they need to resize. An example is in here in redis_store_client.h where RedisKey has a const string and RedisCommand owns a RedisKey. Now vectors of RedisCommand copy instead of move RedisCommands during resizes of the vector. Have a similar example of the issue in the compiler explorer link below.
-
Returning by const value has no real advantages as a normal assignment will result in the variable being copied into a non-const variable anyways immediately, and doesn't restrict the user from doing anything really. It can also prevent some automatic performance optimizations from happening. It seems to be done throughout the code base and should be able to be removed through a clang-tidy lint https://clang.llvm.org/extra/clang-tidy/checks/readability/const-return-type.html. Example below in compiler explorer shows how it can result in two copies instead of one move on assignment.
CompilerExplorer with examples of the downsides of both. CompilerExplorer link
We should look at condensing current clang-tidy checks into just more important ones that we agree to follow as right now basically every file fails the maybe over-extensive list of checks in the current .clang-tidy file, and then incorporate into ci/cd as a failure condition for those more important checks.
Versions / Dependencies
3.0dev
Reproduction script
N/A
Issue Severity
Low: It annoys or frustrates me.
