Page MenuHomeWildfire Games

Stop serializing TechnologyManager.autoResearchTech
ClosedPublic

Authored by elexis on Dec 5 2017, 4:25 PM.

Details

Summary

In #4239 we noticed that the Aura component and TechnologyManager serialize the Aura and Tech templates. That is useless overhead and should be avoided.
This patch removes the first of multiple properties from TechnologyManager serialization (only serializes the names, not the entire object).
If it's correct I'll remove the others with the same approach.

Test Plan

Notice that we got a nasty OOS which was addressed by rP18752. The TechnologyManager had the same object contents but different backrefs after deserialization.
This is no problem for savegame loading, but causes an OOS on rejoin in MP.
Just not serializing the JSON files to begin with as sanderd17 mentioned in the trac ticket should be the solution to both OOS and unneeded overhead.
But we must be careful af.

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

elexis created this revision.Dec 5 2017, 4:25 PM
bb added inline comments.Dec 5 2017, 5:07 PM
binaries/data/mods/public/simulation/components/TechnologyManager.js
57 ↗(On Diff #4562)

sad we need to get the interface twice,
Actually is there a need for doing so? can't we pass as the argument not just the techs itself, instead of the names (when serializing we substract the names, and in init we could use Get instead of List)

Vulcan added a subscriber: Vulcan.Dec 5 2017, 5:13 PM

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

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (308 tests).ERROR: JavaScript error: simulation/components/TechnologyManager.js line 28
TypeError: Engine.QueryInterface(...).ListAllTechs is not a function
  TechnologyManager.prototype.Init@simulation/components/TechnologyManager.js:28:31
  global.ConstructComponent@simulation/components/tests/setup.js:105:2
  @simulation/components/tests/test_TechnologyManager.js:8:28

In TestComponentScripts::test_scripts:
/mnt/data/jenkins-phabricator/workspace/phabricator/source/simulation2/components/tests/test_scripts.h:44: Error: Test failed: L"Running script simulation/components/tests/test_TechnologyManager.js"
/mnt/data/jenkins-phabricator/workspace/phabricator/source/simulation2/components/tests/test_scripts.h:44: Error: Assertion failed: scriptInterface.LoadScript(pathname, content)
..................................................................................................................................................................................................................................................................................................................
Failed 1 and Skipped 0 of 308 tests
Success rate: 99%
Running debug tests...
Running cxxtest tests (308 tests).ERROR: JavaScript error: simulation/components/TechnologyManager.js line 28
TypeError: Engine.QueryInterface(...).ListAllTechs is not a function
  TechnologyManager.prototype.Init@simulation/components/TechnologyManager.js:28:31
  global.ConstructComponent@simulation/components/tests/setup.js:105:2
  @simulation/components/tests/test_TechnologyManager.js:8:28

In TestComponentScripts::test_scripts:
/mnt/data/jenkins-phabricator/workspace/phabricator/source/simulation2/components/tests/test_scripts.h:44: Error: Test failed: L"Running script simulation/components/tests/test_TechnologyManager.js"
/mnt/data/jenkins-phabricator/workspace/phabricator/source/simulation2/components/tests/test_scripts.h:44: Error: Assertion failed: scriptInterface.LoadScript(pathname, content)
..................................................................................................................................................................................................................................................................................................................
Failed 1 and Skipped 0 of 308 tests
Success rate: 99%
Checking XML files...
Vulcan added a comment.Dec 5 2017, 5:15 PM
Executing section Default...
Executing section Source...
Executing section JS...

binaries/data/mods/public/simulation/components/TechnologyManager.js
|  83| »   »   »   ||·(tech.top·&&·(this.IsTechnologyResearched(tech.top)·||·this.IsTechnologyResearched(tech.bottom))))
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '||'; readers may interpret this as an expression boundary.

binaries/data/mods/public/simulation/components/TechnologyManager.js
| 249| »   »   »   for·(var·component·in·modifiedComponents)
|    | [NORMAL] JSHintBear:
|    | 'component' is already defined.

binaries/data/mods/public/simulation/components/TechnologyManager.js
| 255| »   »   var·cmpTemplateManager·=·Engine.QueryInterface(SYSTEM_ENTITY,·IID_TemplateManager);
|    | [NORMAL] JSHintBear:
|    | 'cmpTemplateManager' is already defined.

binaries/data/mods/public/simulation/components/TechnologyManager.js
| 256| »   »   var·template·=·cmpTemplateManager.GetCurrentTemplateName(msg.entity);
|    | [NORMAL] JSHintBear:
|    | 'template' is already defined.

