[PATCH] Upgrade SpiderMonkey to v24

[Yves]

Tickets related to the Spidermonkey upgrade:

#2126
#2137
#2140
#2172
#2212
#2213
#2241
#2267
#2322
#2348
#2372
#2416
#2434
#2442

Related bugs in Spidermonkey:

Meta-Bug for performance bugs affecting 0 A.D.: ​Bug 897962

Overview

We are currently using Spidermonkey 1.8.5. That's a very old version and we need to upgrade to a newer version for various reasons. Unfortunately that's not very easy because the API changes quite often.

Also check the forum thread for the current status.

Reasons for upgrading:

• Switching earlier avoids writing too much code for the older API that has to be changed later
• Only when using a supported version it's possible to fix Spidermonkey security issues (I know that 0 A.D. itself most likely has more security issues at the moment)
• Learning how the new API works with multi-threading is important for our AI multi-threading design.
• Performance: During the work on upgrading we figured out that performance currently isn't much better in the new version compared to v1.8.5. The huge improvements from benchmarks don't translate to most real world applications. Loading random maps is a lot faster, but that's the only thing at the moment. It's still possible that the performance will improve once the most important bugs in Ion Monkey and Baseline are fixed.
• If we find bugs in v1.8.5 we will never get support from Mozilla. If we use the current version or if the same bug also affects the current version, getting help is much more likely.
• Getting new features into the official version of Spidermonkey is also only possible when we use the current version.
• Many Linux distributions will use the new ESR versions of the standalone library. It's better to use that library provided by the system instead of bundling our own version. v1.8.5 won't be supported by Linux distributions forever.

Version

Mozilla plans to release standalone versions of Spidermonkey for each ESR-release. We are going to upgrade to ESR24 first.

• mozjs-24 release discussion: ​mozilla bug 919277

JIT

The Just In Time compiler creates optimized bytecode or even native assembler code for functions or loops that are called often. That's something different than the compiling which translates the scripts to bytecode in the first step.

It's currently not possible to JIT-compile the scripts before they are executed or even store them in JIT-compiled form on the disc.

Using js::NotifyAnimationActivity it's possible to prevent the garbage collection from collecting generated JIT-code (and forcing recompilation). Unfortunately this only works if js::NotifyAnimationActivity is called at least once a second. With big lag-spikes, especially in debug mode, this can't be guaranteed at the moment. At the moment I'm working around this by modifying Spidermonkey's Runtime.cpp directly. I'll have to find a better solution.

Garbage collection

Garbage collection and memory allocation is a major bottleneck for certain parts of JS code that update a lot of data very often. I made some measurements here: http://www.wildfiregames.com/forum/index.php?showtopic=16721&#entry262183

Garbage collection could be improved by improving the way we handle compartments, especially related to the AI. That's a bit vague but I'm still trying to figure out more about that. An interesting article: ​http://andreasgal.com/2010/10/13/compartments/ . That article is quite old, but I think the basic concepts explained there are still valid.

Garbage collection can now even be done in a separate thread or we could run it more often (each frame for example) to avoid lag-spikes when it happens. For the AI that problem would probably be solved anyway when we move it to a separate thread.

[Yves]

I asked about the JIT topic and Ion-Monkey in the jsapi IRC channel. Logs ​here.

Summary: JIT compiled code can't be saved on the harddisk and used later because it contains memory addresses. However, a function should be compiled with Ion-Monkey when it has been run 1000 times, so everything that runs once per frame should be compiled after about 30 seconds with 30-35 fps, which should be fine for the moment.

Garbage collection occasionally throws away the JIT-ed code. To make sure this doesn't happen, calling "js::NotifyAnimationActivity" should be enough. Check ​Bug 753630 for more background information about that.

[Yves]

just for reference and discussion in the associate thread

[Yves]

just for reference and discussion in the associate thread

[ben]

In 13429:

