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

Implement a world-local component id caching API and make use of it in cpp components #1036

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Naios
Copy link
Contributor

@Naios Naios commented Sep 3, 2023


As described in #1032

Note: This still requires support for copying the same components with different ids between worlds, for full correctness, but this will not be part of this PR.

@Naios Naios force-pushed the world_local_components branch 7 times, most recently from 2ecb8d5 to d3a55a2 Compare September 3, 2023 22:18
@Naios
Copy link
Contributor Author

Naios commented Sep 3, 2023

@SanderMertens This is almost finished, could you update the unit tests afterwards to your preference?

@SanderMertens
Copy link
Owner

Sure no problem! A few things before this can get merged:

  • 1150 of the 1200 C++ tests are still failing, I'm only expecting a handful of failures that test multiple worlds
  • Can you measure the performance difference between the old and new approaches? I don't expect this to be large, but since it's a high impact change it's better to be sure. Doesn't have to be complicated, you could do something like:
ecs_time_t t = {};
t = ecs_time_measure(&t);

flecs::entity e = world.entity();

for (int i = 0; i < 1000 * 1000; i ++) {
  e.add<Foo>(); // noop after the first call, so less time spent measuring internals
}

printf("time spent = %f\n", ecs_time_measure(&t));

Make sure to measure once the majority of tests are passing.

@Naios Naios force-pushed the world_local_components branch 3 times, most recently from db46049 to 8f01b1e Compare September 9, 2023 19:14
@Naios
Copy link
Contributor Author

Naios commented Sep 9, 2023

I haved benchmarked my approach with your provided code and interestingly it seems slightly faster for some reason (let me know if you get the same results):

Ryzen R9 5900X - MSVC 17.5 (2022) Release x64:
  Current approach     : time spent = 0.026065 | 0.027411 | 0.027331
  With my fix (this PR): time spent = 0.021597 | 0.023357 | 0.022790

The tests still fail on some platforms, probably due to this code, which needs proper handling of the new logic:

// To support member<uintptr_t> and member<intptr_t> register components
    // (that do not have conflicting symbols with builtin ones) for platform
    // specific types.

    if (!flecs::is_same<i32_t, iptr_t>() && !flecs::is_same<i64_t, iptr_t>()) {
        // TODO
        const entity_t id = flecs::_::cpp_type<iptr_t>::id_explicit(world, nullptr, true, flecs::Iptr);
        (void)id;

        ecs_assert(id == flecs::Iptr, ECS_INTERNAL_ERROR, NULL);
        // Remove symbol to prevent validation errors, as it doesn't match with 
        // the typename
        ecs_remove_pair(world, flecs::Iptr, ecs_id(EcsIdentifier), EcsSymbol);
    }

    if (!flecs::is_same<u32_t, uptr_t>() && !flecs::is_same<u64_t, uptr_t>()) {
        // TODO
        const entity_t id = flecs::_::cpp_type<uptr_t>::id_explicit(world, nullptr, true, flecs::Uptr);
        (void)id;

        ecs_assert(id == flecs::Uptr, ECS_INTERNAL_ERROR, NULL);
        // Remove symbol to prevent validation errors, as it doesn't match with 
        // the typename
        ecs_remove_pair(world, flecs::Uptr, ecs_id(EcsIdentifier), EcsSymbol);
    }

Could you take a look at it? For me it is very unclear what the logical difference of id_explicit(...) vs id(...) is.

@Naios Naios force-pushed the world_local_components branch from fa04345 to 8f01b1e Compare September 9, 2023 19:44
@SanderMertens
Copy link
Owner

Hmm, almost all C++ tests are still failing/segfaulting:

PASS: 18, FAIL:1188, EMPTY:  5 (cpp_api.all)

I'd recommend installing bake locally so you can run the test suite without having to push to CI, that should make debugging a lot easier. Re the difference between id/id_explicit, see the comments:

    // Obtain a component identifier for explicit component registration.
    static entity_t id_explicit(world_t *world = nullptr, 
        const char *name = nullptr, bool allow_tag = true, flecs::id_t id = 0,
        bool is_component = true, bool *existing = nullptr)
    // Obtain a component identifier for implicit component registration. This
    // is almost the same as id_explicit, except that this operation 
    // automatically registers lifecycle callbacks.
    // Additionally, implicit registration temporarily resets the scope & with
    // state of the world, so that the component is not implicitly created with
    // the scope/with of the code it happens to be first used by.
    static id_t id(world_t *world = nullptr, const char *name = nullptr, 
        bool allow_tag = true)

@Naios
Copy link
Contributor Author

Naios commented Sep 16, 2023

@SanderMertens I have tried installing bake locally but sadly it had errors building the tests, which I have fixed now.

Overall it seems like the debugging from an IDE with bake is not possible since it does not seem to support IDE integration, which I would prefer for easier debugging. Otherwise I have to attach a debugger manually which is time-consuming.

