Page MenuHomeWildfire Games

Improvements to simulation hotloading before the SM upgrade
AcceptedPublic

Authored by Itms on Apr 22 2019, 10:25 PM.

Details

Reviewers
wraitii
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Summary

As seen in D1716, SM45 will be more mindful of the "permanent" characters of JS globals. More precisely, it will enforce property attributes, that are documented here.

As a result, properties that have the DontDelete attribute cannot be hotloaded anymore. (The tradeoff is that, now that SM enforces more things, the JIT compiler can make more assumptions, and get performance improvements).

I think the most reasonable solution is to make simulation constants (that is, the scripted component prototypes, and globals such as cheats, sim commands,...) deletable (!DontDelete) but still read-only (ReadOnly). This way, they are still protected to some extend (one cannot redefine them directly, they must be deleted, then recreated) and can still be modified upon hotloading. This patch implements this.

I am open to suggestions on the conceptual level. After all my research I think that the only sensible way to do hotloading while keeping protection against deletion would be to reload the simulation script interface entirely upon hotloading. This is technically possible I believe, and it would allow one to unload some components by deleting them, but it would mean rewriting most of the component manager. I don't think it is reasonable.

Test Plan

Run the tests, test some hotloading, and test the game setup progress bar.

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
/ps/trunk
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 7323
Build 11927: Vulcan BuildJenkins
Build 11926: arc lint + arc unit

Event Timeline

Itms created this revision.Apr 22 2019, 10:25 PM
Itms added a comment.Apr 22 2019, 10:34 PM

Some comments for helping the reviewers ❤