Extends binary serializer to support some standard JS classes: Number, String, Boolean. Fixes #406.
Extends binary serializer to support typed arrays.
Extends binary serializer to support custom JS prototype objects in AIs, fixes #407.
Allows full serialization of AIs (not yet implemented). Refs #1089, #1886
Increases binary serializer script backref arena from 8 MB to 16 MB, refs #1842

[historic_bruno]

r13429 ref'd this ticket because I had to use some hacks to get around AIs sharing data between script contexts. Actually the solution could be much simpler if each AI's data was in a single script context (then ScriptInterface could be used to hold the serializable prototypes, and the implementation wouldn't be AI specific).

Also a heads up since probably these changes will need merging into your local repo, feel free to ask if anything causes trouble.

[Yves]

In 13826:

Changes GameView to expose global functions to scripts instead of using CJSObject.
Fixes #2126
Refs #1886

[Yves]

In 13877:

Changes the Renderer to expose global functions to scripts instead of using CJSObject.
Fixes #2137
Refs #1886

[Yves]

In 13884:

Changes the Console to expose global functions to scripts instead of properties.
Fixes #2140
Refs #1886

[Yves]

In 13914:

Changes the ConfigDB to expose global functions to scripts instead of properties and custom objects.
Fixes #2172
Refs #1886

[Yves]

In 14026:

Remove ScriptableObject/CJSObject

Since all remaining uses of ScriptableObject/CJSObject have been removed, the related files can now also be removed.
Closes #2212
Refs #2126
Refs #2137
Refs #1886

[Yves]

In 14036:

Unify script conversions and remove JSInterface_Vector3D.

Because it was historically grown, we have some duplicated code for converting script types to native types.
This patch removes the file JSConversions.cpp and moves some code to ScriptConversions.cpp.
The places using JSConversions.cpp are changed to use the ScriptInterface's conversion functions in ScriptConversions.cpp.
I also removed JSInterface_Vector3D because it had additional requirements to the conversion code that no other code has and because it's currently not used. I think it doesn't make sense to maintain code just because it could possibly be used again later.

Closes #2213
Refs #1886

[Yves]

In 14199:

In #2241 I'm going to change the GUI to have one ScriptInterface for each GUI page because that will be required for the Spidermonkey upgrade (#1886).
The Multiplayer lobby needs some changes to avoid compartment mismatches. Instead of initializing it with a ScriptInterface and storing script values at different locations, it takes a ScriptInterface argument in the functions that really need to read or write some script values and avoids storing values as script values with an associated compartment where possible.
The scripting interface of the lobby is also adjusted to use JSInterface_Lobby.h/cpp files as other components instead of adding all functions to ScriptFunctions.cpp. This makes it a bit more clearly arranged IMO.

Fixes #2267
Refs #2241
Refs #1886

[Yves]

In 14441:

Moves AI players to one global using the module pattern.
This avoids wrapping overhead that would otherwise be required because multiple globals per compartment aren't supported anymore in newer versions of SpiderMonkey.

Check the ticket for a detailed explanation.

Closes #2322
Refs #2241
Refs #1886

[Yves]

In 14496:

Removes g_ScriptingHost and implements global to compartment 1 to 1 relation.
Each GUI Page gets its own compartment and all ScriptInterfaces in the same thread should now use the same JS Runtime.
This is required for the SpiderMonkey upgrade.
Check the ticket for details.

Closes #2241
Refs #1886
Refs #1966

[Yves]

In 14506:

Disables the JS debugger.
It's completely broken since r14496 and will have to be updated for the new SpiderMonkey API.
I only uncomment it at the moment because I plan to fix/reimplement it soon after the upgrade.

Closes #2348
Refs #2241
Refs #1886

[Yves]

Current patch with SpiderMonkey version 145669:aa9ec17cf912 (​http://hg.mozilla.org/mozilla-central) and 0 A.D. version r14528 . This is still heavily WIP. There's a description how to apply it here: http://www.wildfiregames.com/forum/index.php?showtopic=17289&st=80#entry271330

[Yves]

Command to find missing var statements (probably not perfect but it worked and I need a place to write it down): grep --perl-regexp -R '(for\s?\()(?!var|\svar)' --exclude=\*{~,svn-base}

[Yves]

The current WIP patch for ESR24 (download it here and copy it to libraries/source/spidermonkey: ​https://ftp.mozilla.org/pub/mozilla.org/js/mozjs-24.2.0.tar.bz2)

[fabio]

A couple of suggestions:

• rather than hardcoding -j5 you could use -jnproc (nproc inside back ticks which the trac is omitting here), and fall back to 2 if nproc is not available;
• the build script could do a wget for mozjs24 tar.gz, rather than having it in the svn, reducing svn size and making easy to upgrade to, example, a .0.1 version.

[sanderd17]

Replying to fabio:

• the build script could do a wget for mozjs24 tar.gz, rather than having it in the svn, reducing svn size and making easy to upgrade to, example, a .0.1 version.

I don't agree with this. I'd like to be able to build the game when being offline too.

[Yves]

Current WIP patch for ESR24. This version is with incremental GC. It's still WIP and there are still some known errors.

[Yves]

@Fabio Thanks for the input. I'll try to set the -j option dynamically.

I agree with Sander and would rather bundle it at the moment. I remember some discussions about scripts for getting platform-specific dependencies. It could be worth developing something, but it should be a general solution for all libraries and dependencies.

Btw. sorry for answering late. E-Mail notifications still don't work for me and I didn't notice your comment in the logs.

[leper]

Replying to fabio:

A couple of suggestions:

• rather than hardcoding -j5 you could use -jnproc (nproc inside back ticks which the trac is omitting here), and fall back to 2 if nproc is not available;

Just pass -jsomething to update-workspaces.sh and the spidermonkey build.sh script will use the same value. (I'd probably leave the default at -j2 though, or change it to -j1)

[Yves]

In 14724:

Removes ValueCloner.

Fixes #2416
Refs #2415
Refs #1886

[Yves]

In 14733:

Reverts r14724. Structured clones don't support cyclic references in SpiderMonkey v1.8.5.
SpiderMonkey v24 supports it and I'll add this change to #1886 instead.

Refs #2416
Refs #2415
Refs #1886

[Yves]

Tested on r14775. Remaining todos: Test on Windows, Test on Mac OS X, Fix related tickets linked in the description, Review.

[fabio]

Could you enable building it against system mozjs24? I saw the comment on the patch, in the meantime Ubuntu has it: ​http://packages.ubuntu.com/source/trusty/mozjs24 Debian is on its way: ​https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=739132

Thanks.

[Yves]

Replying to fabio:

Could you enable building it against system mozjs24?

Good to know that the packages are now available! I will have a look when I consider the rest of the patch ready for review (it's a topic that can be worked on indepently of the rest).

Btw. build.sh now uses the -j argument passed to update-workspaces.sh or -j2 which is the default value coming from update-workspaces.sh.

[Yves]

In 14783:

Removes the ScriptInterface and SpiderMonkey dependencies from Atlas.
Replaces the code for JSON parsing and writing with an implementation using the JSON Spirit library.

Fixes #2434
Refs #1886

[Yves]

I've started a branch on Github: ​https://github.com/Yves-G/0ad It now contains pre-built binaries for SpiderMonkey and builds on Windows. You should be able to build it as usual without any additional steps.

There are a couple of things I still have to test or have a look at, so I'm not yet marking this ticket for the final review. If you want to test it and have feedback, that's still welcome of course! :)

EDIT: You need to install libnspr-dev on Linux

[Yves]

It's ready for review now! Get the most recent version from ​my github repository.

My plan is to commit the SpiderMonkey upgrade in about a week and maybe earlier if I get enough positive feedback. Please tell me about all issues you find when doing the review but also make a clear statement if you think they can be solved after committing or have to be solved before the patch goes into SVN. It's important to get some testing and for that it would be important to commit soon to have enough time until the alpha release for the remaining bugs to pop up.

These are the known issues I'm aware of (none of these is a commit-blocker IMO):

• The new warning messages (#2372). That's just because SpiderMonkey is more strict now when the option JSOPTION_EXTRA_WARNINGS is enabled. If we can't fix these warnings until release, we can always just disable them. It's actually a feature, not a bug! ;)
• I think historic_bruno said the Mac build still needs more testing (but it works on different systems he tested it on).
• The JS Runtime size is hardcoded at different places and the incremental GC implementation also sets hardcoded values that depend on the runtime size.
• I'm not sure if the implementation of the Tick() function works as it should. To prevent garbage collection of JIT code there's a function that has to be called at least once a second. At least in debug builds that's not guaranteed and it causes performance issues (but these are included in the measurements I've made so far). There are also some testing functions to prevent GC of JIT code without this time-based approach, but they're meant just for testing. I'll check that again in later versions of SpiderMonkey and open a bug if it's not yet solved.
• Here are the latest performance comparisons. Unfortunately SpiderMonkey 24 is slower than v1.8.5. I think it's an acceptable slowdown and later versions already get closer to v1.8.5. I should probably make some new measurements now that it's so close to completion... but I don't expect a big difference to the previous measurements.
• Needs to be tested on BSD systems (if you use such a system, test it please!).
• Proper stack rooting is still a work in progress. I don't use these types consistently. That's quite a big project for the next upgrade.
• The compiler warnings "warning: "_MSC_VER" is not defined [-Wundef]" are actually disabled but there's a ​GCC bug affecting multiple versions and they are still printed.

[Yves]

The build with --with-system-mozjs24 is now implemented and work on the beta of Ubuntu 14.04. I've removed this from the list of known issues.

[Yves]

profile comparing v24 and v1.8.5 (2vs2 non-visual replay - v24 is slower)

[Yves]

I've measured the performance once again (graph is attached). This time I measured on Windows. As expected it didn't change since the previous measurement. V24 is still a bit slower, which comes mainly from the AI scripts. We've seen that future versions come closer to v1.8.5 performance, hopefully beyond that at some point.

[leper]

Some stuff I found: uint32_t, uint, u32 and unsigned are used, even mixing all of those in single files which is a bit strange to read. Some whitespace issues adding tabs in empty lines, removing tabs in empty lines indenting with spaces, ...

Specific stuff:

spidermonkey/build.sh absolute paths for win32

• remove the openbsd block for changing the suffix completely or not change it at all (I suspect it will need the unchanged block and if not we can still change it. I plan to test it there before release)

graphics/MapGenerator.cpp

• typo in TODO ScirptInterface -> ScriptInterface (CMapGeneratorWorker::Initialize)

gui/IGUIObject.cpp

• some tabs in otherwise blank lines

gui/scripting/GuiScriptConversions.cpp (also scriptinterface/ScriptConversions.cpp (ToJSVal<T>))

• document why we are passing ret values as refs (I remember crashes with returning stuff being discussed on IRC, could be worth noting that somewhere)

gui/scripting/JSInterface_GUITypes.cpp

• remove the "TODO: ugly with so much repetition" (using defines makes it more ugly, making an if for 8 and 4 to init let top right and bottom doesn't help with making it readable either) (or using an if for 4 and the else for r{left,top,right,bottom})
• JSI_GUIMouse::toString() why not just int instead of int32_t (we use uint elsewhere (in this patch) and not uint32_t)

gui/scripting/JSInterface_IGUIObject.cpp

• in getProperty(): the #define P(x) could span a few more lines; as it is now it hides that something happens after the if

lib/types.h

• any reason for the define -> typedef change?

ps/Profile.cpp

• Why is that being removed? If we remove it, why keep the code that depends on setting that. Though I don't see any obvious reason why it should conflict (if it conflicts with SpiderMonkey in some way we should just make sure that both aren't set at the same time)

ps/{GameSetup.cpp,Replay.cpp}

• should we use a constant for the runtime size, or rely on both being changed in lockstep? (Already in your known issues, but it still caught my eye)

scriptinterface/ScriptExtraHeaders.h

• the reenabling of the diagnostic pragmas for gcc>=4.6 isn't needed as the pragma GCC diagnostic pop takes care of that

scriptinterface/ScriptInterface.cpp

• comment in GCSliceCallbackHook() has spaces for everything but the second line
• some of the code ("else if ([...])" has spaces instead of tabs
• make the code in there for printing some debug info be in an ifdef?
• use a constant/define for the runtime and gc size?
• unsigned around line 360 (but uint elsewhere in the file)
• Math_random() early return in the if, no need for the else
• CallConstructor() usless parens at that if
• FreezeObject() why is the ? true : false there at all?
• ParseJSON() check the return value? vp is undefined if it fails

scriptinterface/ScriptInterface.h

• Tick, MaybeIncrementalRuntimeGC, ForceGC and MathRandom could use a comment

scriptinterface/ScriptTypes.h

• use #pragma GCC diagnostic push/pop (for >=4.6)
• why is "-Wnon-virtual-dtor" commented out when re-enabling stuff?

scriptinterface/ScriptVal.cpp

• structUnrooter spaces instead of tabs before closing } of operator()

test_ComponentManager.h

• might be nice to create a ticket for that TODO (around line 705) at some point
• indent the string at line 720 (and use \n to still keep it readable)

[Yves]

Issues fixed / questions answered

Replying to leper:

uint32_t, uint, u32 and unsigned are used, even mixing all of those in single files which is a bit strange to read.

gui/scripting/JSInterface_GUITypes.cpp

• JSI_GUIMouse::toString() why not just int instead of int32_t (we use uint elsewhere (in this patch) and not uint32_t)

scriptinterface/ScriptInterface.cpp

• unsigned around line 360 (but uint elsewhere in the file)

I've committed a consistency improvement for integer types ​here.

Some whitespace issues adding tabs in empty lines, removing tabs in empty lines indenting with spaces,

graphics/MapGenerator.cpp

• typo in TODO ScirptInterface -> ScriptInterface (CMapGeneratorWorker::Initialize)

gui/IGUIObject.cpp

• some tabs in otherwise blank lines

gui/scripting/JSInterface_GUITypes.cpp

• remove the "TODO: ugly with so much repetition" (using defines makes it more ugly, making an if for 8 and 4 to init let top right and bottom doesn't help with making it readable either) (or using an if for 4 and the else for r{left,top,right,bottom})

gui/scripting/JSInterface_IGUIObject.cpp

• in getProperty(): the #define P(x) could span a few more lines; as it is now it hides that something happens after the if

scriptinterface/ScriptInterface.cpp

• comment in GCSliceCallbackHook() has spaces for everything but the second line
• some of the code ("else if ([...])" has spaces instead of tabs
• make the code in there for printing some debug info be in an ifdef?
• Math_random() early return in the if, no need for the else
• CallConstructor() usless parens at that if
• FreezeObject() why is the ? true : false there at all?
• ParseJSON() check the return value? vp is undefined if it fails

scriptinterface/ScriptVal.cpp

• structUnrooter spaces instead of tabs before closing } of operator()

Fixed ​here.

spidermonkey/build.sh

• absolute paths for win32
• remove the openbsd block for changing the suffix completely or not change it at all (I suspect it will need the unchanged block and if not we can still change it. I plan to test it there before release)

It didn't work with relative paths. I think there were some different places that expected the paths to be relative to different locations, but I don't remember exactly. I've documented it along with the BSD change and some other things in ​this commit.

lib/types.h

• any reason for the define -> typedef change?

SpiderMonkey does this in Value.h, which doesn't work with defines:

union {
    int32_t        i32;
    uint32_t       u32;
    JSWhyMagic     why;
} payload;

ps/Profile.cpp

• Why is that being removed? If we remove it, why keep the code that depends on setting that. Though I don't see any obvious reason why it should conflict (if it conflicts with SpiderMonkey in some way we should just make sure that both aren't set at the same time)

I uncommented that block because of a crash and wanted to have a look later. It should now be solved good enough (meaning not worse than before) in r14874.

gui/scripting/GuiScriptConversions.cpp (also scriptinterface/ScriptConversions.cpp (ToJSVal<T>))

• document why we are passing ret values as refs (I remember crashes with returning stuff being discussed on IRC, could be worth noting that somewhere)

Documentation added ​here.

scriptinterface/ScriptTypes.h

• use #pragma GCC diagnostic push/pop (for >=4.6)
• why is "-Wnon-virtual-dtor" commented out when re-enabling stuff?

scriptinterface/ScriptExtraHeaders.h

• the reenabling of the diagnostic pragmas for gcc>=4.6 isn't needed as the pragma GCC diagnostic pop takes care of that

Fixed ​here.

scriptinterface/ScriptInterface.h

• Tick, MaybeIncrementalRuntimeGC, ForceGC and MathRandom could use a comment

I've removed Tick ​here because measurements didn't show a clear performance difference. I've added comments to the other functions in ScriptInterface.h ​here.

Still open

ps/{GameSetup.cpp,Replay.cpp}

• should we use a constant for the runtime size, or rely on both being changed in lockstep? (Already in your known issues, but it still caught my eye)

scriptinterface/ScriptInterface.cpp

• use a constant/define for the runtime and gc size?

test_ComponentManager.h

• might be nice to create a ticket for that TODO (around line 705) at some point
• indent the string at line 720 (and use \n to still keep it readable)

... plus some others I've mentioned before.

[Yves]

In r14876 (forgot to ref the ticke):

First commit for the SpiderMonkey upgrade (new binaries and headers).

Adds the precompiled Windows binaries, pdb files, the source tarball and the header files for Windows (both release and debug).

[Yves]

In 14877:

Second (main) commit for the SpiderMonkey upgrade.

This commit contains all the required changes to our source files and build scripts (hopefully).
A next commit will remove the old stuff of SpiderMonkey 1.8.5.

Spcial thanks to:

• H4writer who helped a lot mainly with the performance issues we had/have, but also with other problems or questions.
• Leper for the review.
• Historic_bruno for implementing the build scripts on Mac OS X and testing on the Mac.
• The people from the #jsapi channel and from mozilla.dev.tech.js-engine who answered a lot of questions and helped solving problems.
• All the other people who helped

Refs #1886
Fixes #2442
Fixes #2416

[Yves]

In 14878:

Third commit for the SpiderMonkey upgrade (remove v1.8.5).

This commit removes the old binaries and header files of SpiderMonkey 1.8.5 plus two patches for openbsd.
I'm closing the SpiderMonkey upgrade ticket because most of the work is done.
Of course I'll continue to work on the remaining known issues (check the ticket for a list).

Fixes #1886
Refs #2415

[fabio]

This file has a couple of TODO related to mozjs24 update: ​http://trac.wildfiregames.com/browser/ps/trunk/binaries/data/mods/public/gui/common/colorFades.js

[Yves]

In 14889:

Workaround for GCC bug to disable compiler warning

Only works when using the bundled SpiderMonkey and not when using --with-system-mozjs24 (because it patches the bundled headers).
ESR31 doesn't have this issue anymore, so this workaround should be enough until then.

Refs #1886

[Yves]

In 15495:

Revert obsolete workaround for structured clones.

Reverts r14573 because that workaround isn't needed anymore with SpiderMonkey 24 (and 31).

Refs #2241
Refs #1886

[Yves]

In 15496:

SpiderMonkey now supports default parameters!

I've just modified those places with TODO comments, but there are most likely others that could benefit of the feature to make the code a bit cleaner and easier to understand.
Thanks to Fabio for pointing it out.
Refs #1886