There are three calls in EngineScriptConversions which were not updated to use the CreateObject syntax from rP22528. This was done because these methods are more performance relevant than the changed calls and thus should be checked more thoroughly.
Details
Details
Consult D2127 for testing as to whether the proposed change is perfectly equal in statements (i.e. it cannot produce a performance regression).
Diff Detail
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
Comment Actions
Build failure - The Moirai have given mortals hearts that can endure.
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/241/display/redirect
Comment Actions
I performed a new test, and found there is a catch.
The GetScriptInterfaceAndCBData call is adding a measurable difference, but in the original test it was not repeated for each call:
With GetScriptInterfaceAndCBData:
Old Grid JS::Object construction: 752224 New CreateObject Grid: 843466 Old Grid JS::Object construction: 755448 New CreateObject Grid: 803954
Without GetScriptInterfaceAndCBData:
JS::Object construction: 711188 New CreateObject Grid: 696876
Since that call is only used to obtain the JSContext CreateObject, and since the JSContext is already known to all the callers of CreateObject, one can remove it.
Then, according to the data, CreateObject even seems to be a tiny performance improvement.
It should just be clear that it's not written to optimize the performance but to leave cleaner code without adding a performance regression, even if someone uses the word performance improvement.
Comment Actions
Did further testing.
Don't believe the tests.
The results of the test above depend on the order of the tests.
If I measure the old function being run 500 times, then the new function being run 500 times, I get the exact opposite result when I measure the new function first and then the old function afterwards.
Even with GC at each start.
The good news is that I found another optimization, namely not calling GetScriptInterfaceAndCBData when only the JSContext is needed.
This is also the case for at least one other function (deepfreeze).
The function only needs to be called when a private member of ScriptInterface is accessed that isn't the JS context.
So to get more certain test results, I ran the first function, then the second function, then th second function again, then the first function again and measured that, each.
As (more) expected, it's slightly slower to do it that way.
To get an indication as to whether that performance difference is actually relevant, I compared the CreateObject call of the u16 Grid to the memset hunk preceding it, and at least for the Grids, the CreateObject overhead seems negligible:
Part 1 is the memset hunk, Part 2 is the CreateObject call:
Part 1 took: 1203 Part 2 took 7 Part 1 took: 1093 Part 2 took 7 Part 1 took: 1058 Part 2 took 5 Part 1 took: 1511 Part 2 took 7
But if that difference is about 0.5%, then the results that suggest something like 5% performance decrease are contradictory.
Those are 2MB (1024*1024 sized u16 grid), so it should be clear that the CreateObject call is dwarfed by the memset.
After that, I decided to run the entire thing 20 times:
Old Grid JS::Object construction: 729588 New CreateObject Grid: 647929 New CreateObject Grid: 626208 Old Grid JS::Object construction: 650643 Old Grid JS::Object construction: 653228 New CreateObject Grid: 642001 New CreateObject Grid: 631554 Old Grid JS::Object construction: 626023 Old Grid JS::Object construction: 636505 New CreateObject Grid: 662680 New CreateObject Grid: 669159 Old Grid JS::Object construction: 656638 Old Grid JS::Object construction: 638204 New CreateObject Grid: 630381 New CreateObject Grid: 622603 Old Grid JS::Object construction: 659138 Old Grid JS::Object construction: 660891 New CreateObject Grid: 633227 New CreateObject Grid: 628144 Old Grid JS::Object construction: 625517 Old Grid JS::Object construction: 640835 New CreateObject Grid: 643499 New CreateObject Grid: 660958 Old Grid JS::Object construction: 628685 Old Grid JS::Object construction: 625564 New CreateObject Grid: 628149 New CreateObject Grid: 641421 Old Grid JS::Object construction: 640858 Old Grid JS::Object construction: 655373 New CreateObject Grid: 649994 New CreateObject Grid: 626398 Old Grid JS::Object construction: 636205 Old Grid JS::Object construction: 648697 New CreateObject Grid: 642656 New CreateObject Grid: 654895 Old Grid JS::Object construction: 630873 Old Grid JS::Object construction: 627039 New CreateObject Grid: 648640 New CreateObject Grid: 695728 Old Grid JS::Object construction: 652438 Old Grid JS::Object construction: 674896 New CreateObject Grid: 647430 New CreateObject Grid: 650433 Old Grid JS::Object construction: 650396 Old Grid JS::Object construction: 653963 New CreateObject Grid: 657857 New CreateObject Grid: 632974 Old Grid JS::Object construction: 633127 Old Grid JS::Object construction: 634486 New CreateObject Grid: 624888 New CreateObject Grid: 661619 Old Grid JS::Object construction: 664655 Old Grid JS::Object construction: 633409 New CreateObject Grid: 627489 New CreateObject Grid: 631027 Old Grid JS::Object construction: 639107 Old Grid JS::Object construction: 675084 New CreateObject Grid: 663432 New CreateObject Grid: 629744 Old Grid JS::Object construction: 627054 Old Grid JS::Object construction: 631199 New CreateObject Grid: 652127 New CreateObject Grid: 646685 Old Grid JS::Object construction: 670105 Old Grid JS::Object construction: 649837 New CreateObject Grid: 643957 New CreateObject Grid: 662200 Old Grid JS::Object construction: 664663 Old Grid JS::Object construction: 660238 New CreateObject Grid: 639485 New CreateObject Grid: 631581 Old Grid JS::Object construction: 640142 Old Grid JS::Object construction: 632302 New CreateObject Grid: 648909 New CreateObject Grid: 656096 Old Grid JS::Object construction: 632602 Old Grid JS::Object construction: 633627 New CreateObject Grid: 625855 New CreateObject Grid: 646967 Old Grid JS::Object construction: 653792
And in this case it says
old = 25877626
new = 25766979
which is a performance improvement by 0.427%
I need to dismiss the 5% number based on the fact that copying 2MB must be dwarfing the CreateObject call and that it is consistent with the "big measurement".
Since it was so fun, I ran the test again
new = 23210226
old = 23292337
and got 0.352523665% performance improvement.
That is without the Private call.
This is where the story ends until someone comes back with new evidence in rage.
Comment Actions
Make CreateObject static, so that it can be used without the GetScriptInterfaceAndCBData call!
Comment Actions
Build failure - The Moirai have given mortals hearts that can endure.
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/609/display/redirect
Comment Actions
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/100/display/redirect
Comment Actions
Build failure - The Moirai have given mortals hearts that can endure.
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/653/display/redirect
Comment Actions
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/143/display/redirect
Comment Actions
../../../source/scriptinterface/ScriptConversions.cpp:292:120: error: parameter 'val' includes reference to array of unknown bound 'const wchar_t []' template<> void ScriptInterface::ToJSVal<wchar_t[N]>(JSContext* cx, JS::MutableHandleValue ret, const wchar_t (&val)[N]) \ ../../../source/scriptinterface/ScriptConversions.cpp:301:1: note: in expansion of macro 'TOJSVAL_CHAR' TOJSVAL_CHAR() ^
const char[] engine_version = "0.0.24";
to
const char* engine_version = "0.0.24";
Comment Actions
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/146/display/redirect
Comment Actions
Successful build - Chance fights ever on the side of the prudent.
Linter detected issues:
Executing section Source...
source/network/tests/test_NetMessage.h
| 1| /*·Copyright·(C)·2010·Wildfire·Games.
| | [NORMAL] LicenseYearBear:
| | License should have "2019" year instead of "2010"
source/network/tests/test_NetMessage.h
| 24| class·TestNetMessage·:·public·CxxTest::TestSuite
| | [MAJOR] CPPCheckBear (syntaxError):
| | Code 'classTestNetMessage:' is invalid C code. Use --std or --language to configure the language.
source/network/NetClient.h
| 60| class·CNetClient·:·public·CFsm
| | [MAJOR] CPPCheckBear (syntaxError):
| | Code 'classCNetClient:' is invalid C code. Use --std or --language to configure the language.
source/ps/Pyrogenesis.h
| 1| /*·Copyright·(C)·2015·Wildfire·Games.
| | [NORMAL] LicenseYearBear:
| | License should have "2019" year instead of "2015"
source/simulation2/scripting/EngineScriptConversions.cpp
| 172| » » FAIL_VOID("Failed·to·get·Vector3D·constructor");
| | [MAJOR] CPPCheckBear (unknownMacro):
| | There is an unknown macro here somewhere. Configuration is required. If STMT is a macro then please configure it.
source/ps/Pyrogenesis.cpp
| 1| /*·Copyright·(C)·2015·Wildfire·Games.
| | [NORMAL] LicenseYearBear:
| | License should have "2019" year instead of "2015"
source/network/tests/test_Net.h
| 37| class·TestNetComms·:·public·CxxTest::TestSuite
| | [MAJOR] CPPCheckBear (syntaxError):
| | Code 'classTestNetComms:' is invalid C code. Use --std or --language to configure the language.
source/tools/atlas/GameInterface/Handlers/MapHandlers.cpp
| 211| » CMapWriter·writer;
| | [MAJOR] CPPCheckBear (syntaxError):
| | Unmatched '}'. Configuration: 'MESSAGESSETUP_NOTFIRST'.
Executing section JS...
Executing section cli...Link to build: https://jenkins.wildfiregames.com/job/docker-differential/656/display/redirect