Page MenuHomeWildfire Games

ScriptInterface CreateObject performance improvement
ClosedPublic

Authored by elexis on Fri, Jul 26, 2:13 PM.

Details

Summary

rP22528 / D2080 uses the SetProperty call to set the properties of the object to be created in ScriptInterface::CreateObject.
This, in comparison to the previous code, must be a little slower, because there is one JSAutoRequest class instance constructed per property, whereas before it created only one per object (and the according Start / End request calls).
This diff extens the code of this function to only create one JSAutoRequest call, thus making the performance difference indistinguishable.

Test Plan

In order to test the performance, I wrote a test case where the original code was tested against the currently committed CreateObject code and compared against the proposed CreateObject code.
The tests seem a bit inconclusive, because the result will depend on which of the tests is run first (for a reason that is not clear to me - it's not garbage collection as I tested for that too.)

Old CColor JS::Object construction: 113127
Current CreateObject Color: 123451
New CreateObject Color: 109974

Old Grid JS::Object construction: 765894
Current CreateObject Grid: 571565
New CreateObject Grid: 556627
Old CColor JS::Object construction: 116960
Current CreateObject Color: 133201
New CreateObject Color: 111334

Old Grid JS::Object construction: 150888
Current CreateObject Grid: 113665
New CreateObject Grid: 114157

Results do vary. It's not conclusive whether it's objectively slower, than creating the object by hand, but it's conslusive that it's a performance benefit to change the previous CreateObject calls to the new CreateObject calls.

Decide D2128.

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.Fri, Jul 26, 2:13 PM
elexis edited the test plan for this revision. (Show Details)Fri, Jul 26, 2:23 PM
elexis edited the test plan for this revision. (Show Details)

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

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

vladislavbelov added inline comments.
source/scriptinterface/ScriptInterface.h
139 ↗(On Diff #9129)

One may add some complicated templates to prevent const references for basic types and moving types.

elexis added inline comments.Tue, Jul 30, 5:51 PM
source/scriptinterface/ScriptInterface.h
139 ↗(On Diff #9129)

Complicated templates or macros should be avoided as long as possible. The objective of this diff was only to remove the small performance regression (multiple JSAutoRequests). I don't want anyone to tell that I reduced the performance with cleanup. If I want to add complicated code ot improve the performance, I would prefer to do that first where it is needed most. (I don't think the affected code is currently a bottleneck, perhaps except for the AI Grid)

vladislavbelov added inline comments.Tue, Jul 30, 6:16 PM
source/scriptinterface/ScriptInterface.h
381 ↗(On Diff #9129)

I suggest to avoid not CC naming. I suggest to use a name like CreateObjectImpl, CreateObjectInternal or use a namespace for static functions internal::CreateObject.

vladislavbelov added inline comments.Tue, Jul 30, 6:30 PM
source/scriptinterface/ScriptInterface.h
144 ↗(On Diff #9129)

Can the obj be invalid here?

elexis added inline comments.Sat, Aug 17, 5:22 AM
source/scriptinterface/ScriptInterface.h
144 ↗(On Diff #9129)

The specs remain silent on that https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_reference/JS::Rooted https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_reference/JSObject
It's not (easy) to find in the code what happens.

JS::RootedObject obj(cx); Creates a pointer and the address is set.

So no, should never happen, I don't know about the Out-Of-Memory case. If there is something, it's not documented anywhere.

The later initialization using JS_NewPlainObject can fail due to OOM and then return false, which is caught by this code:
https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_reference/JS_NewPlainObject

Also just for the record, using objectValue.setObject(*obj);doesn't work (probably because that sets the value, not the address)

Now that I test this again, it actually seems to segfault always all the time period. It can only be the SM45 update changing that.
Setting it after CreateObject_ works however.

381 ↗(On Diff #9129)

I'm following the CC:
https://trac.wildfiregames.com/wiki/Coding_Conventions#Introduction

The goal of these coding conventions is to encourage consistency within the code, rather than claiming some particular style is better than any other. As such, the main guideline is:

Primarily, try to match the style of the functions that you're editing (assuming it's at least self-consistent and not too bizarre), in order to avoid making it less self-consistent.

Just below this line there is:

CallFunction_
Eval_
Eval_
SetGlobal_
SetProperty_
SetProperty_
SetPropertyInt_
GetProperty_
GetPropertyInt_
This revision was not accepted when it landed; it landed in state Needs Review.Sat, Aug 17, 5:30 AM
This revision was automatically updated to reflect the committed changes.