Page MenuHomeWildfire Games

Early-exit when destroying invalid entities (INVALID_ENTITY, already destroyed…)
Needs ReviewPublic

Authored by wraitii on Sun, Jan 6, 11:43 AM.

Details

Reviewers
Itms
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Summary

Destroying INVALID_ENTITY is valid and should do nothing.

The current code will send MT_Destroy messages when doing this, which is un-necessary work and feels kind of broken to me.

Early-exit instead.

Test Plan

Code review.

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
D1736_invalid_entity_deleted
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 6755
Build 11113: Vulcan BuildJenkins
Build 11112: arc lint + arc unit

Event Timeline

wraitii created this revision.Sun, Jan 6, 11:43 AM
Vulcan added a subscriber: Vulcan.Sun, Jan 6, 11:54 AM

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/simulation2/system/ComponentManager.cpp
|   1| /*·Copyright·(C)·2018·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2018"
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/differential/932/

wraitii planned changes to this revision.Sun, Jan 6, 2:00 PM

Right idea, terrible implementation

wraitii updated this revision to Diff 7292.Sun, Jan 6, 2:35 PM

Do this in the flushing using the component cache, which also handles already-deleted entities.

Vulcan added a comment.Sun, Jan 6, 2:37 PM

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/simulation2/system/ComponentManager.cpp
|   1| /*·Copyright·(C)·2018·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2018"
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/differential/934/

Stan added a reviewer: Itms.Sun, Jan 6, 3:09 PM
Stan added a subscriber: Stan.

Don't forget to bump the year :)

wraitii added a reviewer: Restricted Owners Package.Sun, Jan 6, 5:30 PM
Itms added a comment.Sun, Jan 6, 6:24 PM

I really think all the things that happen after this early return are inexpensive. In particular, when posting messages, if the entity is actually fully destroyed, no component subscribed to the message will be found, so there won't be a lot of overhead. If it's not actually fully destroyed, the patch is wrong (I do not think that's the case, but I might be wrong).

I'd be less worried about a patch that just early-exits when ent == INVALID_ENTITY, that would be a trivial change.

I'd actually be interested in seeing the performance of FlushDestroyedComponents with and without the patch on a game.

I really think all the things that happen after this early return are inexpensive. In particular, when posting messages, if the entity is actually fully destroyed, no component subscribed to the message will be found, so there won't be a lot of overhead. If it's not actually fully destroyed, the patch is wrong (I do not think that's the case, but I might be wrong).

I believe that's incorrect: components that are globally subscribed will received the message.

This isn't so much about the performance than about the fact that globally subscribed components will receive bogus messages.

Itms added a comment.Sun, Jan 6, 6:50 PM

Ah right, sorry. But I'd be interested in knowing how much of the cases are actually ent == INVALID_ENTITY. It doesn't sound like there are a lot of cases where an entity would be deleted several times?

Maybe we could place the message posting and the dynamic subscription flattening inside the conditional instead of early returning? I don't like testing only the entity handle to determine whether we should delete the handle plus other things. But that's just an irrational fear 😄

vladislavbelov added inline comments.
source/simulation2/system/ComponentManager.cpp
932

The GetComponentCache function doesn't look like a valid check of aliveness. Because the condition may change. Shouldn't we add a method like CEntityHandle::IsAlive?

Ah right, sorry. But I'd be interested in knowing how much of the cases are actually ent == INVALID_ENTITY. It doesn't sound like there are a lot of cases where an entity would be deleted several times?

Probably most of them if not all.

Maybe we could place the message posting and the dynamic subscription flattening inside the conditional instead of early returning? I don't like testing only the entity handle to determine whether we should delete the handle plus other things. But that's just an irrational fear 😄

So long as the component cache's lifetime is tied to an entity's lifetime, this is safe. I don't think we can expect this to change anytime soon, though I agree it's not ideal.

wraitii updated this revision to Diff 7302.Mon, Jan 7, 8:15 PM
wraitii retitled this revision from Early-exit when destroying INVALID_ENTITY to Early-exit when destroying invalid entities (INVALID_ENTITY, already destroyed…).

This type of check was done already in AllocateEntityHandle to centralise in a common function.
IMO like that it's fine to commit, it's more "canonical" and less tricky.

Vulcan added a comment.Mon, Jan 7, 8:22 PM

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/simulation2/system/ComponentManager.h
|   1| /*·Copyright·(C)·2018·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2018"

source/simulation2/system/ComponentManager.cpp
|   1| /*·Copyright·(C)·2018·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2018"
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/differential/939/