[Discussion] Spidermonkey upgrade

[Yves]

I decided to open a thread about the Spidermonkey upgrade.

There are some design decisions to take and it's good to have a place to write down the pros and contras. This is meant for discussions to take place and for work in progress designs and ideas that aren't final yet. I intend to use ticket 1886 for documenting more "final" decisions and information.

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.

I figured out that the music code passes information across pages by using the global object called "global" (for example "global.curr_music"). A grep for "global\." returned only music related code, so it doesn't seem to be an often used pattern and could be replaced by a state stored in the music manager itself that could be accessed by a function.

Concerning the JS console, the idea is that it should always use the currently active GUI page.

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.

Standardizing scripting access

It's possible to access and use the JSAPI in a lot of different ways and we use too many of them in my opinion. That's probably because it grew that way and partially because different code has different requirements.

I don't want to do too much at the moment but after thinking about it a lot and after discussing it with Philip I think we should remove CJSObject and the associated classes. It's currently used by the CRenderer, CGameView and the Soundmanager and I'd try to apply the same new pattern also for the JS console.

The general approach would be providing global functions to scripts instead of custom C++ objects with GetProperty and SetProperty overloads, custom properties and custom methods. That's more or less the same approach Philip has chosen for the Simulation code and it seems to work quite well there.

Object oriented design is good, but I think it's enough to have an object oriented scripting language and an object oriented engine. The interface between JS and the engine benefits more from a clean separation than from OOP. If there are global functions that can be called like "Engine.DoWhatEver()" it's clear that it is an engine function. Properties on the other hand can be script properties or native properties with custom getter- and setter functions that do potentially unexpected things. It's not obvious when using an object if it's a JS object or a C++ object with some custom functionality.

The deprecated interface itself (CJSObject) required some quite hacky bits of code to create a relatively smooth transition between global/static callback functions and the objects. It stores some pointers in private data fields of JS objects and has to take care of some rooting to avoid garbage collection. I have to admit that this is hidden pretty well from the class users but what it does is still quite ugly (necessarily for that approach).

I thought about either extending or removing the CJSObject-approach and my conclusion was that the additional flexibility is not really needed and the simplicity of global functions is the better deal.

Here are some pros and cons I listed if you're interested (+ for pro, - for contra, ? for unknown or to-be-analyzed):

CJSObject (to be removed):

+ Class users have to write very little code and it's easy to use

+ Supports properties plus methods

+ Cares about the transition from static/golobal functions/variables to object specific member functions/variables

+ Makes objects available in Javascript which adds some nice grouping of functions and properties

- Makes objects available in Javascript which makes the separation between engine code and script code less clear

- Makes the C++ side of the interface more complex because it has to cope with objects instead of just global/static functions

- The class uses some clever and rarely used c++ concepts like pointers-to-data members which makes it hard to understand

- The methods use arguments like JSNative functions ( JSContext* cx, uint32_t argc, jsval* argv ) which isn't as nice as the ScriptInterface which uses required typed arguments. Functions in the JSNative type need to make sure all required arguments are present and are of the correct type. That's something the function shouldn't have to worry about. It can cause crashes if it's not done properly and it makes the code less readable.

? Rooting

? Usage of g_ScriptingHost/Script registration: The global "g_ScriptingHost" is still used in the class. Instead a function should be implemented that registers the object with its properties and methods to a specified ScriptInterface context.

? The class supports different kinds of properties but their purpose / design idea is currently partially unknown.

static PropertyTable m_NativeProperties; // The properties defined by the engine

PropertyTable m_NonsharedProperties;

PropertyTable m_ScriptProperties; // Properties added by script

ScriptInterface::RegisterFunction (to be used)

+ Functions can be registered with strictly typed arguments and return types. A lot of plausibility-checking of arguments and their type is done by design an the functions have to worry about it

- Currently limited to global/static functions

Changing CGameView from the old pattern with CJSObject to the new one with global functions was very straight forward. Here is a reference implementation of CGameView

@stwf

I asked you about changing the Soundmanager code.

What I had in mind was something like the example above plus the required changes in JS-Code and something as a replacement for passing data across GUI pages using "global.". For the moment you could use g_ScriptingHost for calling RegisterFunction instead of passing the ScriptInterface as an argument like in the attached example. I can easily change that in my code without causing merging conflicts later.

Some additional thoughts about a configuration system

