qlibs / reflect

C++20 Static Reflection library
322 stars 16 forks source link

results compiling below code with -ftime-trace #35

Closed arturbac closed 8 months ago

arturbac commented 8 months ago

Continuing from our reddit discussion, I am attaching promised results This is test code the same for both obraz

obraz

magic_enum_reflect_ut.cc.json reflect_enum_reflect_ut.cc.json Screenshot_20240228_220426

arturbac commented 8 months ago

You have 8 EvaluateAsRValue with lower and higher times example 100ms wasted for unit test EvaluateAsRValue reflect-src/reflect:337:19, col:76 96,178 ms static_assert(std::string_view{"_42"} == enum_name<42, 256>(sparse::_42));

kris-jusiak commented 8 months ago

Thanks. Fixed. Please compare v1.0.8+. The following are the results for v1.0.8 with static_assert enabled.

Note: It's hard to compare exactly, as there are different features provided and different defaults.

time g++ -std=c++20 magic_enum.cpp # 0.322ms
time g++ -std=c++20 reflect.cpp # 0.318ms // with tests
time g++ -std=c++20 reflect.cpp # 0.187ms // without tests (`#define static_assert(...)`)

Tested code

magic_enum:HEAD

#include <magic_enum/magic_enum_all.hpp>

enum E { _1, _2, _3, _4, _5, _6, _7, _8, _9, _10, _11, _12, _13, _14, _15, _16 };

int main() {
  (void)magic_enum::enum_name(E::_1);
  (void)magic_enum::enum_name(E::_2);
  (void)magic_enum::enum_name(E::_3);
  (void)magic_enum::enum_name(E::_4);
  (void)magic_enum::enum_name(E::_5);
  (void)magic_enum::enum_name(E::_6);
  (void)magic_enum::enum_name(E::_7);
  (void)magic_enum::enum_name(E::_8);
  (void)magic_enum::enum_name(E::_9);
  (void)magic_enum::enum_name(E::_10);
  (void)magic_enum::enum_name(E::_11);
  (void)magic_enum::enum_name(E::_12);
  (void)magic_enum::enum_name(E::_13);
  (void)magic_enum::enum_name(E::_14);
  (void)magic_enum::enum_name(E::_15);
  (void)magic_enum::enum_name(E::_16);
}

reflect:HEAD (v1.0.8)

#include <refelct>

enum E { _1, _2, _3, _4, _5, _6, _7, _8, _9, _10, _11, _12, _13, _14, _15, _16 };

int main() {
  (void)magic_enum::enum_name(E::_1);
  (void)magic_enum::enum_name(E::_2);
  (void)magic_enum::enum_name(E::_3);
  (void)magic_enum::enum_name(E::_4);
  (void)magic_enum::enum_name(E::_5);
  (void)magic_enum::enum_name(E::_6);
  (void)magic_enum::enum_name(E::_7);
  (void)magic_enum::enum_name(E::_8);
  (void)magic_enum::enum_name(E::_9);
  (void)magic_enum::enum_name(E::_10);
  (void)magic_enum::enum_name(E::_11);
  (void)magic_enum::enum_name(E::_12);
  (void)magic_enum::enum_name(E::_13);
  (void)magic_enum::enum_name(E::_14);
  (void)magic_enum::enum_name(E::_15);
  (void)magic_enum::enum_name(E::_16);
}

Relevant changes:

arturbac commented 8 months ago

