New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cache DomainIndex in PathFinder #15503

Merged
merged 1 commit into from Sep 30, 2018

Conversation

Projects
None yet
5 participants
@reaperrr
Copy link
Contributor

reaperrr commented Aug 12, 2018

Should save a trait look-up for each path search.

Note: Neither Requires<DomainIndexInfo> nor caching in INotifyCreated were enough to avoid NREs, hence the somewhat unusual approach of caching the first time one of the path-finding methods is called.

@@ -64,10 +66,14 @@ public PathFinder(World world)
public List<CPos> FindUnitPath(CPos source, CPos target, Actor self, Actor ignoreActor)
{
var li = self.Info.TraitInfo<MobileInfo>().LocomotorInfo;
if (!cached)

This comment has been minimized.

@teinarss

teinarss Aug 14, 2018

Contributor

Why not just check if domainIndex is null?

This comment has been minimized.

@reaperrr

reaperrr Sep 29, 2018

Contributor

In the (unlikely, but not impossible) case that a mod doesn't use the DomainIndex, it would then try to populate it on every path search.

@chrisforbes

This comment has been minimized.

Copy link
Member

chrisforbes commented Aug 16, 2018

Perf numbers?

I would not have expected this to make any significant difference. It's one trait lookup per path request.

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented Aug 18, 2018

I don't have any numbers.

I'm not expecting this to make a meaningful difference myself, but I thought "it can't hurt" and just might slightly ease some worst-case spikes.

@@ -64,10 +66,14 @@ public PathFinder(World world)
public List<CPos> FindUnitPath(CPos source, CPos target, Actor self, Actor ignoreActor)
{
var li = self.Info.TraitInfo<MobileInfo>().LocomotorInfo;
if (!cached)
{
domainIndex = world.WorldActor.Trait<DomainIndex>();

This comment has been minimized.

@Mailaender

Mailaender Sep 12, 2018

Member

I believe this should be TraitOrDefault and the null checks below left in. Otherwise you are making DomainIndex mandatory instead of just improving performance.

@reaperrr reaperrr force-pushed the reaperrr:cache-domain-index branch from 98e5017 to f524acd Sep 28, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented Sep 28, 2018

Updated.

@reaperrr reaperrr force-pushed the reaperrr:cache-domain-index branch from f524acd to 89a2a36 Sep 28, 2018

@@ -64,9 +66,13 @@ public PathFinder(World world)
public List<CPos> FindUnitPath(CPos source, CPos target, Actor self, Actor ignoreActor)
{
var li = self.Info.TraitInfo<MobileInfo>().LocomotorInfo;
if (!cached && domainIndex == null)

This comment has been minimized.

@abcdefg30

abcdefg30 Sep 29, 2018

Member

I don't think the null check is needed. If cached is false this is certainly null.

Cache DomainIndex in PathFinder
Should save a trait look-up for each path search. Also ditch bogus null checks (this trait is a must-have anyway, so we *want* to crash if it's missing).

@reaperrr reaperrr force-pushed the reaperrr:cache-domain-index branch from 89a2a36 to 55de5e0 Sep 29, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented Sep 29, 2018

Updated.

@abcdefg30 abcdefg30 merged commit adc03c4 into OpenRA:bleed Sep 30, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@abcdefg30

This comment has been minimized.

Copy link
Member

abcdefg30 commented Sep 30, 2018

@reaperrr reaperrr deleted the reaperrr:cache-domain-index branch Oct 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment