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.
Differential D1736
Early-exit when destroying invalid entities (INVALID_ENTITY, already destroyed…) wraitii on Jan 6 2019, 11:43 AM. Authored by
Details
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. Code review.
Diff Detail
Event TimelineComment Actions 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/ Comment Actions Do this in the flushing using the component cache, which also handles already-deleted entities. Comment Actions 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/ Comment Actions 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. Comment Actions
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. Comment Actions 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 ?
Comment Actions
Probably most of them if not all.
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. Comment Actions This type of check was done already in AllocateEntityHandle to centralise in a common function. Comment Actions 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/ Comment Actions 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 Comment Actions 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.
Comment Actions (Maybe rephrase alive as entities with 0 health points can exist, maybe 'exists', or not) Comment Actions Ah actually yes, I agree with elexis. EntityExists is better. Something bugged me about the name and I couldn't pinpoint what. Comment Actions 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. Comment Actions 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 Comment Actions Successful build - Chance fights ever on the side of the prudent. Link to build: https://jenkins.wildfiregames.com/job/differential/1486/display/redirect Comment Actions 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 Comment Actions
Perhaps doing nothing is better than doing something that isnt noticeable to anyone but might have a performance impact, That would warn if an entity was added to the destruction queue and deleted outside of this function. 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.)
Comment Actions True about the warning. I was merely following the spec at https://github.com/0ad/0ad/blob/master/source/simulation2/Simulation2.h#L193
Also not something we want to do ever I guess |