Especially the renderer makes a lot of properties available to scripts that are essentially settings.

When an options-screen eventually will be implemented a lot more settings will have to be accessible to scripts. I didn't want to develop such a system for settings because it's actually quite complex and requires some more time thinking about it. That should basically also be possible with global functions or the JSAPI could be used directly (something like the GUI does). In the worst case it would still be possible to reintroduce CJSObject if there are good reasons.

Edit: Leper and I discussed that on IRC and came to the conclusion that functions are probably better anyway for the settings because they can provide a return value if for example the setting can't be enabled because the hardware doesn't support it.

Is anyone against any of these changes or do you have additional inputs to consider? I've spent a lot of time thinking about it and I'm sure enough to write that post, but I still don't understand everything in detail and could be wrong about some aspects.

[zoot]

If you are redesigning scripting access, I recommend taking this issue into consideration: http://trac.wildfire...com/ticket/1589

In short, we need a 'manager' or something similar which can be accessed from all ScriptInterface instances.

Edited May 1, 2013 by zoot

[RedFox]

This is a huge change, but also something just has to be done. We could follow OOC (Object-Oriented C) approach, where objects are expressed as opaque handles:

// myscript.js
var g_Playlist = [ "music01.ogg", "music02.ogg" ]; // our playlist
var g_Current = 0; // invalid handle
var g_Next = 0; // index for the next sound to play
// ... some event ...
{
	// where sound_* is the global sound interface
	var newsound = sound_load(g_Playlist[g_Next++]); // load the next piece
	if(g_Next >= g_Playlist.length) g_Next = 0;
	sound_stop(g_Current); // stop the current music after we actually loaded newsound
	sound_play(newsound); // start playing the new music
	g_Current = newsound;
}

The C++ code would keep track of the handles and their validity. For example 0 would be an invalid handle. You won't ever get dangling pointers this way, since the handles can be invalidated after it has been deleted. Null pointers are also impossible, since handle 0 is the mark of an invalid handle - in which case the proper interface method should do nothing.

Edited May 2, 2013 by RedFox

[stwf]

I'm willing to make whatever changes are needed to move the game forward, but there are a few things that need to be thought through here. What time frame are we talking about here?

I'm not as much in favor with passing handles back to the simulation. It makes me think I would need to keep these references around forever since I wouldn't know when a handle went out of scope or the converse, forcing js developers to dispose of each handle seems like a complication for them. In general I've tried to move the work into the C code like with playlists.

Is the behavior of source/simulation2/CCmpSoundManager along the lines of what you want?

[stwf]

Is anyone against any of these changes or do you have additional inputs to consider? I've spent a lot of time thinking about it and I'm sure enough to write that post, but I still don't understand everything in detail and could be wrong about some aspects.

Hi , they seem doable, so if it helps out keeping the code current I have no problem with it...

Trying to build the latest code with your patch applied brings up the following errors and warnings though:

../../../source/graphics/GameView.cpp:621:10: warning: address of function

'CGameViewImpl::ConstrainCamera' will always evaluate to 'true'

[-Wbool-conversions]

if (!m->ConstrainCamera)

~~~~^~~~~~~~~~~~~~~

../../../source/graphics/GameView.cpp:407:18: error: no matching member function

for call to 'RegisterFunction'

...scriptInterface.RegisterFunction<void, bool, &CGameViewImpl::LockCullCamera>...

~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

[Yves]

The patch wasn't really meant to be applied directly. It was more a general idea how it could work.

I've changed the implementation approach again. Adding a separate file with some global functions in a namespace instead of adding static functions to the class itself seems to be cleaner. When class members become static a lot more in the class may have to become static too, which isn't good.

But again, this patch isn't meant to work directly and without other changes.

New patch here.

[stwf]

Hi,

OK, I have these changes done, although there is a bug or two I'm still working out, the JSObjects have all been replaced by globals.

This is currently in a github branch. Can you get it from there? Would you just like it as a diff file? I suppose I could just check in the changes to svn when its bug free, but that may take a little bit. Would you rather have the changes first?

[Yves]

Very good, thanks!

I should be able to get the diff from the GIT branch myself and this way I can always get the newest version. Please post the link.

Unfortunately I haven't come as far as I wanted in these two weeks of holidays and the progress will slow down when I start working again next week. But anyway, I'm able to start the game and navigate through the menus with sound disabled. At the moment it crashes when I try starting a match because of some issues with the Javascript context's lifetime. Some more issues could come up with the AI because it uses Javascript intensively.

