Skip to content

Commit

Permalink
#1339 Fix issue with cleaning up iterator resources for systems that …
Browse files Browse the repository at this point in the history
…match nothing
  • Loading branch information
SanderMertens committed Sep 9, 2024
1 parent 1ad8fb5 commit 2c9877a
Show file tree
Hide file tree
Showing 14 changed files with 176 additions and 31 deletions.
14 changes: 13 additions & 1 deletion distr/flecs.c
Original file line number Diff line number Diff line change
Expand Up @@ -34173,6 +34173,7 @@ int flecs_query_finalize_terms(
}

bool cacheable = true;
bool match_nothing = true;
for (i = 0; i < term_count; i ++) {
ecs_term_t *term = &terms[i];
bool prev_is_or = i && term[-1].oper == EcsOr;
Expand All @@ -34183,6 +34184,11 @@ int flecs_query_finalize_terms(
return -1;
}

if (term->src.id != EcsIsEntity) {
/* If term doesn't match 0 entity, query doesn't match nothing */
match_nothing = false;
}

if (scope_nesting) {
/* Terms inside a scope are not cacheable */
ECS_BIT_CLEAR16(term->flags_, EcsTermIsCacheable);
Expand Down Expand Up @@ -34509,6 +34515,9 @@ int flecs_query_finalize_terms(
cacheable && (cacheable_terms == term_count) &&
!desc->order_by_callback);

/* If none of the terms match a source, the query matches nothing */
ECS_BIT_COND(q->flags, EcsQueryMatchNothing, match_nothing);

for (i = 0; i < q->term_count; i ++) {
ecs_term_t *term = &q->terms[i];
/* Post process term names in case they were used to create variables */
Expand Down Expand Up @@ -62641,7 +62650,10 @@ ecs_entity_t flecs_run_intern(

ecs_run_action_t run = system_data->run;
if (run) {
if (!system_data->query->term_count) {
/* If system query matches nothing, the system run callback doesn't have
* anything to iterate, so the iterator resources don't get cleaned up
* automatically, so clean it up here. */
if (system_data->query->flags & EcsQueryMatchNothing) {
it->next = flecs_default_next_callback; /* Return once */
run(it);
ecs_iter_fini(&qit);
Expand Down
23 changes: 12 additions & 11 deletions distr/flecs.h
Original file line number Diff line number Diff line change
Expand Up @@ -461,17 +461,18 @@ extern "C" {
#define EcsQueryMatchOnlyThis (1u << 12u) /* Query only has terms with $this source */
#define EcsQueryMatchOnlySelf (1u << 13u) /* Query has no terms with up traversal */
#define EcsQueryMatchWildcards (1u << 14u) /* Query matches wildcards */
#define EcsQueryHasCondSet (1u << 15u) /* Query has conditionally set fields */
#define EcsQueryHasPred (1u << 16u) /* Query has equality predicates */
#define EcsQueryHasScopes (1u << 17u) /* Query has query scopes */
#define EcsQueryHasRefs (1u << 18u) /* Query has terms with static source */
#define EcsQueryHasOutTerms (1u << 19u) /* Query has [out] terms */
#define EcsQueryHasNonThisOutTerms (1u << 20u) /* Query has [out] terms with no $this source */
#define EcsQueryHasMonitor (1u << 21u) /* Query has monitor for change detection */
#define EcsQueryIsTrivial (1u << 22u) /* Query can use trivial evaluation function */
#define EcsQueryHasCacheable (1u << 23u) /* Query has cacheable terms */
#define EcsQueryIsCacheable (1u << 24u) /* All terms of query are cacheable */
#define EcsQueryHasTableThisVar (1u << 25u) /* Does query have $this table var */
#define EcsQueryMatchNothing (1u << 15u) /* Query matches nothing */
#define EcsQueryHasCondSet (1u << 16u) /* Query has conditionally set fields */
#define EcsQueryHasPred (1u << 17u) /* Query has equality predicates */
#define EcsQueryHasScopes (1u << 18u) /* Query has query scopes */
#define EcsQueryHasRefs (1u << 19u) /* Query has terms with static source */
#define EcsQueryHasOutTerms (1u << 20u) /* Query has [out] terms */
#define EcsQueryHasNonThisOutTerms (1u << 21u) /* Query has [out] terms with no $this source */
#define EcsQueryHasMonitor (1u << 22u) /* Query has monitor for change detection */
#define EcsQueryIsTrivial (1u << 23u) /* Query can use trivial evaluation function */
#define EcsQueryHasCacheable (1u << 24u) /* Query has cacheable terms */
#define EcsQueryIsCacheable (1u << 25u) /* All terms of query are cacheable */
#define EcsQueryHasTableThisVar (1u << 26u) /* Does query have $this table var */
#define EcsQueryCacheYieldEmptyTables (1u << 27u) /* Does query cache empty tables */

////////////////////////////////////////////////////////////////////////////////
Expand Down
23 changes: 12 additions & 11 deletions include/flecs/private/api_flags.h
Original file line number Diff line number Diff line change
Expand Up @@ -146,17 +146,18 @@ extern "C" {
#define EcsQueryMatchOnlyThis (1u << 12u) /* Query only has terms with $this source */
#define EcsQueryMatchOnlySelf (1u << 13u) /* Query has no terms with up traversal */
#define EcsQueryMatchWildcards (1u << 14u) /* Query matches wildcards */
#define EcsQueryHasCondSet (1u << 15u) /* Query has conditionally set fields */
#define EcsQueryHasPred (1u << 16u) /* Query has equality predicates */
#define EcsQueryHasScopes (1u << 17u) /* Query has query scopes */
#define EcsQueryHasRefs (1u << 18u) /* Query has terms with static source */
#define EcsQueryHasOutTerms (1u << 19u) /* Query has [out] terms */
#define EcsQueryHasNonThisOutTerms (1u << 20u) /* Query has [out] terms with no $this source */
#define EcsQueryHasMonitor (1u << 21u) /* Query has monitor for change detection */
#define EcsQueryIsTrivial (1u << 22u) /* Query can use trivial evaluation function */
#define EcsQueryHasCacheable (1u << 23u) /* Query has cacheable terms */
#define EcsQueryIsCacheable (1u << 24u) /* All terms of query are cacheable */
#define EcsQueryHasTableThisVar (1u << 25u) /* Does query have $this table var */
#define EcsQueryMatchNothing (1u << 15u) /* Query matches nothing */
#define EcsQueryHasCondSet (1u << 16u) /* Query has conditionally set fields */
#define EcsQueryHasPred (1u << 17u) /* Query has equality predicates */
#define EcsQueryHasScopes (1u << 18u) /* Query has query scopes */
#define EcsQueryHasRefs (1u << 19u) /* Query has terms with static source */
#define EcsQueryHasOutTerms (1u << 20u) /* Query has [out] terms */
#define EcsQueryHasNonThisOutTerms (1u << 21u) /* Query has [out] terms with no $this source */
#define EcsQueryHasMonitor (1u << 22u) /* Query has monitor for change detection */
#define EcsQueryIsTrivial (1u << 23u) /* Query can use trivial evaluation function */
#define EcsQueryHasCacheable (1u << 24u) /* Query has cacheable terms */
#define EcsQueryIsCacheable (1u << 25u) /* All terms of query are cacheable */
#define EcsQueryHasTableThisVar (1u << 26u) /* Does query have $this table var */
#define EcsQueryCacheYieldEmptyTables (1u << 27u) /* Does query cache empty tables */

////////////////////////////////////////////////////////////////////////////////
Expand Down
5 changes: 4 additions & 1 deletion src/addons/system/system.c
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,10 @@ ecs_entity_t flecs_run_intern(

ecs_run_action_t run = system_data->run;
if (run) {
if (!system_data->query->term_count) {
/* If system query matches nothing, the system run callback doesn't have
* anything to iterate, so the iterator resources don't get cleaned up
* automatically, so clean it up here. */
if (system_data->query->flags & EcsQueryMatchNothing) {
it->next = flecs_default_next_callback; /* Return once */
run(it);
ecs_iter_fini(&qit);
Expand Down
9 changes: 9 additions & 0 deletions src/query/validator.c
Original file line number Diff line number Diff line change
Expand Up @@ -1095,6 +1095,7 @@ int flecs_query_finalize_terms(
}

bool cacheable = true;
bool match_nothing = true;
for (i = 0; i < term_count; i ++) {
ecs_term_t *term = &terms[i];
bool prev_is_or = i && term[-1].oper == EcsOr;
Expand All @@ -1105,6 +1106,11 @@ int flecs_query_finalize_terms(
return -1;
}

if (term->src.id != EcsIsEntity) {
/* If term doesn't match 0 entity, query doesn't match nothing */
match_nothing = false;
}

if (scope_nesting) {
/* Terms inside a scope are not cacheable */
ECS_BIT_CLEAR16(term->flags_, EcsTermIsCacheable);
Expand Down Expand Up @@ -1431,6 +1437,9 @@ int flecs_query_finalize_terms(
cacheable && (cacheable_terms == term_count) &&
!desc->order_by_callback);

/* If none of the terms match a source, the query matches nothing */
ECS_BIT_COND(q->flags, EcsQueryMatchNothing, match_nothing);

for (i = 0; i < q->term_count; i ++) {
ecs_term_t *term = &q->terms[i];
/* Post process term names in case they were used to create variables */
Expand Down
4 changes: 3 additions & 1 deletion test/addons/project.json
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,9 @@
"run_pipeline_multithreaded_tasks",
"pipeline_init_no_terms",
"pipeline_init_no_system_term",
"disable_component_from_immediate_system"
"disable_component_from_immediate_system",
"run_w_empty_query",
"run_w_0_src_query"
]
}, {
"id": "SystemMisc",
Expand Down
39 changes: 39 additions & 0 deletions test/addons/src/Pipeline.c
Original file line number Diff line number Diff line change
Expand Up @@ -3246,3 +3246,42 @@ void Pipeline_disable_component_from_immediate_system(void) {

ecs_fini(world);
}

void Pipeline_run_w_empty_query(void) {
ecs_world_t *world = ecs_init();

ECS_COMPONENT(world, Position);

ecs_system(world, {
.entity = ecs_entity(world, {
.add = ecs_ids(ecs_pair(EcsDependsOn, EcsOnUpdate))
}),
.run = SysA
});

ecs_progress(world, 0);
test_int(sys_a_invoked, 1);

ecs_fini(world);
}

void Pipeline_run_w_0_src_query(void) {
ecs_world_t *world = ecs_init();

ECS_COMPONENT(world, Position);

ecs_system(world, {
.entity = ecs_entity(world, {
.add = ecs_ids(ecs_pair(EcsDependsOn, EcsOnUpdate))
}),
.query.terms = {
{ ecs_id(Position), .src.id = EcsIsEntity }
},
.run = SysA
});

ecs_progress(world, 0);
test_int(sys_a_invoked, 1);

ecs_fini(world);
}
12 changes: 11 additions & 1 deletion test/addons/src/main.c
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,8 @@ void Pipeline_run_pipeline_multithreaded_tasks(void);
void Pipeline_pipeline_init_no_terms(void);
void Pipeline_pipeline_init_no_system_term(void);
void Pipeline_disable_component_from_immediate_system(void);
void Pipeline_run_w_empty_query(void);
void Pipeline_run_w_0_src_query(void);

// Testsuite 'SystemMisc'
void SystemMisc_invalid_not_without_id(void);
Expand Down Expand Up @@ -907,6 +909,14 @@ bake_test_case Pipeline_testcases[] = {
{
"disable_component_from_immediate_system",
Pipeline_disable_component_from_immediate_system
},
{
"run_w_empty_query",
Pipeline_run_w_empty_query
},
{
"run_w_0_src_query",
Pipeline_run_w_0_src_query
}
};

Expand Down Expand Up @@ -2515,7 +2525,7 @@ static bake_test_suite suites[] = {
"Pipeline",
NULL,
NULL,
83,
85,
Pipeline_testcases
},
{
Expand Down
3 changes: 2 additions & 1 deletion test/cpp/project.json
Original file line number Diff line number Diff line change
Expand Up @@ -509,7 +509,8 @@
"register_twice_w_each",
"register_twice_w_run",
"register_twice_w_run_each",
"register_twice_w_each_run"
"register_twice_w_each_run",
"run_w_0_src_query"
]
}, {
"id": "Event",
Expand Down
17 changes: 16 additions & 1 deletion test/cpp/src/System.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1049,7 +1049,7 @@ void System_self_rate_filter(void) {

int32_t count = 0;

auto sys = world.system<Position>("sys")
world.system<Position>("sys")
.rate(2)
.each([&](Position & p){
count ++;
Expand Down Expand Up @@ -2310,3 +2310,18 @@ void System_register_twice_w_each_run(void) {
sys2.run();
test_int(count2, 1);
}

void System_run_w_0_src_query(void) {
flecs::world world;

int count = 0;

world.system()
.write<Position>()
.run([&](flecs::iter&){
count ++;
});

world.progress();
test_int(count, 1);
}
7 changes: 6 additions & 1 deletion test/cpp/src/main.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -492,6 +492,7 @@ void System_register_twice_w_each(void);
void System_register_twice_w_run(void);
void System_register_twice_w_run_each(void);
void System_register_twice_w_each_run(void);
void System_run_w_0_src_query(void);

// Testsuite 'Event'
void Event_evt_1_id_entity(void);
Expand Down Expand Up @@ -3242,6 +3243,10 @@ bake_test_case System_testcases[] = {
{
"register_twice_w_each_run",
System_register_twice_w_each_run
},
{
"run_w_0_src_query",
System_run_w_0_src_query
}
};

Expand Down Expand Up @@ -6592,7 +6597,7 @@ static bake_test_suite suites[] = {
"System",
NULL,
NULL,
72,
73,
System_testcases
},
{
Expand Down
4 changes: 3 additions & 1 deletion test/query/project.json
Original file line number Diff line number Diff line change
Expand Up @@ -679,7 +679,9 @@
"ref_fields_static_src",
"ref_fields_variable_src",
"ref_fields_up_src",
"ref_fields_self_up_src"
"ref_fields_self_up_src",
"0_src_match_nothing",
"0_terms_match_nothing"
]
}, {
"id": "Combinations",
Expand Down
35 changes: 35 additions & 0 deletions test/query/src/Basic.c
Original file line number Diff line number Diff line change
Expand Up @@ -10741,3 +10741,38 @@ void Basic_ref_fields_self_up_src(void) {

ecs_fini(world);
}

void Basic_0_src_match_nothing(void) {
ecs_world_t *world = ecs_mini();

ECS_COMPONENT(world, Position);

ecs_query_t *q = ecs_query(world, {
.terms = {{ .id = ecs_id(Position), .src.id = EcsIsEntity }},
.cache_kind = cache_kind
});

test_assert(q != NULL);
test_assert(q->flags & EcsQueryMatchNothing);

ecs_query_fini(q);

ecs_fini(world);
}

void Basic_0_terms_match_nothing(void) {
ecs_world_t *world = ecs_mini();

ECS_COMPONENT(world, Position);

ecs_query_t *q = ecs_query(world, {
.cache_kind = cache_kind
});

test_assert(q != NULL);
test_assert(q->flags & EcsQueryMatchNothing);

ecs_query_fini(q);

ecs_fini(world);
}
12 changes: 11 additions & 1 deletion test/query/src/main.c
Original file line number Diff line number Diff line change
Expand Up @@ -668,6 +668,8 @@ void Basic_ref_fields_static_src(void);
void Basic_ref_fields_variable_src(void);
void Basic_ref_fields_up_src(void);
void Basic_ref_fields_self_up_src(void);
void Basic_0_src_match_nothing(void);
void Basic_0_terms_match_nothing(void);

// Testsuite 'Combinations'
void Combinations_setup(void);
Expand Down Expand Up @@ -4682,6 +4684,14 @@ bake_test_case Basic_testcases[] = {
{
"ref_fields_self_up_src",
Basic_ref_fields_self_up_src
},
{
"0_src_match_nothing",
Basic_0_src_match_nothing
},
{
"0_terms_match_nothing",
Basic_0_terms_match_nothing
}
};

Expand Down Expand Up @@ -10172,7 +10182,7 @@ static bake_test_suite suites[] = {
"Basic",
Basic_setup,
NULL,
212,
214,
Basic_testcases,
1,
Basic_params
Expand Down

0 comments on commit 2c9877a

Please sign in to comment.