ScriptInterface CreateObject performance improvement
ClosedPublic
Actions

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

Details

Reviewers: None
Commits: rP22680: Improve performance of ScriptInterface::CreateObject from rP22528 / D2080 by…

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

CreateObject_performance_test.patch6 KBDownload

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.

Update: Test results are rigged by the GetScriptInterfaceAndCBData call. With the call it's slower than originally, without the call it's faster than ever.

Diff Detail

Repository

rP 0 A.D. Public Repository

Branch

/ps/trunk

Lint

Lint OK

Unit

No Unit Test Coverage

Build Status

Buildable 8607
Build 14096: Vulcan Build	Jenkins
Build 14095: arc lint + arc unit

Event Timeline

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

elexis mentioned this in D2128: Use CreateObject in EngineScriptConversions.Jul 26 2019, 2:16 PM

elexis edited the test plan for this revision. (Show Details)Jul 26 2019, 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

Harbormaster completed remote builds in B8607: Diff 9129.Jul 26 2019, 2:38 PM

elexis mentioned this in rP22528: Provide ScriptInterface CreateObject and CreateArray functions to replace Eval….Jul 29 2019, 1:51 PM

vladislavbelov added a subscriber: vladislavbelov.Jul 29 2019, 1:54 PM

vladislavbelov added inline comments.

source/scriptinterface/ScriptInterface.h
139	One may add some complicated templates to prevent const references for basic types and moving types.

elexis added inline comments.Jul 30 2019, 5:51 PM

source/scriptinterface/ScriptInterface.h
139	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.Jul 30 2019, 6:16 PM

source/scriptinterface/ScriptInterface.h
381	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.Jul 30 2019, 6:30 PM

source/scriptinterface/ScriptInterface.h
144	Can the `obj` be invalid here?

elexis added inline comments.Aug 17 2019, 5:22 AM

source/scriptinterface/ScriptInterface.h
144	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	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.Aug 17 2019, 5:30 AM

Closed by commit rP22680: Improve performance of ScriptInterface::CreateObject from rP22528 / D2080 by… (authored by elexis). · Explain Why

This revision was automatically updated to reflect the committed changes.

elexis added a commit: rP22680: Improve performance of ScriptInterface::CreateObject from rP22528 / D2080 by….

elexis edited the test plan for this revision. (Show Details)Aug 21 2019, 4:06 PM

vladislavbelov added inline comments.Aug 21 2019, 9:44 PM

source/scriptinterface/ScriptInterface.h
381	For me it sounds like you continue using `NULL` instead of `nullptr` as other file contains `NULL`.

elexis added inline comments.Aug 21 2019, 9:48 PM

source/scriptinterface/ScriptInterface.h
381	To me it sounds like you are making a wrong comparison.

vladislavbelov added inline comments.Aug 21 2019, 9:50 PM

source/scriptinterface/ScriptInterface.h
381	I disagree, where it is wrong? Everything is about CC.

elexis added inline comments.Aug 21 2019, 9:54 PM

source/scriptinterface/ScriptInterface.h
381	For me it sounds like you continue using NULL instead of nullptr as other file contains NULL. Because there is a difference between a different file and 10 grouped lines.