binaries/data/mods/_test.sim/simulation/components/test-modding1.js
1 ↗(On Diff #7811)

The new files are for a new test for the component manager.

source/ps/GameSetup/GameSetup.cpp
187

This is better than what I committed in D1716. Indeed, the progress bars are not constants, and we are not hotloading them, we are merely updating them.

source/scriptinterface/ScriptInterface.cpp
647

This is the core of the patch.

source/scriptinterface/ScriptInterface.h
143

I think this is way clearer.

145

I chose to move the replace parameter (now called currentlyHotloading) at the end for semantics. This does increase the length of lines in general, but I think it's an improvement.

source/scriptinterface/tests/test_ScriptInterface.h
260 ↗(On Diff #7811)

This is unrelated to hotloading, but it deals with modification of functions that are properties of the global object. Modders do this a lot, and it's very useful. So I want to make sure this doesn't break with SM45 or a later version.

source/simulation2/system/ComponentManager.cpp
149

This was a huge oversight!

source/simulation2/tests/test_ComponentManager.h
620 ↗(On Diff #7811)

This test checks that one can redefine scripted components when modding, using Engine.ReRegisterComponent, even outside of hotloading. I think this useful function wasn't covered by the tests until now.

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

Linter detected issues:
Executing section Source...

source/scriptinterface/tests/test_ScriptInterface.h
|   1| /*·Copyright·(C)·2017·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2017"

source/simulation2/tests/test_ComponentManager.h
|   1| /*·Copyright·(C)·2017·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2017"

source/simulation2/system/ComponentManager.cpp
|   1| /*·Copyright·(C)·2018·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2018"
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/differential/1258/display/redirect

Stan added a subscriber: Stan.Apr 22 2019, 10:56 PM
Stan added inline comments.
source/scriptinterface/ScriptInterface.h
143

Maybe you could add @param and @return :)

elexis added a subscriber: elexis.Apr 22 2019, 11:02 PM
elexis added inline comments.
source/ps/GameSetup/GameSetup.cpp
187

The complaint in D1716 is that these JS variables should not be defined in C++ to begin with and every change to these lines that isn't removing them is just adding another layer of the workarounds. I had created D1754 since my explanations on how it could be done didn't carry.

I'm OK with the reasoning. Some comments.

source/ps/GameSetup/GameSetup.cpp
187

Still that doesn't really prevent committing this.

source/scriptinterface/ScriptInterface.cpp
615–617

What does it mean to return false here? We don't replace a property we inherited from a parent prototype?

629

Nice error message 👍

source/scriptinterface/ScriptInterface.h
143

Still not 100% clear to me.
ReadOnly maps to "constant", DontEnum maps to enumerate I suppose, and then what does "DontDelete" map to?

Also where do these name come from?

145

2019 - screens are wide

source/scriptinterface/tests/test_ScriptInterface.h
260 ↗(On Diff #7811)

Would be nice to commit separately but agreed that we need this to stay.

source/simulation2/system/ComponentManager.cpp
334

This makes me think we should use enums (or possibly #define XXX true) instead, reading true, true, true isn't very helpful.

source/simulation2/tests/test_ComponentManager.h
620 ↗(On Diff #7811)

Maybe a separate "test" commit then?

Itms marked an inline comment as done.Apr 22 2019, 11:37 PM

Thanks for the quick comments!

source/ps/GameSetup/GameSetup.cpp
187

Oh you're right elexis! I'll review that one and remove this part of the diff.

source/scriptinterface/ScriptInterface.cpp
615–617

Here returning false is: there is a bug. I can't prove this is never supposed to happen, so I chose to pass the result over, instead of using ENSURE.

629

I'm a bit unsure about "please restart" , because I don't want to assume that the user wants to do that. But I don't think that could happen in a different situation.

source/scriptinterface/ScriptInterface.h
143

The names come from the ecma standard. I mainly made the doc string consistent with the other functions. constant maps with ReadOnly and DontDelete, but we don't keep the latter if we want to make the constant hotloadable.

source/scriptinterface/tests/test_ScriptInterface.h
260 ↗(On Diff #7811)

Okay I'll split it.

source/simulation2/system/ComponentManager.cpp
334

Ideally, those would be named parameters, true by default, but we can't export facultative parameters from C++ to JS 😢

source/simulation2/tests/test_ComponentManager.h
620 ↗(On Diff #7811)

OK.

Stan added inline comments.Apr 22 2019, 11:41 PM
source/scriptinterface/ScriptInterface.cpp
629

Isn't it can't be modified at runtime ? During hotload would imply the variable changes states during hotloading, while it's usually either after or before.

wraitii added inline comments.Apr 22 2019, 11:46 PM
source/scriptinterface/ScriptInterface.cpp
615–617

Maybe at least a logError or a comment // shouldn't happen then? This reads like a "normal" exit.

629

You can always add "please restart the engine to change it".

elexis added inline comments.Apr 26 2019, 4:14 PM
source/ps/GameSetup/GameSetup.cpp
187

If I understand correctly, the SM upgrade needs either D1716 or this (D1844), or both, but this (D1844) should also be committed independent of the SM upgrade, because it intends to improve the SetGlobal function, right? (Assuming it does improve SetGlobal)

source/scriptinterface/ScriptInterface.cpp
615–617

I can't prove this is never supposed to happen

ENSURE are only to help developers, the function should work correctly for all provided input values if we can make it so.
Scary error dialogs should be avoided when we can.
The return false makes the code work correctly if that case was to happen, the stacktrace that ENSURE adds might help however. Otherwise I'd have said just LOGERROR or such.

https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_reference/JS_GetOwnPropertyDescriptor

If desc->obj is null, then this property was not found on the prototype chain.

https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_reference/JS_HasProperty says
Section

JS_HasProperty searches an object, obj, and its prototype chain, for a property with the specified name

So it could happen if the specs change, during an SM upgrade for instance.

629

Why Please restart the engine.? We don't have this recommendation for any other error message.

If someone choses to call SetGlobal with hotloadable = false, but then the developer hotloads that page, then should it warn, should it error out, or should it just not hotload the non-hotloadable global?

647

But does SetGlobal have to be educated about hotloading? Can't the hotloading code just account for permanent properties by not replacing these (and warning the developers that permanent properties won't be hotloaded)?

source/scriptinterface/ScriptInterface.h
145

2019 - mac users ony have one mouse button :/

Is currentlyHotloading really more clear (because the thing that currentlyHotloading triggers may also be useful outside of hotloading context)?

The other names "constant", "enumerate" directly relate to defined JS property... properties.

To me it adds more indirection than transparency, I know what JSPROP_READONLY | JSPROP_PERMANENT means, but "hotloadable" is something WFG branded by the author, not so clearly defined what is meant.

One could also consider a SetGlobalHotloadable proxy function, or just a code comment that warns developers from making globals permanent (since hotloading needs to replace them).

source/simulation2/system/ComponentManager.cpp
334

A flag int?

Itms planned changes to this revision.Apr 27 2019, 11:34 AM
Itms marked an inline comment as done.

Thanks a lot for the input! I pondered a long time with the question of whether SetGlobal should get this or that parameter while writing the patch. Based on the comments I'll remove one of the parameters, which will make the patch a bit more simple.

I'll also split the new tests.

source/ps/GameSetup/GameSetup.cpp
187

The upgrade needs this, because some tests fail if we keep some properties permanent, thus not hotloadable. Improving SetGlobal is indeed a nice side effect that should be committed as well.

source/scriptinterface/ScriptInterface.cpp
629

Hum I made a mistake here, I wanted to warn the user that the non-hotloadable global won't change until the next engine restart, but return true. I shouldn't have copy-pasted the JS_ReportError; return false; from the rest of the function.

If I remove the possibility to create non-hotloadable globals, this should not happen, so this would be an error, and I could remove the out-of-place "Please restart the engine".

647

Yes, that was the idea, but currently every global that is permanent should be hotloadable (like for instance scripted component protoypes, or Commands or Cheats, ...).

Either SetGlobal is educated about hotloading, or

  1. We lose hotloading for the simulation (properties are permanent and can't be overwritten)
  2. We lose the ability to make globals readonly (the function won't know if we are hotloading, and will have to accept redefinitions)

I'm going to make hotloadable = true by default, so SetGlobal will still be educated about whether we are currently hotloading, but some of the complexity will be removed 🙂

source/scriptinterface/ScriptInterface.h
145

I think it's more clear, but I don't intended it to be transparent, rather properly encapsulated. I wanted to provide an external interface to developers that only has useful parameters (constant, enumerable, hotloadable) instead of making them deal with the JS property attributes (in case they don't know that DontDelete prevents hotloading).

currentlyHotloading is something that the simulation passes to the script interface, whereas replace could have been abused by developers wanting to work around the fact that a global is constant (which they shouldn't). I put it as last optional parameter since devs should usually leave it to false when defining globals.

Now that you say it, we are indeed using hotloadable = true in all occurrences of the function, and I am not sure we will have a reason to make a global not hotloadable. So a code comment explaining why SetGlobal(..., constant = true, ...) doesn't add the DontDelete attribute would be enough.

  1. If I understand correctly, the SetGlobal hotloading mechanism is only relevant to the simulation, yes?
  • The GUI hotloading mechanism works by creating a new GUI page from scratch (CGUIManager::LoadPage), calling getHotloadData and passing hotloadData back to the JS init function.
  • The other hotloading mechanisms (l10n, filesystem, renderer) don't relate to JS, right?
  1. The purpose of the ScriptInterface is to build a wrapper around the JS engine, so it should ideally expose a subset of the JS engine features, no more.

The ScriptInterface should be agnostic of Simulation specifics, since it is also used for the GUI, rmgen, and arbitrary JS related code.
For example one might use the ScriptInterface in a JS context where hotloading doesn't exist as a concept, or has a different context (GUI), then the hotloadable property would not be relatable.

Why not give the ComponentManager a SetGlobal function that reads from it's own m_CurrentlyHotloading and removes and changes the JS attributes in the ScriptInterface SetGlobal call appropriately?
(The only downside to this is that people might call the ScriptInterface method rather than the ComponentManager one, but SetGlobal calls outside of the ComponentManager sound like trouble to begin with)

Another alternative would be to have two SetGlobal functions in the ScriptInterface, one that is only about JS logic, the other could have the hotloading logic.
(The downside to that is that ScriptInterface isn't agnostic of simulation specifics)

Removing ScriptInterface expression freedom might make it more simple, but also sounds like it removes freedom to use the JS engine for contexts that don't use the simulation hotloading principle or an equivalent one, no?

source/ps/GameSetup/GameSetup.cpp
187

I meant D1754 (not D1716). (So this patch doesn't add another workaround and D1844 would only make it 2 lines shorter but not better qualitatively, so D1754 isn't part of the SM roadmap)

source/scriptinterface/ScriptInterface.cpp
629

I mean Please restart the engine isn't shown either for other gamebreaking errors, it's just implied that it's pronounced dead and open to the user to spawn a new instance or continue in the broken state.

source/scriptinterface/ScriptInterface.h
145

I think it's more clear, but I don't intended it to be transparent, rather properly encapsulated.

Being transparent and properly encapsulated aren't mutually exclusive.
(We can have two variants of the function, one that exposes the JS attributes as arguments, one educated about hotloading that calls the other one for example. Sounds both more transparent and encapsulated simultaneously?)

Itms added a comment.Apr 27 2019, 6:28 PM
In D1844#76445, @elexis wrote:
  1. If I understand correctly, the SetGlobal hotloading mechanism is only relevant to the simulation, yes?

Yes.

  1. The purpose of the ScriptInterface is to build a wrapper around the JS engine, so it should ideally expose a subset of the JS engine features, no more.

Yes, no more than what is needed by the caller.

The ScriptInterface should be agnostic of Simulation specifics, since it is also used for the GUI, rmgen, and arbitrary JS related code.
For example one might use the ScriptInterface in a JS context where hotloading doesn't exist as a concept, or has a different context (GUI), then the hotloadable property would not be relatable.
Why not give the ComponentManager a SetGlobal function that reads from it's own m_CurrentlyHotloading and removes and changes the JS attributes in the ScriptInterface SetGlobal call appropriately?
(The only downside to this is that people might call the ScriptInterface method rather than the ComponentManager one, but SetGlobal calls outside of the ComponentManager sound like trouble to begin with)

That sounds like a good idea, but I'm not sure I can do that without it becoming ugly. Indeed, currently the script interface exposes functions that take a boolean constant parameter. It means DontDelete+ReadOnly (in ECMA vocabulary). This patch adds some difference between global properties and object properties, by making constant only ReadOnly for the former. But the API of the script interface doesn't change, which is nice. Following your idea would make me change the API and a lot of function calls for little benefit. Maybe I can write the patch and upload it as a paste to decide on it, unless you see what I mean and agree it's not nice.

So with respect to your point 2. above, I think exposing all property attributes is unnecessary, exposing constant-ness is enough and allows devs to not worry about hotloading issues, they only have to tell the script interface whether they are currently hotloading.

Another alternative would be to have two SetGlobal functions in the ScriptInterface, one that is only about JS logic, the other could have the hotloading logic.
(The downside to that is that ScriptInterface isn't agnostic of simulation specifics)
Removing ScriptInterface expression freedom might make it more simple, but also sounds like it removes freedom to use the JS engine for contexts that don't use the simulation hotloading principle or an equivalent one, no?

The other downside is that the first function would only be called by the second one, so it's not very useful. Currently the code I'm touching is only used by the simulation, but if we need that freedom in the future it will be easy to add this new function without hotloading logic that you are mentioning.

Thanks again for the thoughtful input.

source/ps/GameSetup/GameSetup.cpp
187

Ah yes. Indeed, I can either commit this patch which "upgrades" the workaround before D1754; or if D1754 is in, this patch will be a bit shorter because there will be no call to SetGlobal to upgrade anymore. D1754 doesn't block the SM upgrade (but it's a good opportunity to commit it because it's nice).

exposing all property attributes is unnecessary, exposing constant-ness is enough

Agree, not necessarily all JS attributes (https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_reference/Property_attributes),
But those that are exposed determine (Java)ScriptInterface behavior, don't relate to ComponentManager behavior.

allows devs to not worry about hotloading issues, they only have to tell the script interface whether they are currently hotloading.

But hotloading in the simulation context means something different in the GUI context. In the GUI context one can be hotloading currently but it doesn't matter whether hotloadable is set to true or false?
And some caller other than the ComponentManager may need to replace somehow constant globals without being in hotloading context.
To me it seems like a https://en.wikipedia.org/wiki/Separation_of_concerns issue if the hotloading isn't shown to be (1) universally applicable to any hotloading mechanism and (2) no other mechanism wants to trigger the code flow that hotloadable triggers.

The other downside is that the first function would only be called by the second one, so it's not very useful.

The first function would be useful for the generic JS engine developer.
The second function would be useful for the ComponentManager developer that has to worry about the ComponentManagers hotloading mechanism.
But the variant could also be defined in the ComponentManager.

Renaming the arguments is also considerable, for example hotloadable -> overrideable, currentlyHotloading -> currentlyOverriding, but not convinced either.

I'm going to make hotloadable = true by default, so SetGlobal will still be educated about whether we are currently hotloading, but some of the complexity will be removed 🙂

The default might be wrong for callers other than the ComponentManager, so the caller should be know well whether he should pass true or false if he wants the code to perform as expected. If he should know well, then it's better to make him face the decision whether he wants true or false, right?

Looking at the code and specs (https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_reference/Property_attributes
) again,

constant = JSPROP_READONLY = "The property's value cannot be set"
hotloadable = JSPROP_PERMANENT = "The property cannot be deleted"

Why not keep the arguments so straight-forward?

In order to understand what hotloadable and currentlyHotloading will do to a callers defined global, one doesn't have much choice other than to lookup what READONLY, PERMANENT do depending on the two proposed arguments.
So why not remove the indirection and instead educate about what READONLY and PERMANENT mean to the general (Java)ScriptInterface caller?

(At least that's how it looks to me, but I have incomplete knowledge as I didn't try to implement alternatives)

source/scriptinterface/ScriptInterface.cpp
623

The error message is omissive / misleading, it's possible to call SetGlobal on a variable multiple times, it should only not be possible for PERSISTENT ones. It should actually be possible for READONLY ones, no? (Which would mean currentlyHotloading not being supposed to be this function?)

646

If a property with that name is already defined, if that property is PERSISTENT but not READONLY, then this call will return an error. (Probably not bad, but why are the other cases captured if not this one as well.)

647

If readonly, attrs |= JSPROP_READONLY
If permanent, attrs |= JSPROP_PERMANENT?
(It's daring but one may consider getting attrsas an argument and posting a link to the specs?)

Itms added a comment.Apr 27 2019, 8:34 PM
In D1844#76466, @elexis wrote:

But hotloading in the simulation context means something different in the GUI context. In the GUI context one can be hotloading currently but it doesn't matter whether hotloadable is set to true or false?
And some caller other than the ComponentManager may need to replace somehow constant globals without being in hotloading context.
To me it seems like a https://en.wikipedia.org/wiki/Separation_of_concerns issue if the hotloading isn't shown to be (1) universally applicable to any hotloading mechanism and (2) no other mechanism wants to trigger the code flow that hotloadable triggers.

I see your point. I'll write the alternative patch to be able to decide between separation of concerns and the simplicity of the code.

Looking at the code and specs (https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_reference/Property_attributes
) again,
constant = JSPROP_READONLY = "The property's value cannot be set"
hotloadable = JSPROP_PERMANENT = "The property cannot be deleted"
Why not keep the arguments so straight-forward?

That's because this is only true for attributes of the global object. Permanent properties of children objects (like this.template in scripted components) can be hotloaded, since the parent property (the component) will be replaced by a new value with its own permanent properties. So hotloadable only makes sense for SetGlobal.

Writing more explicit comments about what are the different property attributes would be good regardless.

source/scriptinterface/ScriptInterface.cpp
623

No, it's not possible either for ReadOnly ones. SpiderMonkey is consistent with JavaScript, so JS_DefineProperty will do the same as JS_SetProperty when the property is already existing (and additionally it will set attributes, getters and setters) and will fail if the existing property is ReadOnly.

Itms updated this revision to Diff 7878.Apr 27 2019, 11:47 PM
Itms edited the summary of this revision. (Show Details)

This is the version of the patch where I make all globals hotloadable. I also reverted currentlyHotloading to replace in order to make the script interface agnostic of the hotloading concept.

Extra tests were moved to D1850.

Since we were clarifying vocabulary elements, I renamed some parameters that were confusingly wrongly named.

Owners added a subscriber: Restricted Owners Package.Apr 27 2019, 11:47 PM

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

Link to build: https://jenkins.wildfiregames.com/job/differential/1302/display/redirect

One thing I'm curious about: there is no way for C++ code to delete a "PERMANENT" property? That seems dubious.

Looking at properties, it appears SM38 has "IGNORE_PERMANENT" attributes: https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_Reference/Property_attributes

This seems like the kind of thing we should use here, and we can still make these globals permanent.

source/simulation2/system/ComponentManager.cpp
149

TBH this feels like it could be committed separately, but not a huge problem.

Itms added a comment.May 19 2019, 11:00 AM

One thing I'm curious about: there is no way for C++ code to delete a "PERMANENT" property? That seems dubious.
Looking at properties, it appears SM38 has "IGNORE_PERMANENT" attributes: https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_Reference/Property_attributes
This seems like the kind of thing we should use here, and we can still make these globals permanent.

I'm happy because you have the exact same ideas I had so I'm not alone 😄

I asked on the SM IRC channel about the possibility for the engine to remove the DontDelete property: it's unadvisable because it would break some JIT assumptions. In the worst case we could create segfaults, I actually just expect us to not benefit from some performance improvements to the JIT if we ignore that property. See https://mozilla.logbot.info/jsapi/20190324#c16126429

I suggest you take a closer look at all the code involved with the ignore_permanent hack. It actually had a very specific use case which is far less general than the name of the property suggests, so we can't use it for our situation. Additionally, it was removed before SM45 (which allowed them to increase strictness over DontDelete, probably improving the performance of the JIT, and creating the issues with our code that I'm fixing here), so we can't use it.

wraitii accepted this revision.May 19 2019, 11:40 AM

TBH it appears you've done the work, and looking at SM code is a rabbit hole I'd rather not go back in too much :p

This looks fine to me as is.

This revision is now accepted and ready to land.May 19 2019, 11:40 AM
Itms added a comment.May 19 2019, 11:46 AM

If you have five minutes I really think you could be interested in that code, especially the comments in c44465f2a483, and how it actually not ignores the permanent property in general but only in a very specific case.

Thanks for reviewing 🙂