Page MenuHomeWildfire Games

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

Authored by wraitii on Jan 6 2019, 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 7209
Build 11743: Vulcan BuildJenkins
Build 11742: arc lint + arc unit

Event Timeline

wraitii created this revision.Jan 6 2019, 11:43 AM
Vulcan added a subscriber: Vulcan.Jan 6 2019, 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.Jan 6 2019, 2:00 PM

Right idea, terrible implementation

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

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

Vulcan added a comment.Jan 6 2019, 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.Jan 6 2019, 3:09 PM
Stan added a subscriber: Stan.

Don't forget to bump the year :)

wraitii added a reviewer: Restricted Owners Package.Jan 6 2019, 5:30 PM
Itms added a comment.Jan 6 2019, 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.Jan 6 2019, 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.Jan 7 2019, 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.Jan 7 2019, 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/

Itms added a comment.Apr 16 2019, 4:38 PM

This new method should be const. But apart from that this looks good to me.

wraitii updated this revision to Diff 7769.Apr 19 2019, 11:37 AM

const-correct, @Itms I'm putting this up for you to accept it.

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/1226/display/redirect

Itms accepted this revision.Tue, May 21, 6:51 PM

I just did some quick profiling on the patch. I don't see any difference between patched and unpatched versions on "Combat demo (huge)", so the perf gain is probably minimal. It's not bad to have it in though, and the new method might be useful in the future.

So unless @vladislavbelov thinks of something I missed, this is good for me! Please don't forget to bump the year in both files.

source/simulation2/system/ComponentManager.cpp
932

But such a function would be just checking that handle.m_ComponentCache is allocated, no? So it would be equivalent in terms of logic, and it's easier to use m_ComponentCaches from the manager and keep CEntityHandle slim IMO.

This revision is now accepted and ready to land.Tue, May 21, 6:51 PM
elexis added a subscriber: elexis.Tue, May 21, 7:17 PM

(Maybe rephrase alive as entities with 0 health points can exist, maybe 'exists', or not)

Itms requested changes to this revision.Tue, May 21, 7:19 PM

Ah actually yes, I agree with elexis. EntityExists is better. Something bugged me about the name and I couldn't pinpoint what.

This revision now requires changes to proceed.Tue, May 21, 7:19 PM