Page MenuHomeWildfire Games

SpiderMonkey changes in preparation for the SM45 upgrade
ClosedPublic

Authored by Itms on Sat, Dec 29, 10:27 PM.

Details

Summary

This is a first diff changing our code in preparation for the SM45 upgrade. The changes are applicable to our current copy and will reduce the number of changes needed during the upgrade.

Here is the list of improvements, for eased review:

Test Plan

Should be tested on all platforms, but the changes are mostly cosmetic and are not supposed to break things.

The removal of the parent reference in objects could be the most risky one. Testable by running some serialization tests, since it happens in that code. Note that the removal itself changes the sim state.

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

Itms created this revision.Sat, Dec 29, 10:27 PM
Vulcan added a subscriber: Vulcan.Sun, Dec 30, 12:54 AM

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

Link to build: https://jenkins.wildfiregames.com/job/differential/850/

Checked out r21946 and applied the patch, replays a (hashed) MP match and used the rejointest to confirm that the hash is the same, at least in this instance. Ran the -rejointest which also succeeds. OS is arch.

(Also one could commit the trivial changes separately too, although the diff is already small enough for it to be negligible.)

On JS_FN:

Use JS_FN instead of JS_FS: in future versions, JS_FS is not public because it isn't supposed to be used in JSAPI code

I didn't see that one of them becomes obsolete or private:
https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_reference/JS_FS
The only difference according to the source and that page is the JSFUN_STUB_GSOPS flag:
https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_reference/Property_attributes
which sounds like having to specify the getter/setters functions for every JSclass that uses that flag.
But we only want to specify getter/setter functions when they are actually doing something other than the default, no?