I would like to have a quick look at your sound code now to make sure it is what I need and then wait with the integration in my Spidermonkey working copy until I get a normal match running.

[stwf]

Hi, sorry for the delay, I missed this somehow...

https://github.com/stwf/0ad/tree/spidermonkey

Is the link to the branch, I think any bugs in it now already existed.....

[Yves]

Hi, sorry for the delay, I missed this somehow...

No problem, I didn't have time to work on 0ad in the past three days anyway.

It's awesome how you made these changes so quickly to help with the Spidermonkey upgrade!

I've applied the changes and it works well. I can hear music, the music continues playing across multiple GUI pages, there are ambient sounds after a game starts and it starts playing another piece of music in game after the first is over. I haven't found any issues yet.

Your implementation has a few minor differences compared to the other scripting implementations I "ported" like for GameView or for the Renderer.

We should try to eliminate these differences just for consistency. These are the main differences I noticed:

1. I'm using a directory called "scripting" for the js stuff, which is the same naming that the simulation and the GUI already use. The file containing the global functions which are regsitered to javascript is called "JSInterface_Renderer" or "JSInterface_GameView" respectively. I suggest you also follow this convention just for consistency. Remember to change Premake4.lua if you change the directory name.
   
2. I've put all the global scripting functions in a namespace called JSI_Renderer or JSI_GameView respectively. I think this makes sense, you could also add your global functions in a namespace "JSI_Sound" or similar.
   
3. Currently we use three different ways to make the ScriptingInit/RegisterScriptFunctions function available. I'm using a public non-static class memberfunction for GameView and a public static class memberfunction for the renderer and you use a public non-static global function. Basically I think your approach could be combined with a namespace (point 1) so that we could omit the "Sound" prefix and just name the function "ScriptingInit" or "RegisterScriptFunctions". I prefer the second term because it describes clearer what it actually does. I don't see any advantage of a static class memberfunction at the moment so I'll change that in my code. Another closely related question is...
   
4. What do we do when functions are only available at certain stages in game? "GameView" for example can only be used when a GameView instance is available which currenty means when a match is running. My hacky workaround is only registring the function in the case of "if(g_Game && g_Game->GetView())". That's not very good because this check runs when the GUI page is initialized and in the future it could be required to access GameView's function from a page that was initialized before the game started. I've noticed that you check for "if ( g_SoundManager )" in each function and simply do nothing if it's false. In the soundmanager's case that's probably good enough because it gets initialized quite early and stays active until the game ends. I'm wondering if I should try to implement a common solution for this problem like a check that can be added when registring the function so that the ScriptInterface itself would react sensible and the C++ code providing the functions doesn't need to worry about that in each individual function. This way I could also use the same approach for registring the functions of GameView and they could be used in all pages when GameView is available no matter when the page was initialized.
   

[tuan kuranes]

A note/request while you're on js-c++ interface: with current code, cannot make any method with const or const& parameters, and many of them are string or std::vector, thus a lot of unecessary allocation and copies are made on many js/c++ calls.

Edited May 16, 2013 by tuan kuranes

[Yves]

A note/request while you're on js-c++ interface: with current code, cannot make any method with const or const& parameters, and many of them are string or std::vector, thus a lot of unecessary allocation and copies are made on many js/c++ calls.

I don't think that's possible with the current approach of converting parameters to native c++ types. That would require direct access to Spidermonkey's memory and knowledge of the internal structure of jsvals (which could change from one Spidermonkey release to another). Also as far as I know spidermonkey's GC could theoretically move the storage location of values.

I think it works without copying memory when registring JSNative functions directly and accessing the argv array using the JSAPI.

But being able to register functions with a fixed number of arguments and native C++ types is worth the additional overhead in most cases IMO.

Also keep in mind that this approach is currently only used for relatively simple argument types like integers or strings and not for passing large objects. I know strings are only "relatively" simple but simplicity of code and the additional automatic validation of the number of arguments and their type is also valuable and not just the performance.

In my opinion we should only worry about this performance loss if we can prove that it's significant enough using the profiler. In this case we can find a specific solution.

[ot]btw, how did you make the graphs in the trac ticket ? is there a wiki page explaining that?[/ot]

Which ones do you mean? This?

