Page MenuHomeWildfire Games

Use CreateObject in EngineScriptConversions

Authored by elexis on Jul 26 2019, 2:16 PM.



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.

Test Plan

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

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

Event Timeline

elexis created this revision.Jul 26 2019, 2:16 PM

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

Link to build:

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.

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.

elexis updated this revision to Diff 9670.Sep 8 2019, 8:05 PM

Make CreateObject static, so that it can be used without the GetScriptInterfaceAndCBData call!

Vulcan added a comment.Sep 8 2019, 8:10 PM

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

Link to build:

Vulcan added a comment.Sep 8 2019, 8:11 PM

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

Link to build:

elexis added inline comments.Sep 9 2019, 11:03 AM
536 ↗(On Diff #9670)

No, I'm not removing (all of) those, and I already broke it by removing them in rP22856 assuming without having verified that they are unneeded. Gloox provides UTF encoded strings, so they are needed in fact.

elexis updated this revision to Diff 9728.Sep 13 2019, 1:34 AM

Rebase following lobby excursion.

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

Link to build:

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

Link to build:

elexis updated this revision to Diff 9731.Sep 13 2019, 2:23 AM
../../../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'

const char[] engine_version = "0.0.24";


const char* engine_version = "0.0.24";

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

Link to build:

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

Linter detected issues:
Executing section Source...

|   1| /*·Copyright·(C)·2010·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2010"

|  24| class·TestNetMessage·:·public·CxxTest::TestSuite
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classTestNetMessage:' is invalid C code. Use --std or --language to configure the language.

|  60| class·CNetClient·:·public·CFsm
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCNetClient:' is invalid C code. Use --std or --language to configure the language.

|   1| /*·Copyright·(C)·2015·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2015"

| 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.

|   1| /*·Copyright·(C)·2015·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2015"

|  37| class·TestNetComms·:·public·CxxTest::TestSuite
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classTestNetComms:' is invalid C code. Use --std or --language to configure the language.

| 211| »   CMapWriter·writer;
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Unmatched '}'. Configuration: 'MESSAGESSETUP_NOTFIRST'.
Executing section JS...
Executing section cli...

Link to build:

This revision was not accepted when it landed; it landed in state Needs Review.Sep 13 2019, 2:57 AM
This revision was automatically updated to reflect the committed changes.
Owners added subscribers: Restricted Owners Package, Restricted Owners Package, Restricted Owners Package.Sep 13 2019, 2:57 AM