source/gui/scripting/JSInterface_IGUIObject.cpp
44 ↗(On Diff #7122)

(Could be JS_PS_END or just deleting JSI_props)

53 ↗(On Diff #7122)

(why is there no JS_FN_END -.-)

source/ps/GameSetup/GameSetup.cpp
188 ↗(On Diff #7122)

This sounds like it should become / have been either a GUI event or pulled from JS through JSInterface_*.

This was READONLY | PERMANENT before, why not anymore?
I changed it to true and still works.
The JS script should not be able to change the progress value, as the C++ value will overwrite it again.

How does it relate to SM45?

source/scriptinterface/ScriptInterface.cpp
628 ↗(On Diff #7122)

ok variable unneeded

source/simulation2/serialization/StdDeserializer.cpp
1 ↗(On Diff #7122)

8

On JS_FN:
[…]

I believe the documentation is extremely outdated. The code of the API (particularly header comments) is a much more worthwhile documentation after SM45.

source/ps/GameSetup/GameSetup.cpp
188 ↗(On Diff #7122)

I'm pretty sure this no longer works in SM45 if you don't change it because we are actually changing these values.

wraitii accepted this revision.Mon, Dec 31, 9:45 AM

Compiles on my machine too - I've tested these changes before.
Approving on my side.

source/simulation2/serialization/StdDeserializer.cpp
1 ↗(On Diff #7122)

9 actually :p

This revision is now accepted and ready to land.Mon, Dec 31, 9:45 AM
Itms planned changes to this revision.Wed, Jan 2, 11:50 AM

Thanks for the comments, addressing them today.

In D1716#68020, @elexis wrote:

On JS_FN:

Ach I knew I was forgetting some piece of information. I asked upstream because I had the same questions in mind when reading the docs.
The answer is at: https://mozilla.logbot.info/jsapi/20181228#c15768081

source/ps/GameSetup/GameSetup.cpp
188 ↗(On Diff #7122)

I'm going to test changing it to true in SM45. Indeed originally I figured the change was needed semantically, and that things would break with a stricter SM45. So I included it here.

elexis added a comment.Wed, Jan 2, 1:54 PM

JS_FS->JS_FN correctness:

Re JS_FS/JS_FN/JSFUN_STUB_GSOPS, the source is:

#define JS_FS(name,call,nargs,flags)                                          \
    JS_FNSPEC(name, call, nullptr, nargs, flags, nullptr)
#define JS_FN(name,call,nargs,flags)                                          \
    JS_FNSPEC(name, call, nullptr, nargs, (flags) | JSFUN_STUB_GSOPS, nullptr)

If JS_FS is ancient and obsolete and therefore replaced with JS_FN, then we gain the JSFUN_STUB_GSOPS flag. So we should confirm that this doesn't invalidate the intended code flow.

The specs about JSFUN_STUB_GSOPS say (https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_reference/Property_attributes):

Use JS_PropertyStub getter/setter instead of defaulting to class gsops for property holding function.

So if I read this line correctly, this only changes the default behavior.

But we don't use the default behavior, since we pass JSI_IGUIObject::getProperty and JSI_IGUIObject::setProperty to the JSClass constructor:
https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_reference/JSClass

Use NULL or JS_PropertyStub (SpiderMonkey 31 or older) for default behavior.

(JS_PropertyStub would be passed intead of JSI_IGUIObject::getProperty in the JSClass ctor https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_reference/JS_PropertyStub )

Therefore the removal of the flag does not change the code behavior,
therefore dropping the future obsolete syntax this way is correct.

)

(Also leaving a copy of your dev chat for any legitimately interested reader:)

09:01:24 <Itms> Hello! I have a question about the different JS_FS/FN functions described at https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_reference/JS_FS. I do not understand the differences and specifics of each variant. Could someone give me pointers? Thanks :)
09:23:03 <Waldo> Itms: just use JS_FN these days
09:23:26 <Waldo> Itms: it used to be that we had two different function pointer types, that could be the implementation of a function defined in C/C++, but those days are ancient now
09:25:12 <Itms> Okay. And the other variants in the doc are also obsolete?
09:33:14 <Waldo> Itms: JS_FS is obsolete; the SELF_HOSTED ones are for internal use, basically; JS_SYM_FN is for if you're defining a function like @@iterator where the property key is a symbol
09:34:10 <Waldo> Itms: SpiderMonkey does not have clearly delineated splits between what we think of as public+stable API that is unlikely to change, and internal/unstable API that typically Firefox happens to need that may be esoteric or difficult to use correctly
09:36:20 <Itms> I see :) Thanks!

Only question remaining why the loading progress is not permanent / readonly anymore. (Not so harmful, but why is it changed?)

source/ps/GameSetup/GameSetup.cpp
188 ↗(On Diff #7122)

(It's a GUI event already, but the GUI events can't carry data, that must have been the catch why these are globals rather than event arguments. I guess someone motivated could spend time implementing that or writing a simple ticket, as passing arguments would be useful for other GUI events too (for example the OOS or ReplayFinished event))

elexis added a comment.Wed, Jan 2, 2:04 PM

JS_FS->JS_FN

Actually doesn't make sense what I said, because the flag is passed to the functions in JSFunctionSpec. I guess good faith and experimental testing works too.

Only question remaining why the loading progress is not permanent / readonly anymore. (Not so harmful, but why is it changed?)

I mean it never was constant/read-only since we change it on the JS-side?

I believe somewhere between SM52 and SM60n the JS_GetProperty stuff are removed altogether in favour of a different system which took me ages to understand anyways.

smiley added a subscriber: smiley.Wed, Jan 2, 2:46 PM

I mean it never was constant/read-only since we change it on the JS-side?

It’s not changed in JS AFAIK. The exposed function changes the internal C++ variable, not the exposed JS variable. At least that’s what I recall.

Itms updated this revision to Diff 7299.Sun, Jan 6, 10:26 PM
Itms marked 6 inline comments as done.
Itms edited the summary of this revision. (Show Details)

Addressed comments above, and tested the JSPROP_PERMANENT thing.

This revision is now accepted and ready to land.Sun, Jan 6, 10:26 PM
Itms added inline comments.Sun, Jan 6, 10:27 PM
source/gui/scripting/JSInterface_IGUIObject.cpp
44 ↗(On Diff #7122)

Deleted.

53 ↗(On Diff #7122)

I KNOW

source/ps/GameSetup/GameSetup.cpp
188 ↗(On Diff #7122)

I confirm that with SM 45, changing false to true here breaks with:

ERROR: JavaScript error: can't redefine non-configurable property "g_Progress"
ERROR: JavaScript error: can't redefine non-configurable property "g_LoadDescription"

Your suggestion about GUI events could be something I do in relation with the SM upgrade(s), but I'm not sure I see what you propose. It sounds interesting to rewrite it "properly" rather than with a workaround. But this change is, for now, needed.

Itms requested review of this revision.Sun, Jan 6, 10:27 PM

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

Link to build: https://jenkins.wildfiregames.com/job/differential/937/

elexis accepted this revision.Mon, Jan 7, 2:33 PM

Replacing it explains it as that may violate the PERMANENT flag (although I couldn't find a SM45 related change online).
(The variables could still be readonly and not permanent, and instead of passing 3 bools one could pass the SM flag directly, but that would become inconsistent with the other functions and not so important, bla.)

Conclusively every line is correct, every line (except the JSI Property removal) is necessary for the SM45 upgrade. I didn't check whether the patch is sufficient (i.e. whether any other replacements are necessary).
I tested the previous version, but not the new one, but the new version has so little changes that it would be very unkind of the code to break.
Thanks for the patch!

This revision is now accepted and ready to land.Mon, Jan 7, 2:33 PM
This revision was automatically updated to reflect the committed changes.
Owners added a subscriber: Restricted Owners Package.Sun, Jan 13, 5:37 PM
elexis added inline comments.Fri, Jan 18, 11:18 PM
source/ps/GameSetup/GameSetup.cpp
188 ↗(On Diff #7122)

ScriptEvent can take a JSObject argument (SendEventcan't).

However that sends it to only one GUIObject, which means we'd have to add SendScriptEventToAll to the GUIManager (there is SendEventToAll already) to send it to all GUIObjects. That function would have to use RecurseObject. It seems one could get away with passing the JS value there.