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.
Details
Details
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
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| binaries/data/mods/public/simulation/components/TechnologyManager.js | ||
|---|---|---|
| 57 ↗ | (On Diff #4562) | sad we need to get the interface twice, |
Comment Actions
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...
Comment Actions
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.
| 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) |
Comment Actions
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. |
Comment Actions
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 |
Comment Actions
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.
Comment Actions
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...
Comment Actions
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.
Comment Actions
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...
Comment Actions
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.
Comment Actions
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)
Comment Actions
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.)
Comment Actions
Maybe even rename it unresearchedAutoResearchTechs or something. And I suppose could be an array rather than a set, not sure which way is better.
Comment Actions
Yep, comment can vanish.
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!
Comment Actions
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.