Page MenuHomeWildfire Games

Make PlayerHasMarket event-based
ClosedPublic

Authored by Itms on Jul 28 2020, 5:20 PM.

Details

Reviewers
wraitii
Freagarach
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rP24114: Make PlayerHasMarket event-based.
Summary

Currently, cmpBarter.PlayerHasMarket is polling-based: it goes through all the entities of the given player, queries cmpIdentity and cmpFoundation from each of them, and tries to find a built market.

Since that function is called each turn in order for the GUI to allow or disallow bartering, this is a huge performance hit.

I detected it using the SpiderMonkey tracelogger while working on upgrading SM. The tracelogger estimates that 15% of the JS execution time is spent in this function...

This patch makes the function event-based. When a market is built or captured, it is added to a list of markets in the Player component. When a market is lost, it is removed from the list. The function now just checks whether the list is empty.

Performance comparison in a non-visual replay, on the current SVN, using some AIs:


The perf loss from the previous code is even more important under SM52.

Test Plan

-

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Itms created this revision.Jul 28 2020, 5:20 PM

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

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (semi):
|    | Missing semicolon.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/components/Player.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/Player.js
| 770| 770| 		}
| 771| 771| 
| 772| 772| 		this.barterEntities = this.barterEntities.filter(
| 773|    |-			ent => ent != msg.entity)
|    | 773|+			ent => ent != msg.entity);
| 774| 774| 	}
| 775| 775| 	if (msg.to == this.playerID)
| 776| 776| 	{

binaries/data/mods/public/simulation/components/Player.js
| 352| »   for·(var·type·in·amounts)
|    | [NORMAL] JSHintBear:
|    | 'type' is already defined.

binaries/data/mods/public/simulation/components/Player.js
| 773| »   »   »   ent·=>·ent·!=·msg.entity)
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.
|    | [NORMAL] ESLintBear (object-curly-spacing):
|    | A space is required before '}'.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/components/tests/test_Player.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/tests/test_Player.js
|  77|  77| });
|  78|  78| AddMock(60, IID_Ownership);
|  79|  79| AddMock(60, IID_Foundation, {});
|  80|    |-cmpPlayer.OnGlobalOwnershipChanged({ "entity": 60, "from": INVALID_PLAYER, "to": 1});
|    |  80|+cmpPlayer.OnGlobalOwnershipChanged({ "entity": 60, "from": INVALID_PLAYER, "to": 1 });
|  81|  81| TS_ASSERT(!cmpPlayer.HasMarket());
|  82|  82| 
|  83|  83| AddMock(61, IID_Identity, {
|    | [NORMAL] ESLintBear (object-curly-spacing):
|    | A space is required before '}'.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/components/tests/test_Player.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/tests/test_Player.js
|  84|  84| 	"GetClassesList": () => {},
|  85|  85| 	"HasClass": (cl) => false
|  86|  86| });
|  87|    |-cmpPlayer.OnGlobalOwnershipChanged({ "entity": 61, "from": INVALID_PLAYER, "to": 1});
|    |  87|+cmpPlayer.OnGlobalOwnershipChanged({ "entity": 61, "from": INVALID_PLAYER, "to": 1 });
|  88|  88| TS_ASSERT(!cmpPlayer.HasMarket());
|  89|  89| 
|  90|  90| AddMock(62, IID_Identity, {
|    | [NORMAL] ESLintBear (object-curly-spacing):
|    | A space is required before '}'.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/components/tests/test_Player.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/tests/test_Player.js
|  91|  91| 	"GetClassesList": () => {},
|  92|  92| 	"HasClass": (cl) => true
|  93|  93| });
|  94|    |-cmpPlayer.OnGlobalOwnershipChanged({ "entity": 62, "from": INVALID_PLAYER, "to": 1});
|    |  94|+cmpPlayer.OnGlobalOwnershipChanged({ "entity": 62, "from": INVALID_PLAYER, "to": 1 });
|  95|  95| TS_ASSERT(cmpPlayer.HasMarket());
|    | [NORMAL] ESLintBear (dot-notation):
|    | ["sell"] is better written in dot notation.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/components/Barter.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/Barter.js
|  82|  82| 	amountsToSubtract[resourceToSell] = amount;
|  83|  83| 	if (cmpPlayer.TrySubtractResources(amountsToSubtract))
|  84|  84| 	{
|  85|    |-		var amountToAdd = Math.round(prices["sell"][resourceToSell] / prices["buy"][resourceToBuy] * amount);
|    |  85|+		var amountToAdd = Math.round(prices.sell[resourceToSell] / prices["buy"][resourceToBuy] * amount);
|  86|  86| 		cmpPlayer.AddResource(resourceToBuy, amountToAdd);
|  87|  87| 
|  88|  88| 		// Display chat message to observers
|    | [NORMAL] ESLintBear (dot-notation):
|    | ["buy"] is better written in dot notation.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/components/Barter.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/Barter.js
|  82|  82| 	amountsToSubtract[resourceToSell] = amount;
|  83|  83| 	if (cmpPlayer.TrySubtractResources(amountsToSubtract))
|  84|  84| 	{
|  85|    |-		var amountToAdd = Math.round(prices["sell"][resourceToSell] / prices["buy"][resourceToBuy] * amount);
|    |  85|+		var amountToAdd = Math.round(prices["sell"][resourceToSell] / prices.buy[resourceToBuy] * amount);
|  86|  86| 		cmpPlayer.AddResource(resourceToBuy, amountToAdd);
|  87|  87| 
|  88|  88| 		// Display chat message to observers

binaries/data/mods/public/simulation/components/Barter.js
|  85| »   »   var·amountToAdd·=·Math.round(prices["sell"][resourceToSell]·/·prices["buy"][resourceToBuy]·*·amount);
|    | [NORMAL] JSHintBear:
|    | ['sell'] is better written in dot notation.

binaries/data/mods/public/simulation/components/Barter.js
|  85| »   »   var·amountToAdd·=·Math.round(prices["sell"][resourceToSell]·/·prices["buy"][resourceToBuy]·*·amount);
|    | [NORMAL] JSHintBear:
|    | ['buy'] is better written in dot notation.
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/2798/display/redirect

Stan added a subscriber: Stan.Jul 28 2020, 5:27 PM
Stan added inline comments.
binaries/data/mods/public/simulation/components/Player.js
765 ↗(On Diff #12949)

Wonder if it's worth caching MatchesClassList(cmpIdentity.GetClassesList(), panelEntityClasses) since it can be called twice in a row.

772 ↗(On Diff #12949)

Maybe we could use splice instead of creating a new array.

Imarok added a subscriber: Imarok.Jul 28 2020, 5:49 PM

Just some style issues found by Jenkins.

binaries/data/mods/public/simulation/components/Player.js
773 ↗(On Diff #12949)

semicolon

binaries/data/mods/public/simulation/components/tests/test_Player.js
80–94 ↗(On Diff #12949)

curly spacing at the end.

Itms updated this revision to Diff 12951.Jul 28 2020, 5:51 PM

Fixed Jenkins lint.

Use splice. Don't check what doesn't need to be checked in the similar code for panel entities.

Add a test for loosing a market.

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/2800/display/redirect

Nescio added a subscriber: Nescio.Jul 28 2020, 8:08 PM

Maybe rename HasMarket to CanBarter? Since you need an entity with the Barter class, not the Market class, and mods might want to do things differently and split Dock or Market functions (cf. D2892/rP23865).

binaries/data/mods/public/simulation/components/Barter.js
76 ↗(On Diff #12949)

Proper sentences end with a full stop.

wraitii accepted this revision.Jul 29 2020, 10:13 AM
wraitii added a subscriber: wraitii.

This was actually reported a little over 2 years ago ;) : https://wildfiregames.com/forum/index.php?/topic/23518-alpha-23-planning/page/12/&tab=comments#comment-356440
(staff forum thread)

Works fine, I agree with the "CanBarter" comment though.

binaries/data/mods/public/simulation/components/Barter.js
77 ↗(On Diff #12951)

I suppose it wouldn't hurt to check for cmpPlayer too, just in case.

This revision is now accepted and ready to land.Jul 29 2020, 10:13 AM
Itms updated this revision to Diff 12962.Jul 29 2020, 2:09 PM

Use CanBarter rather than HasMarket.
Test cmpPlayer and add a unit test for cmpPlayer.SetPlayerID while I'm at it.

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

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (dot-notation):
|    | ["sell"] is better written in dot notation.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/components/Barter.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/Barter.js
|  82|  82| 	amountsToSubtract[resourceToSell] = amount;
|  83|  83| 	if (cmpPlayer.TrySubtractResources(amountsToSubtract))
|  84|  84| 	{
|  85|    |-		var amountToAdd = Math.round(prices["sell"][resourceToSell] / prices["buy"][resourceToBuy] * amount);
|    |  85|+		var amountToAdd = Math.round(prices.sell[resourceToSell] / prices["buy"][resourceToBuy] * amount);
|  86|  86| 		cmpPlayer.AddResource(resourceToBuy, amountToAdd);
|  87|  87| 
|  88|  88| 		// Display chat message to observers
|    | [NORMAL] ESLintBear (dot-notation):
|    | ["buy"] is better written in dot notation.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/components/Barter.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/Barter.js
|  82|  82| 	amountsToSubtract[resourceToSell] = amount;
|  83|  83| 	if (cmpPlayer.TrySubtractResources(amountsToSubtract))
|  84|  84| 	{
|  85|    |-		var amountToAdd = Math.round(prices["sell"][resourceToSell] / prices["buy"][resourceToBuy] * amount);
|    |  85|+		var amountToAdd = Math.round(prices["sell"][resourceToSell] / prices.buy[resourceToBuy] * amount);
|  86|  86| 		cmpPlayer.AddResource(resourceToBuy, amountToAdd);
|  87|  87| 
|  88|  88| 		// Display chat message to observers

binaries/data/mods/public/simulation/components/Barter.js
|  85| »   »   var·amountToAdd·=·Math.round(prices["sell"][resourceToSell]·/·prices["buy"][resourceToBuy]·*·amount);
|    | [NORMAL] JSHintBear:
|    | ['sell'] is better written in dot notation.

binaries/data/mods/public/simulation/components/Barter.js
|  85| »   »   var·amountToAdd·=·Math.round(prices["sell"][resourceToSell]·/·prices["buy"][resourceToBuy]·*·amount);
|    | [NORMAL] JSHintBear:
|    | ['buy'] is better written in dot notation.

binaries/data/mods/public/simulation/components/Player.js
| 352| »   for·(var·type·in·amounts)
|    | [NORMAL] JSHintBear:
|    | 'type' is already defined.
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/2805/display/redirect

Freagarach added inline comments.
binaries/data/mods/public/simulation/components/Player.js
74 ↗(On Diff #12962)

Is the comment needed? (If so, change markets ;) (And put on top.) )

774 ↗(On Diff #12962)

Can be right before if (cmpIdentity (,,,)).

779 ↗(On Diff #12962)

The two ifs can be combined?

if (cmpIdentity)
{
  panels
  bartering
}
782 ↗(On Diff #12962)

MatchesClassList?

785 ↗(On Diff #12962)

cmpFoundation not used, so can be inlined :)

binaries/data/mods/public/simulation/components/tests/test_Player.js
42 ↗(On Diff #12962)

1 as variable? (Here and below.)

Stan added inline comments.Jul 29 2020, 4:44 PM
binaries/data/mods/public/simulation/components/Player.js
782 ↗(On Diff #12962)

Likely slower.

785 ↗(On Diff #12962)

It's used to determine whether the building is a foundation or not, I believe.

Itms added a comment.Aug 7 2020, 12:36 PM

I am testing a few Jenkins improvements on this diff, sorry for the noise!

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

builderr-debug-gcc6.txt
builderr-release-gcc6.txt

Link to build: https://jenkins.wildfiregames.com/job/tests/35/display/redirect

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

Link to build: https://jenkins.wildfiregames.com/job/tests/38/display/redirect

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

Link to build: https://jenkins.wildfiregames.com/job/tests/40/display/redirect

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

Link to build: https://jenkins.wildfiregames.com/job/tests/41/display/redirect

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

Link to build: https://jenkins.wildfiregames.com/job/tests/42/display/redirect

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

Link to build: https://jenkins.wildfiregames.com/job/tests/43/display/redirect

Nescio removed a subscriber: Nescio.Aug 10 2020, 10:46 AM
Freagarach added inline comments.Sep 24 2020, 7:17 PM
binaries/data/mods/public/simulation/components/Player.js
785 ↗(On Diff #12962)

Sure, but it can just be if (!Engine.QueryInterface(msg.entity, IID_Foundation)) then.

Stan added inline comments.Sep 24 2020, 7:21 PM
binaries/data/mods/public/simulation/components/Player.js
785 ↗(On Diff #12962)

Ah yes. Then

if (cmpIdentity && cmpIdentity.HasClass("Barter") && !Engine.QueryInterface(msg.entity, IID_Foundation))
    his.barterEntities.push(msg.entity);
Itms updated this revision to Diff 13671.Oct 28 2020, 9:41 PM

Address Freagarach's comments.

Freagarach accepted this revision.Oct 29 2020, 7:39 AM
  • Works as advertised.
  • Code looks clean (nitpick can be ignored when desired).
  • Event-based is a good step here.
binaries/data/mods/public/simulation/components/Barter.js
78 ↗(On Diff #13671)

(See Nescio's previous comment about the full stop.)

79 ↗(On Diff #13671)

One could also ditch the comment above and combine this if with L75 (the cmpPlayer check), for there may be other conditions added when players are not allowed to barter (e.g. triggers).

Itms added a comment.Oct 29 2020, 10:12 AM

Thanks a lot for the quick review!

binaries/data/mods/public/simulation/components/Barter.js
79 ↗(On Diff #13671)

Yeah, I did split the checks because of the comment, but I will ditch it as it is not a good comment, conditions can change. Thanks ??

This revision was automatically updated to reflect the committed changes.