Page MenuHomeWildfire Games

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

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

Details

Reviewers
Itms
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rP22300: Early-exit when destroying invalid entities (INVALID_ENTITY, already destroyed…)
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.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.May 21 2019, 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.May 21 2019, 6:51 PM
elexis added a subscriber: elexis.May 21 2019, 7:17 PM

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

Itms requested changes to this revision.May 21 2019, 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.May 21 2019, 7:19 PM
wraitii updated this revision to Diff 8125.May 25 2019, 4:55 PM

EntityIsAlive > EntityExists

wraitii updated this revision to Diff 8127.May 25 2019, 5:11 PM

>> >>> > >

wraitii updated this revision to Diff 8129.May 25 2019, 5:12 PM

wrong diff :p

Itms accepted this revision.May 25 2019, 5:19 PM

Let's just wait for the build to finish: spoiler alert: it will warn about the copyright years ?

Apart from that this is good for me.

This revision is now accepted and ready to land.May 25 2019, 5:19 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/1484/display/redirect

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

Link to build: https://jenkins.wildfiregames.com/job/differential/1486/display/redirect

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

This revision was automatically updated to reflect the committed changes.

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.

Perhaps doing nothing is better than doing something that isnt noticeable to anyone but might have a performance impact,
but there is also the chance to actually do something useful if this case is detected, i.e. to warn about it.

That would warn if an entity was added to the destruction queue and deleted outside of this function.
If I recall correctly, the entityIDs are not reused (IDs unique throughout the time of the simulation), otherwise that could mean that an entityID is unintentionally marked for destruction.

I guess the downside of such a warning is that it can be triggered for the user, and the advantage of the warning is that it can indicate code worthy to improve to the developer.

(As Itms noted, it could also be that some code relies on duplicate destruction messages, and that this seems unlikely.)

ps/trunk/source/simulation2/system/ComponentManager.cpp
927 ↗(On Diff #8130)

(possibly ENSURE)

True about the warning. I was merely following the spec at https://github.com/0ad/0ad/blob/master/source/simulation2/Simulation2.h#L193

(As Itms noted, it could also be that some code relies on duplicate destruction messages, and that this seems unlikely.)

Also not something we want to do ever I guess