It was created using Profiler2. Check source/tools/profiler2 and I've written a short howto here.

Unfortunately there's no wiki article explaining it. It's on my todo-list since quite a while...

[Yves]

I wanted to test the new Spidermonkey in the profiler a first time. And guess what... it's currently even a little bit slower than the old version.

This is from the non-visual replay on Oasis4 with 4 AI players (so no rendering, no GUI etc...). V185 is the old version and the other two lines are both from the new spidermonkey (I ran it twice to be sure).

I'll try to figure out why it's slower this weekend. I have some ideas and I don't think it will stay like that.

[stwf]

No problem! It actually made my code much simpler, so I'm glad to have them in there...

I'll incorporate all of your suggestions soon, although I may let you work through it a little more on your own so I get all of the fixes at one time ;-)

What do we do when functions are only available at certain stages in game? "GameView" for example can only be used when a GameView instance is available which currenty means when a match is running. My hacky workaround is only registring the function in the case of "if(g_Game && g_Game->GetView())". That's not very good because this check runs when the GUI page is initialized and in the future it could be required to access GameView's function from a page that was initialized before the game started. I've noticed that you check for "if ( g_SoundManager )" in each function and simply do nothing if it's false. In the soundmanager's case that's probably good enough because it gets initialized quite early and stays active until the game ends. I'm wondering if I should try to implement a common solution for this problem like a check that can be added when registring the function so that the ScriptInterface itself would react sensible and the C++ code providing the functions doesn't need to worry about that in each individual function. This way I could also use the same approach for registring the functions of GameView and they could be used in all pages when GameView is available no matter when the page was initialized.

In my case its best since the SoundManager will also be nil if the game was compiled without audio support, or if the user disables music from the settings menu during a game. So its a case I always have to deal with.

[tuan kuranes]

Thanks for the graph link. Definitely needed on wiki. Can I just at least copy/paste it there, even if not exhaustive, that help when searching for it ?

Also keep in mind that this approach is currently only used for relatively simple argument types like integers or strings and not for passing large objects.

In CCmpRangeManager: ExecuteQuery, ResetActiveQuery, GetEntitiesByPlayer, etc.

All those methods do uses std::vector from and to js, and are called very frequently.

In my opinion we should only worry about this performance loss if we can prove that it's significant enough using the profiler.

It does show here with "very sleepy" profiler. Js related string, malloc, free are in the top calls.

Not that it beats the huge "EconomyManager.prototype.buildDropsites" perf gap in aegis bot, but memory fragmentation is the reason of the overall slowdown over time of 0ad.

[wraitii]

That function should be disregarded when profiling. It does big 2D image manipulations that really shouldn't be done in JS in one turn. I'll try to find a way to even that one out whenever I can.

[scroogie]

wraitii: do you mean EconomyManager.prototype.getBestResourceBuildSpot()? Maybe that could even be held globally on the C++ side?

[wraitii]

Exactly. Basically it handles a lot of maps in one go, and JS is not exactly perfect to do that. I'm considering switching some stuffs to c++ as part of improving the AI. I want however to be sure that the added cost of memory transfers are not bigger than the increase in computation speed.

[Yves]

I haven't found the problem yet.

First I thought it could be caused by JIT code being thrown away by the garbage collector and then being regenerated over and over again. I found a switch that apparently turns off gargabe collection of JITed code completely. It increased performance a bit, but that's barely visible on the graph (total_keepjit/bright blue).

Another Idea I had was that the wrapper values for sharedAI and gamestate passed to the AI cause overhead each time they are accessed. The new Spidermonkey doesn't support multiple global objects per compartment or per runtime anymore, so the gamestate and the sharedAI object can't be passed directly to each AIPlayer as we did with 1.85. This means we have to pass wrapper objects that allow them to be accessed through compartment boundaries.

For testing I changed the code to copy the gamestate to each AIPlayer's compartment at the beginning of the AI computations. My theory was that this would cause overhead at the beginning of the simulation turn but If the wrapper acces made a significant difference, it should speed up those turns that contain a lot of wrapper access and therefore the pieks on graph should become smaller.

As you can see (total_copy_state/red) the performance decrease is relatively even accross all turns. My conclusion is that wrapping doesn't have a significant impact on performance.

I also tested how fast it is with methodJIT completely disabled (total_nojit/yellow).