?? Wall Duration 22,783 ms "reflect::v1_0_8::(anonymous class)::operator()<reflect::v1_0_8::(lambda at /home/artur/projects/amrpc/targeo_amrpc/build/clang-release-time-trace-auto/_deps/reflect-src/reflect:569:33){}>" static_assert(([]<auto expect = [](const bool cond) { return std::array{true}[not cond]; }> {

Wall Duration 68,613 ms "reflect::v1_0_8::(anonymous class)::operator()<reflect::v1_0_8::(lambda at /home/artur/projects/amrpc/targeo_amrpc/build/clang-release-time-trace-auto/_deps/reflect-src/reflect:659:33){}>"

static_assert(([]<auto expect = [](const bool cond) { return std::array{true}[not cond]; }> {

Wall Duration 67,197 ms "reflect::v1_0_8::(anonymous class)::operator()<reflect::v1_0_8::(lambda at /home/artur/projects/amrpc/targeo_amrpc/build/clang-release-time-trace-auto/_deps/reflect-src/reflect:746:33){}>"

static_assert(([]<auto expect = [](const bool cond) { return std::array{true}[not cond]; }> {

and few more ...

kris-jusiak commented 8 months ago

Can you elaborate, pleas. I don't follow this numbers as they are out of context. Is there any project/benchmark I can try or are there any steps I can follow to reproduce it, what setup is used, compiler, how many TUs? I don't see any issues from v1.0.8 with compilations times on benchmarks I'm performing, if anything, it seems faster than other projects, so would appreciate some guidance here so that I can track down what's happening.

arturbac commented 8 months ago

Will continue later after work, can not right now.

kris-jusiak commented 8 months ago

no worries, thanks :+1:

arturbac commented 8 months ago

1) example for reflect in this comments You wrote is invalid (magic_enum:: namespace) 2) reflect doesn't work with clang and enums with undefined underlying type obraz 3) Here You have comparison for reflect with unit tests on top compared to reflect with disabled unit tests for the example You provided in this topic but amended to enum E : uint8_t for clang main_cc And now explanation: For me it doesn't matter how it compares to magic_enum as magic_enum at production I can use only with a rule single enum_name wrapper in single translation unit per enum. When i tried when i started using it use it as constexpr scattering magic_enum::enum_name across project with ~1mln lines of code, it was disaster compiler was spending almost half of total time of building entire solution on compiling enum name from magic_enum. So now I am very cautious to compile time meta and check it carefully. For me it is a snow ball effect when in many files given include is included in project and used, unit tests of external project are simply not allowed at every solution compilation as it is pure waste of time, I don not unit test spdllog, ut, libc++ and many more at compile time, I can't ;-) I have ready pull request and will do it it is up to You what You are going to do with it. Btw amazing! work You did with ut and reflect. Pozdro.

kris-jusiak commented 8 months ago

Okay, thanks for the update, the details and traces. I understand the scale of the project and your requirements. I think it's fair to have option to disable the static tests overall. The only part important for my is that by default the tests will be enabled but can be opt out. Going to merge #37 but will refactor it slightly as a follow-up to avoid #ifdefs all over the place. Thanks.

arturbac commented 8 months ago

Just to let You know reflect:enum_name is 10x faster now in Function Instantiation than magic_enum, excellent work with reflect! When I measured it with time-trace it is on my HW magic_enum : 85,9 ms reflect : 6,9 ms obraz

kris-jusiak commented 8 months ago

Pretty cool, thanks for sharing. Just out of curiosity, it seems like you are compiling quite a big project with a lot translation units, hence was wondering whether you are using precompiled headers for third-party library or that not possible for some reason?

arturbac commented 8 months ago

I am working on few projects in company so situation is complicated :-) The better granulation of source code for multiple cores (many libraries and translation units) the less efficient pch is as You have bottle neck for pch creation at every library. So one of projects that is mostly my baby very well layered into multiple libraries with relatively small translation units, with high quality include hygiene is very fast for compiling on multiple cores but it doesn't like heavy includes, and using pch on it causes actually slower compile times that without pch. And I was hit not by inclusion times with magic_enum but with instantiation as you see 80ms per every enum_name in given enum type in given translation unit is a rather heavy influence on total time when used all across project and pch will not solve instantiation. Second project result of work of multiple ppl has terrible translation units with even 30-50k lines of code and it is using pch. I personally don't use pch in debug mode with IDE as it is affecting proper header hygiene during development.

arturbac commented 8 months ago

Just to validate results, I looked at Your code and I am not sure if comparison is fair, as You cut names at runtime so reflect and magic enum have different outcome, ME generates enum literals while reflect generates functions names and runtime code so this is not very comparable by looking only just at Instantiation time.

kris-jusiak commented 8 months ago

Fixed now by https://github.com/boost-ext/reflect/commit/0f80f4fa447e478d6f96d37830cadcfb6a590bc6. Expecting a bit slower compilation times as more things are done at run-time but not by much.

arturbac commented 8 months ago

In real world case using enum_name without enum bounds it doesn't compile with clang You declared reflect to work with clang so that's why i report

kris-jusiak commented 8 months ago

it's a relatively new constexpr check in clang since clang-16: clang-15 just works https://godbolt.org/z/rPas4sT1z it also works with with clang-16+ with -Wno-enum-constexpr-conversion - https://godbolt.org/z/n8Kcdzr6W. That can be done on the library side but I wanna avoid #pragma ignored in the code so looking for other options and don't wanna do it at run-time.