binaries/data/mods/public/simulation/components/TechnologyManager.js
| 261| »   »   »   var·cmpIdentity·=·Engine.QueryInterface(msg.entity,·IID_Identity);
|    | [NORMAL] JSHintBear:
|    | 'cmpIdentity' is already defined.

binaries/data/mods/public/simulation/components/TechnologyManager.js
| 264| »   »   »   »   var·classes·=·cmpIdentity.GetClassesList();
|    | [NORMAL] JSHintBear:
|    | 'classes' is already defined.

binaries/data/mods/public/simulation/components/TechnologyManager.js
| 322| »   »   »   var·template·=·this.GetTechnologyTemplate(i);
|    | [NORMAL] JSHintBear:
|    | 'template' is already defined.
bb added inline comments.Dec 5 2017, 6:14 PM
binaries/data/mods/public/simulation/components/TechnologyManager.js
57 ↗(On Diff #4562)

Perhaps I should read better, the techdata should be retrieved when deserializing, not serializing (otherwise patch would be useless)

Itms added a subscriber: Itms.Dec 5 2017, 10:23 PM

No major complaints and I don't see how that could OOS, but in order to Accept I would have to run some tests. 🙁
I have some comments on the clarity of the code.

binaries/data/mods/public/simulation/components/TechnologyManager.js
6 ↗(On Diff #4562)

Serialize and Deserialize do go before Init, you can leave them there.

33 ↗(On Diff #4562)

Be more explicit about the "data" and maybe explain where we get them from upon deserialization.

40 ↗(On Diff #4562)

Is there a reason why you don't copy the entire contents and erase the cache and the autoresearched techs? The code here doesn't seem very clear to me compared to the deleted version. But that's personal taste I suppose.

55 ↗(On Diff #4562)

I would put that code in Deserialize because that shouldn't be callable by anything else.

Itms requested changes to this revision.Dec 5 2017, 10:23 PM
This revision now requires changes to proceed.Dec 5 2017, 10:23 PM
elexis planned changes to this revision.Dec 6 2017, 4:23 PM

I'll test to see if we can't get rid of the component cache to begin with.

This should also lower the memory footprint, since we have quite a lot of units with auras!

binaries/data/mods/public/simulation/components/TechnologyManager.js
40 ↗(On Diff #4562)

Conditional copy seemed nicer than copy + delete, also it avoids any hardcoded property name checks.

55 ↗(On Diff #4562)

called on init too

elexis updated this revision to Diff 4612.Dec 6 2017, 7:35 PM

Actually it never read from the object values, so no performance degredation to just save the strings, but a clear up of not too few unused memory allocation.

elexis updated this revision to Diff 4613.Dec 6 2017, 7:36 PM

one semicolon too much

Vulcan added a comment.Dec 6 2017, 7:36 PM

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

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
Vulcan added a comment.Dec 6 2017, 7:37 PM
Executing section Default...
Executing section Source...
Executing section JS...

binaries/data/mods/public/simulation/components/TechnologyManager.js
|  67| »   »   »   ||·(tech.top·&&·(this.IsTechnologyResearched(tech.top)·||·this.IsTechnologyResearched(tech.bottom))))
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '||'; readers may interpret this as an expression boundary.

binaries/data/mods/public/simulation/components/TechnologyManager.js
|  69| »   »   »   this.autoResearchTech.delete(key);;
|    | [NORMAL] JSHintBear:
|    | Unnecessary semicolon.

binaries/data/mods/public/simulation/components/TechnologyManager.js
| 233| »   »   »   for·(var·component·in·modifiedComponents)
|    | [NORMAL] JSHintBear:
|    | 'component' is already defined.

binaries/data/mods/public/simulation/components/TechnologyManager.js
| 239| »   »   var·cmpTemplateManager·=·Engine.QueryInterface(SYSTEM_ENTITY,·IID_TemplateManager);
|    | [NORMAL] JSHintBear:
|    | 'cmpTemplateManager' is already defined.

binaries/data/mods/public/simulation/components/TechnologyManager.js
| 240| »   »   var·template·=·cmpTemplateManager.GetCurrentTemplateName(msg.entity);
|    | [NORMAL] JSHintBear:
|    | 'template' is already defined.

binaries/data/mods/public/simulation/components/TechnologyManager.js
| 245| »   »   »   var·cmpIdentity·=·Engine.QueryInterface(msg.entity,·IID_Identity);
|    | [NORMAL] JSHintBear:
|    | 'cmpIdentity' is already defined.

binaries/data/mods/public/simulation/components/TechnologyManager.js
| 248| »   »   »   »   var·classes·=·cmpIdentity.GetClassesList();
|    | [NORMAL] JSHintBear:
|    | 'classes' is already defined.

binaries/data/mods/public/simulation/components/TechnologyManager.js
| 306| »   »   »   var·template·=·this.GetTechnologyTemplate(i);
|    | [NORMAL] JSHintBear:
|    | 'template' is already defined.
Vulcan added a comment.Dec 6 2017, 7:39 PM

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

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
Vulcan added a comment.Dec 6 2017, 7:39 PM
Executing section Default...
Executing section Source...
Executing section JS...

binaries/data/mods/public/simulation/components/TechnologyManager.js
|  67| »   »   »   ||·(tech.top·&&·(this.IsTechnologyResearched(tech.top)·||·this.IsTechnologyResearched(tech.bottom))))
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '||'; readers may interpret this as an expression boundary.

binaries/data/mods/public/simulation/components/TechnologyManager.js
| 233| »   »   »   for·(var·component·in·modifiedComponents)
|    | [NORMAL] JSHintBear:
|    | 'component' is already defined.

binaries/data/mods/public/simulation/components/TechnologyManager.js
| 239| »   »   var·cmpTemplateManager·=·Engine.QueryInterface(SYSTEM_ENTITY,·IID_TemplateManager);
|    | [NORMAL] JSHintBear:
|    | 'cmpTemplateManager' is already defined.

binaries/data/mods/public/simulation/components/TechnologyManager.js
| 240| »   »   var·template·=·cmpTemplateManager.GetCurrentTemplateName(msg.entity);
|    | [NORMAL] JSHintBear:
|    | 'template' is already defined.

binaries/data/mods/public/simulation/components/TechnologyManager.js
| 245| »   »   »   var·cmpIdentity·=·Engine.QueryInterface(msg.entity,·IID_Identity);
|    | [NORMAL] JSHintBear:
|    | 'cmpIdentity' is already defined.

binaries/data/mods/public/simulation/components/TechnologyManager.js
| 248| »   »   »   »   var·classes·=·cmpIdentity.GetClassesList();
|    | [NORMAL] JSHintBear:
|    | 'classes' is already defined.

binaries/data/mods/public/simulation/components/TechnologyManager.js
| 306| »   »   »   var·template·=·this.GetTechnologyTemplate(i);
|    | [NORMAL] JSHintBear:
|    | 'template' is already defined.
elexis updated the Trac tickets for this revision.Dec 7 2017, 1:56 PM
elexis edited reviewers, added: mimo; removed: Itms.Dec 7 2017, 8:05 PM

Guys I need to get rid of this ;-)

(About the performance, the female inspiration aura shouldn't be cached 800 times if we have 800 units per game. Eventually the component query to the DataTemplateManager will be removed too by making that a direct globalscripts cache)

temple accepted this revision.Dec 7 2017, 10:40 PM

Agreed that we only need to keep track of the names of autoResearch techs that we haven't researched yet. (I don't think you need the serialization comment.)

This revision is now accepted and ready to land.Dec 7 2017, 10:40 PM
temple added a comment.EditedDec 7 2017, 10:49 PM

Maybe even rename it unresearchedAutoResearchTechs or something. And I suppose could be an array rather than a set, not sure which way is better.

Yep, comment can vanish.

In D1109#44979, @temple wrote:

Maybe even rename it unresearchedAutoResearchTechs

Good idea

I suppose could be an array rather than a set, not sure which way is better.

It's the first time in three years that I have chosen a Set instead of an array. The latter are awesome because we can use array functions with them. But in this case we only have the add and remove operation and the latter is a bit ugly with an array (splice(indexOf, 1))

Probably should mention that serialization of Set is given too.
It's in source/simulation2/serialization/BinarySerializer.cpp at the protokey == JSProto_Set condition.
Well that has a TODO and a link to a bugreport actually.
But we do find use of Set in Mirage.js, TerritoryDecay.js, Market.js, so it should be fine.

Thanks for your review!

elexis added a comment.EditedDec 8 2017, 12:13 AM

I didn't knew that pair techs are autoresearched techs to remove the ability to research the other tech of that pair.

When actually testing, I noticed there are many techs in there whose requirements can never be met, most importantly the ones of other civs.
Most of these pair techs are unused now and IMO should be nuked. (We can add them back later easily if we wanted to).
They not only hang around uselessly in memory, they are also all checked again each time the array is scanned again!
I failed to find a quick civ test in Technologies.js.

This revision was automatically updated to reflect the committed changes.