If you compare it to the previous graph (total_new/green), you see that methodJIT improves the overall simulation performance by about 20-35% in this case (I just picked 3 points in the graph). This at least proves that JS execution performance can make a difference and motivates me to try V18 with IonMonkey.

I also played a bit with valgrind but didn't really come to a conclusion there.

The next test will be with qbot because it doesn't use the shared API and therefore needs less compartment switching/wrapping.

Thanks for the graph link. Definitely needed on wiki. Can I just at least copy/paste it there, even if not exhaustive, that help when searching for it ?

Yes you can copy it to the wiki if you want. There's already an outdated profiling wiki page.

In CCmpRangeManager: ExecuteQuery, ResetActiveQuery, GetEntitiesByPlayer, etc.

All those methods do uses std::vector from and to js, and are called very frequently.

Thanks for the hint. For the moment I focus on my changes that could have caused the measurable performance difference. Most of what I changed is in the GUI where it doesn't affect the graph I posted and I didn't touch the simulation. But that's something I will also check when this problem is solved.

Exactly. Basically it handles a lot of maps in one go, and JS is not exactly perfect to do that. I'm considering switching some stuffs to c++ as part of improving the AI. I want however to be sure that the added cost of memory transfers are not bigger than the increase in computation speed.

A guy on the Mozilla #jsapi channel posted this recently:

https://gist.github....idereit/5616961

He also describes some situations where moving code to JS actually improves performance.

The only way to figure it out is probably testing.

[Yves]

Non-Visual replay with qbot:

./pyrogenesis -quickstart -autostart="Oasis 04" -autostart-ai=1:qbot -autostart-ai=2:qbot -autostart-ai=3:qbot -autostart-ai=4:qbot

Random map generation:

./pyrogenesis -autostart=alpine_lakes -autostart-random=123 -autostart-size=256

Spidermonkey 1.85 (3 runs)
TIMER| LoadRMS: 35.2256 s
TIMER| LoadRMS: 35.5012 s
TIMER| LoadRMS: 35.4555 s

Spidermonkey 17: (3 runs)
TIMER| Load RMS: 36.8751 s
TIMER| Load RMS: 37.1415 s
TIMER| Load RMS: 36.8051 s

... I start considering that it really simply is slower

Maybe I should just try V18.

[alpha123]

That is very surprising. Is all the JIT stuff properly enabled and E4X disabled?

You might want to consider asking the Mozilla folks about this; Firefox has gotten considerably faster from FF4 (SM 1.8.5) to FF17 (SM 17). So this looks like some sort of regression for our case.

EDIT: After some discussion on IRC apparently E4X was enabled, but disabling it barely made a difference. (I thought I read somewhere that E4X had some global overhead even if it's not actually being used -- apparently that's not really true.)

Edited May 23, 2013 by alpha123

[Yves]

I have another idea what I could try.

I'm going to write a simple example program that uses Spidermonkey and executes a simple script with a large loop or something.

Then I'm going to test this with both 1.85 and 17.

That should narrow the problem down. Either we can see a performance boost for v17 there and I can start figuring out where the difference is or the situation is the same, which would mean that the problem must be very closely related to Spidermonkey and there's no need to look at the details about how our code uses Spidermonkey.

[scroogie]

Its so confusing with all these different things like TraceMonkey, JägerMonkey, IonMonkey, whateverMonkey, maybe some optimization is simply not enabled?

[Yves]

... maybe some optimization is simply not enabled?

I've tested those options that look suspicious and I don't think that I've missed an important one, but It's still possible because there are quite a lot options and I don't completely understand the meaning of every option.

[tuan kuranes]

I guess that's been covered a lot(couldn't find in forum search, though), and I'm very late there, but why not V8 js engine ( https://code.google.com/p/v8/ )?

There's a bigger community around than mozilla js afaik, lots of apps & docs & tutorial around (http://athile.net/library/wiki/index.php?title=Library/V8/Tutorial), nicer docs ( https://developers.google.com/v8/embed ) and also has buitltin profiler/debugging capabilities. (even gdb support https://code.google.com/p/v8/wiki/GDBJITInterface or with some work webkit/chrome dev tools like nodejs did here https://github.com/dannycoates/node-inspector ).

Some nice working/example c++ interfacing already exists: https://code.google.com/p/nasiu-scripting/ (that one got me with the "std::vector covered"), and the persistent case is indeed covered ( https://developers.google.com/v8/embed#dynamic ).