I have tried setting up the tests with CMake for better IDE integration, which is kind a complicated because it links with bake, but got it to compile: https://github.com/Naios/flecs/commits/cmake_test . Sadly the tests seem to have issues when being run from the CMake generated build files (test failures due to an assertion see below).

entity.c: 1017 ecs_assert(r->table->column_count != 0, ECS_INTERNAL_ERROR, NULL);

 	flecs_get_mut(ecs_world_t * 0x00000148396322b0, unsigned __int64 548, unsigned __int64 9223372105574252560, ecs_record_t * 0x00000148393af3b0) Line 1017	C
 	ecs_get_mut_id(ecs_world_t * 0x00000148396322b0, unsigned __int64 548, unsigned __int64 9223372105574252560) Line 2761	C
 	flecs::set<Position,0>(ecs_world_t * 0x00000148396322b0, unsigned __int64 548, Position && {...}, unsigned __int64 9223372105574252560) Line 19	C++
 	flecs::set<flecs::pair<Position,`Entity_set_override_pair'::`2'::Tgt>,Position>(ecs_world_t * 0x00000148396322b0, unsigned __int64 548, Position && {...}) Line 95	C++
 	flecs::entity_builder<flecs::entity>::set<Position,`Entity_set_override_pair'::`2'::Tgt,flecs::pair<Position,`Entity_set_override_pair'::`2'::Tgt>,Position,0>(Position && {...}) Line 711	C++
 	flecs::entity_builder<flecs::entity>::set_override<Position,`Entity_set_override_pair'::`2'::Tgt,flecs::pair<Position,`Entity_set_override_pair'::`2'::Tgt>,Position,0>(Position && {...}) Line 467	C++
>	Entity_set_override_pair() Line 1553	C++

Also there is a crash in the call to ut_log_fmt(ut_getenv("UT_LOG_FORMAT")); when not being provided a proper environment variable.

Additionally, I have recognized that you run the tests in parallel from the same executable which is not ideal since it also makes debugging non-deterministic. It would be great if there is a flag to allow synchronous single-threaded deterministic execution of the tests as common C++ unit frameworks usually do (most of them run single-threaded with an option to opt-in for parallel execution that is done on a per-process level rather than a per-thread level).

Overall I would have debugged this issues already, but the mentioned problems make it very difficult to do so.

@SanderMertens
Copy link
Owner

Running the tests is not hard, to run all C++ tests you can just do:

bake run test/cpp_api

This runs each test in its own separate process, with multiple processes running in parallel To disable multithreading just specify you want to run on a single thread:

bake run test/cpp_api -- -j 1

To debug a single test you can do

bake run test/cpp_api -- Table.has_id

Also note how each test failure shows you the executable/command to run just that test, which allows you to run it in a debugger:

FAIL: Table.has_id segfaulted
To run/debug your test, do:
export $(bake env)
D:\a\flecs\flecs\test\cpp_api\bin\x86-Mingw-debug\cpp_api.exe Table.has_id

@SanderMertens
Copy link
Owner

Additionally, I have recognized that you run the tests in parallel from the same executable which is not ideal since it also makes debugging non-deterministic

This is not what happens, each test is ran in its own process which ensures determinism & isolation.

@Naios Naios force-pushed the world_local_components branch 6 times, most recently from 1f78c8c to 8374627 Compare October 1, 2023 13:56
@Naios
Copy link
Contributor Author

Naios commented Oct 1, 2023

@SanderMertens I have fixed a few issues in this PR, and the failing unit tests look much more reasonable now.
Sadly, it turns out this PR is a lot more effort than expected since it will also require the static enum data to be reworked to be world-local. Otherwise the unit tests will break when being run from the same process.
Additionally, most multi-world tests seem to be broken and oudated now, since the assumption does not hold anymore that the same entity id is used for the same component when being initialized in different orders.

@Naios Naios force-pushed the world_local_components branch from 8374627 to a3bd866 Compare October 3, 2023 15:32
@Naios Naios force-pushed the world_local_components branch 2 times, most recently from 5ecdae8 to cde4b4f Compare October 3, 2023 16:48
Naios added 2 commits October 3, 2023 18:54
…n cpp components

* Fixes potential conflicting component id issues when initializing
  different worlds with a different order.
* Closes SanderMertens#1032
@Naios Naios force-pushed the world_local_components branch from cde4b4f to da6ab6f Compare October 3, 2023 16:55
@SanderMertens SanderMertens force-pushed the master branch 3 times, most recently from 438bde4 to 220852f Compare December 13, 2023 23:59
@SanderMertens SanderMertens force-pushed the master branch 3 times, most recently from 3b99938 to ac6cdca Compare February 20, 2024 08:24
@SanderMertens SanderMertens force-pushed the master branch 8 times, most recently from ace232e to b968586 Compare March 13, 2024 18:12
@SanderMertens SanderMertens force-pushed the master branch 3 times, most recently from 301243a to 4a5784f Compare April 23, 2024 19:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants