#2241 closed task (fixed)
[PATCH] Remove g_ScriptingHost and implement global to compartment 1 to 1 relation
Reported by: | Yves | Owned by: | Yves |
---|---|---|---|
Priority: | Should Have | Milestone: | Alpha 16 |
Component: | Core engine | Keywords: | Spidermonkey |
Cc: | Patch: |
Description
Here's the description copied from the Spidermonkey upgrade forum thread:
Each GUI page in its own compartment
The first major change is having each GUI Page (CGUI Object) in its own compartment using its own ScriptInterface instance. Currently the GUI uses two global objects. One is a page global for each GUI page which should separate global variables between the pages. The other global is used for all pages and contains engine function definitions and such. The new Spidermonkey associates a compartment with each global object and this design doesn't work anymore. At least I didn't find a way to make it work with minor changes. Cross compartment calls and wrapper objects could provide a solution here but I decided the other approach looks more promising. At the moment the plan is to register all GUI functions for all pages. I don't think registering maybe 100 functions for each GUI page should be a performance problem or memory problem. If it becomes a problem we can still implement something that lets GUI pages include functions they need.
Getting rid of g_ScriptingHost
We are still using the old ScriptingHost that had been created before ScriptInterface was developed. That's unneeded duplication and should be replaced by ScriptInterface. It's sometimes a bit tricky because a lot of code simply accesses this global object and some thoughts are required that this code uses the right context/ScriptInterface/compartment in the future.
I'm currently working on a patch but it's not quite ready yet.
Attachments (13)
Change History (48)
comment:1 by , 11 years ago
comment:2 by , 11 years ago
- Structured clones don't work because there's recursion somewhere.
- Using the current approach (one compartment per runtime) doesn't work because there's a problem with registring the GUIObject type and its functions. It currently looks like a Spidermonkey bug but I could investigate further if I decide to spend another day or two on it with low chances of success.
- The Patch seems to have a lot of prerequisites and it would probably be too much work to create it and test it properly.
I could wait with that commit until the new version of Spidermonkey is ready to commit, but that would mean combining two very large patches and having to review one "megapatch". I would like to avoid that if possible.
by , 11 years ago
Attachment: | Patch_Spidermonkey_1.8.5_clone_wrappers_v0.1.diff added |
---|
A quick hack that fixes the cloning of objects containing wrappers.
comment:3 by , 11 years ago
This is the official patch that fixed this problem: http://hg.mozilla.org/mozilla-central/rev/1fb77c1bb742
Unfortunately it relies on other patches and can't be easily applied. I've attached my own workaround for this problem. My solution works for us, but it probably doesn't work correctly with principals. On the other hand, this case shouldn't affect any other users of the library anyway because cloning of objects containing wrappers doesn't work at all at the moment.
comment:6 by , 10 years ago
Keywords: | review added |
---|---|
Milestone: | Backlog → Alpha 15 |
Summary: | Remove g_ScriptingHost and implement global to compartment 1 to 1 relation → [PATCH] Remove g_ScriptingHost and implement global to compartment 1 to 1 relation |
Here's the first version of the patch ready for tests and review. It will need a lot of testing and careful review! ;) There are most likely some issues remaining, but these should be small enough for a review to make sense (especially if something bigger is brought up during the review). I will continue my own tests, but help with testing is very welcome!
How to apply
- Apply FixSpidermonkeyWrappers_v1.0.diff to solve the wrapping issue of Spidermonkey 1.8.5
- Apply RemoveScriptingHost_oneCompartmentPerGUIPage_[VERSION].diff
- Run clean-workspaces and update-workspaces and DON'T use --with-system-mozjs185
The patch is tested for rev 14197.
Known issues
- I have to take a closer look at some warnings in debug mode: [debug]JS engine warning: leaking GC root 'CScriptValRooted' at 0xa9b4d50
by , 10 years ago
Attachment: | RemoveScriptingHost_OneCompartmentPerGUIPage_v1.0.diff added |
---|
by , 10 years ago
Attachment: | FixSpidermonkeyWrappers_v1.0.diff added |
---|
comment:7 by , 10 years ago
I get a crash when building with --without-lobby which should be due to this commit
Linking pyrogenesis ../../../binaries/system/libgui.a(ScriptFunctions.o): dans la fonction « call<bool (*)(void*)> »: /build/workspaces/gcc/../../../source/scriptinterface/NativeWrapperDefns.h:33: référence indéfinie vers « JSI_Lobby::HasXmppClient(void*) » collect2: error: ld returned 1 exit status make[1]: *** [../../../binaries/system/pyrogenesis] Erreur 1 make: *** [pyrogenesis] Erreur 2
comment:9 by , 10 years ago
Version 1.1 of the patch fixes the leaking GC root issue and a renames "fileExists" to "Engine.FileExists" in lobby.js.
Some additional things to do about the Spidermonkey wrapping issue (thanks to Philip for the input):
- Check if the buggy functions could be copied to our code to avoid compatibility problems with Spidermonkey 1.8.5 packages (probably difficult, but I will try that).
- Otherwise try to detect a wrong version of the library at build time. Some defines could be added to jsversion.h and checked in the engine code for example.
- If repackaging of is needed, contact packages maintainers.
by , 10 years ago
Attachment: | RemoveScriptingHost_OneCompartmentPerGUIPage_v1.1.diff added |
---|
comment:10 by , 10 years ago
I've tried to copy the modified functions into our code (point 1). Unfortunately I came to the conclusion that this is not going to work. This code depends on way too much other code that is not in the public API.
Now I've modified the patch a little bit to inform package maintainers and people who compile from source by causing a compile error that should point them to this ticket (point 2).
I think I will wait until after the release of Alpha 15. I hope the planned release date (5th of December) will be kept. After that I can start contacting package maintainers (point 3).
by , 10 years ago
Attachment: | FixSpidermonkeyWrappers_v1.1.diff added |
---|
This version adds an error at compile-time to inform about the need to patch Spidermonkey
comment:11 by , 10 years ago
Milestone: | Alpha 15 → Alpha 16 |
---|
comment:13 by , 10 years ago
The hotloading problems are solved in my next version of the patch, but there are other problems with wrapping and serialization and I noticed that this patch makes the performance significantly worse. I think the problem is the wrapping and this could also be the problem with the new SpiderMonkey version. I don't mean creating the wrappers but how the wrappers affect the JIT compiler.
by , 10 years ago
Attachment: | RemoveScriptingHost_OneCompartmentPerGUIPage_v1.3.diff added |
---|
The next version with some serialization fixed
comment:15 by , 10 years ago
Keywords: | review added |
---|---|
Status: | new → assigned |
follow-up: 28 comment:16 by , 10 years ago
Some comments:
- The patch changes how messageboxes work (check the comment in functions_global_object.js)
- The fixed renderpath message now pops up after the Splashscreen is closed instead of right after it's opened. This has to be done because the callback code relies on the stack behaviour of the GUI page stack (LIFO order) to find in which page it should try to call the callback function.
- The JS console runs in the active GUI page instead of the g_ScriptingHost context (we don't have direct access to the simulation data or AI data anyway, so it shouldn't matter).
- The size in this line is quite arbitrary (as it was before too): g_ScriptRuntime = ScriptInterface::CreateRuntime(128 * 1024 * 1024);
- Tested so far: Replay mode, hotloading, serialization (full hash at the end of an AI game matches), performance (no problems anymore)
- Tested on rev r14453
by , 10 years ago
Attachment: | RemoveScriptingHost_OneCompartmentPerGUIPage_v1.5.diff added |
---|
This version works without the SpiderMonkey? patch and because of #2322 there are no performance problems anymore. Hotloading works and serialization works too.
comment:17 by , 10 years ago
Tested this morning. C++ side code seems clean, it's readable, it's even clearer in several spots at first sight, I like it, and I haven't noticed any format typo (though I haven't looked at it with extreme scrutiny). It compiles cleanly on my OS.
JS side you've missed a commented one l294 of selection_details.j You haven't changed help.txt You've missed "endgame" in CancelOnErro in functions_utility_errors.js Some search reports no other misses.
It'd need a bit of update for the latest GUI updates I assume, but nothing un-doable. I'd say we should commit this as soon as possible.
by , 10 years ago
Attachment: | RemoveScriptingHost_OneCompartmentPerGUIPage_v1.6.diff added |
---|
Addresses wraitii's input and upldates to r14461
comment:18 by , 10 years ago
I'm trying something to ease the review of this patch. Should be ready in a few hours.
by , 10 years ago
Attachment: | reviewing help.xls added |
---|
a table classifying the complexity of changes per file (should help with reviewing).
by , 10 years ago
Attachment: | RemoveScriptingHost_OneCompartmentPerGUIPage_v1.7.diff added |
---|
Some minor fixes, but it doesn't yet fix the build problem on Windows that was reported
by , 10 years ago
Attachment: | RemoveScriptingHost_OneCompartmentPerGUIPage_v1.8.diff added |
---|
This version fixes the build errors on Windows
comment:19 by , 10 years ago
Concerning the JS Debugger, I think I'll have to remove it together with this patch. Currently it crashes after closing the splash screen. I'm quite sure the reason is what's described in #1890. That problem there becomes bigger with this patch because now there's only one runtime and a lot of contexts per runtime. It would require a major redesign of the JS debugger which will be required anyway for the SpiderMonkey upgrade. It would not make sense to fix that now and then do it from scratch again after updating SpiderMonkey.
by , 10 years ago
Attachment: | RemoveScriptingHost_OneCompartmentPerGUIPage_v1.9.diff added |
---|
This version fixes saving and loading
by , 10 years ago
Attachment: | RemoveScriptingHost_OneCompartmentPerGUIPage_v1.10.diff added |
---|
Modified CGUIManager::SwitchPage to work around a crash that only happened when using --with-system-mozjs for an unknown reason. Applies cleanly with r14478.
comment:22 by , 10 years ago
Keywords: | review removed |
---|
follow-up: 31 comment:28 by , 10 years ago
Replying to Yves:
- The fixed renderpath message now pops up after the Splashscreen is closed instead of right after it's opened. This has to be done because the callback code relies on the stack behaviour of the GUI page stack (LIFO order) to find in which page it should try to call the callback function.
Does this mean multiple overlapping dialogs will break callbacks? That sounds like a bug :/
The workaround doesn't work here, I've forced fixed renderpath but the warning never appears after the splash screen. Also, it should appear before / on top of the splash screen, that's why it was pushed immediately after the splashscreen.
follow-up: 32 comment:31 by , 10 years ago
Replying to historic_bruno:
Replying to Yves:
- The fixed renderpath message now pops up after the Splashscreen is closed instead of right after it's opened. This has to be done because the callback code relies on the stack behaviour of the GUI page stack (LIFO order) to find in which page it should try to call the callback function.
Does this mean multiple overlapping dialogs will break callbacks? That sounds like a bug :/
It breaks callbacks in this case: Page A opens Page B and then (before B is closed) Page C.
It doesn't break callbacks in this case: PageC is opened by Page B which was opened by PageA.
In my opinion we shouldn't do this anyway. Definitely not for simple messageboxes because they are modal dialogs.
I haven't found any places other than the renderpath screen where this limitation is a problem in the current GUI. It could be a problem for future features like tabbed interfaces where tabs are represented with GUI pages. In this case the more central design fault is using a stack to store GUI pages. Using a stack there basically says its OK to accept the limitations of a stack.
Anyway, if this limitation is a problem and we need to find a solution, using private pointers (keywords: JSClass, JSCLASS_HAS_PRIVATE) in the GUI object to pass a pointer to the callback page could work (that's just a rough idea, I'd have to test it).
The workaround doesn't work here, I've forced fixed renderpath but the warning never appears after the splash screen. Also, it should appear before / on top of the splash screen, that's why it was pushed immediately after the splashscreen.
I must have broken it in one version of the patch. It worked at some point, I've tested it. Now it's fixed.
comment:32 by , 10 years ago
Replying to Yves:
In this case the more central design fault is using a stack to store GUI pages. Using a stack there basically says its OK to accept the limitations of a stack.
I think my main concern isn't having limitations, but knowing those limitations and not enforcing them somehow (if that's the case here). This is an obscure aspect of the GUI engine, but one that would be easy to encounter as a GUI designer who doesn't know all the internals of SM and our GUI engine.
What happens when the breakage occurs, is it possible that it won't be obvious? I just fixed a stack-related bug in r14591 because whoever wrote the GUI didn't consider dialogs, yet we have them, it's not terrible to tweak the design to make it functional.
The workaround doesn't work here, I've forced fixed renderpath but the warning never appears after the splash screen. Also, it should appear before / on top of the splash screen, that's why it was pushed immediately after the splashscreen.
I must have broken it in one version of the patch. It worked at some point, I've tested it. Now it's fixed.
Thanks!
comment:33 by , 10 years ago
I'm facing a weird bug with messageBox. I don't know if related, but could be. The problem can be reproduced using a simple messageBox(500, 200, "test", "test", 0, ["Yes", "No"], [null, null]);
If I put this instruction at different places in the code, it works as expected, except in the function loadGame() of gui/savedgames/load.js : If I put it as first line of the function, I got a JS error saying that gameSelection is undefined, and if I put it somewhere else in that function, it is just ignored.
If anybody have some hint about it, that would be welcomed !
comment:34 by , 10 years ago
The problem reported in previous comment was mainly due to my bad usage of messageBox (thanks to Yves for the help) so not related to this ticket.
I'm currently facing the problem that v1.8.5 fails to create a structured clone when the object contains wrappers. That's bad because I have to use a slightly different solution for 1.8.5 and for the new Spidermonkey in this patch. I've listed the possible approaches I see along with some pros and cons. I currently tend to the last approach.