Jump to content

[Discussion] Spidermonkey upgrade

Yves
 Share

Recommended Posts

Yves Primus Pilus

Yves

Posted
  • Author
Posted

@fabio

Thanks for updating the GamePerformance wiki page.

I didn't know they already have a page for Spidermonkey24. I found a good description of the issues with changed integer types and conflicts with our typedefs/defines there. :)

http://whereswalden....mes-to-mozilla/

  On 06/06/2013 at 12:56 PM, stwf said:

ok, I checked my fixes for this upgrade into main trunk. It just makes things so much easier. I did incorporate the suggestions you made on the first batch.

Thanks.

Link to comment
Share on other sites
  • 2 weeks later...
  • Replies 208
  • Created
  • Last Reply

Top Posters In This Topic

Popular Days

Top Posters In This Topic

Popular Days

Popular Posts

Yves
Yves

Between Spidermonkey 1.8.5 and 38 there have been many profound API changes and it took us countless hours to adapt the code to these changes. The GUI was especially difficult because it used a wider

Yves
Yves

The upgrade is quite close to completion now! I've fixed serialization issues, compiler warnings, tests, replaced the SpiderMonkey usage in Atlas (#2434), cleaned up the patch and fixes other smaller

Yves
Yves

The SpiderMonkey upgrade is ready for review. Check the comments on trac and get it from my github repository if you want to help testing and reviewing.

Posted Images

abral Discens

abral

Posted
Posted

The new Mozilla's JS engine, IonMonkey, brought a lot of performance improvements, and it only landed in Firefox 18. So most probably v22 would be a lot faster than v17.

But, if you could use v24 it would be even better, as the performance is improving from release to release, and v24 contains also OdinMonkey (asm.js) that you could use in some cases (for example for the AI, instead of moving code to C++, you could use asm.js so that the code would be a lot faster and there wouldn't be any performance penalty caused by JS-C++ calls).

  • Like 2
Link to comment
Share on other sites
Yves Primus Pilus

Yves

Posted
  • Author
Posted

The good news is that my working copy is now ready for Spidermonkey 24!

The bad news is that it performs even worse than Firefox 17 and much worse than 1.8.5.

On the other hand the good news is that it seems very unlikely that Spidermonkey 24 with all the new improvements like Ion-Monkey and the new Baseline compiler can really be slower than the old 1.8.5 from Firefox 4. I still hope there's a "--make-everything-fast" switch or something :D.

I've already tested the jemalloc switch because of all the memory performance discussions. I don't know what it actually does or if it works but there's no visible difference.

post-7202-0-71037200-1371843265_thumb.gi

... some hard work for this weekend.

Link to comment
Share on other sites
Yves Primus Pilus

Yves

Posted
  • Author
Posted

I also tested alpine lakes random map generation.

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

TIMER| Load RMS: 253.25 s

It was between 35 and 37 seconds for 1.8.5 and 17.

Someone on the #jsapi channel suggested setting the IONFLAGS environment variable and running a debug build.

I think that was quite a good suggestion:

  Reveal hidden contents

Now I just have to solve these problems somehow...

Link to comment
Share on other sites
Yves Primus Pilus

Yves

Posted
  • Author
Posted

Yay, finally something is faster instead of slower! :)


TIMER| Load RMS: 13.2699 s

... which is quite an improvement compared to 253 seconds and it's also much faster than the unpatched v1.8.5 or v17 with 35-37 seconds!

This was the important hint:


[Abort] NYI inlined get argument element
[Abort] aborted @ maps/random/rmgen/random.js:39
[Abort] Builder failed to build.

I had to replace dynamic argument number checking in the functions randInt and randFloat.

rm_random_perf_fix_v1.0.diff

  • Like 1
Link to comment
Share on other sites
Yves Primus Pilus

Yves

Posted
  • Author
Posted
  On 23/06/2013 at 6:07 AM, fabio said:

Great news indeed! But this patch only speeds up 24 or also older versions?

Actually I've already tested replacing the randInt function in Spidermonkey 17, but not randFloat because that was called less often.

It didn't make a big difference.

There's also a bug for this in Bugzilla.

Link to comment
Share on other sites
Yves Primus Pilus

Yves

Posted
  • Author
Posted

I enabled some more logging. There are hundreds of these messages (same file, same line).

What bailouts are is described here.

  Reveal hidden contents

EDIT: The access to this.widthMap[index] here is an out of bounds access.

It's even printed as warning but only once so I didn't fix it before. This out of bounds access happens many times per turn.

I'll try to fix it tomorrow.

EDIT2: h4writer from the #jsapi channel suggested another solution for the random map performance issue.

According to him it's only the access to arguments[x] that causes problems and not the arguments.length.


function randInt(arg0, arg1)
{ 
	 if (arguments.length == 1)
	 {
		 var maxVal = arg0;
		 return Math.floor(Math.random() * maxVal);
	 }
	 else if (arguments.length == 2)
	 {
		 var minVal = arg0;
		 var maxVal = arg1;

		 return minVal + randInt(maxVal - minVal + 1);
	 }
	 else
	 {
		 error("randInt: invalid number of arguments: "+arguments.length);
		  return undefined;
	 }
}

Link to comment
Share on other sites
h4writer Discens

h4writer

Posted
Posted

About the constant hitting bound check. I'm not sure this is a JS error, it could potentially be an IonMonkey issue. I'm willing to look into it and fix it if you can provide me a shell version of it.

About the messages: [Abort] Script too large (2149 bytes)

This is telling me you are running a non-threadsafe build. Therefore you don't have parallel compilation and we cap the max script size to 2000 bytes.

You should really try a threadsafe build. (That's the default in the browser). The maximum script is 20000bytes and compilation happens on background thread.

Should give a nice performance boost.

To compile a threadsafe shell:

$ cd mozilla-inbound

// this is needed for threadsafe build

$ cd nsprpub

$ mkdir build-64

$ cd build-64

$ ../configure --disable-debug --enable-optimize

$ make

// building js shell

$ cd mozilla-inbound/js/src

$ autoconf2.13

$ mkdir build-64

$ cd build-64

$ ../configure --disable-debug --enable-optimize --enable-threadsafe --with-nspr-cflags="-I/PATH_TO/mozilla-inbound/nsprpub/build-64/dist/include/nspr" --with-nspr-libs="/PATH_TO/mozilla-inbound/nsprpub/build-64/dist/lib/libnspr4.a /PATH_TO/mozilla-inbound/nsprpub/build-64/dist/lib/libplc4.a /PATH_TO/mozilla-inbound/nsprpub/build-64/dist/lib/libplds4.a"

$ make

Execute test.js

$ ./js --ion-parallel-compile=on test.js

  • Like 2
Link to comment
Share on other sites
Yves Primus Pilus

Yves

Posted
  • Author
Posted

Thanks for your input and your offer to help with troubleshooting!

I'll try your suggestion about the threadsafe build and will also try to provide a standalone test-script for the bound-issues.

That could be a bit tricky but I'll see what I can do. :)

I will have some time to work on it this Thursday and then on the weekend again.

Link to comment
Share on other sites
historic_bruno Primus Pilus

historic_bruno

Posted
Posted

Something worth investigating if we upgrade Spidermonkey, we've been having a discussion about Spidermonkey floating point math (spurred by #1633, #1990 and #433), does the new Spidermonkey still try for consistent floating point rounding on different platforms? In the past, some people cared enough to find and fix any discrepancies that popped up, but that bug report is 4 years old and surely much has changed.

Link to comment
Share on other sites
h4writer Discens

h4writer

Posted
Posted
  On 26/06/2013 at 11:07 PM, historic_bruno said:

Something worth investigating if we upgrade Spidermonkey, we've been having a discussion about Spidermonkey floating point math (spurred by #1633, #1990 and #433), does the new Spidermonkey still try for consistent floating point rounding on different platforms? In the past, some people cared enough to find and fix any discrepancies that popped up, but that bug report is 4 years old and surely much has changed.

Running code in JIT or the interpreter shouldn't make a difference. So we definitely care for these issues! That was an issue reported by Gary and is about an problem in JM/TM vs the interpreter. Both are removed now in favor of IM. I'm not gonna pretend we don't have these issues in IM, but we try to fix them. Gkw, decoder and Jesse are running special bots to find these issues. Now and than they pop up during differential testing. So reporting bugs about that is highly welcome!

Just reading the bugs, I don't think we can do much about floating point issues on different platforms, though that wouldn't be a total performance loss. Also I don't know how widespread the issues are and what the error range is. I assume they are very very small and in that case I don't think it will make such a difference?

Link to comment
Share on other sites
historic_bruno Primus Pilus

historic_bruno

Posted
Posted
  On 26/06/2013 at 11:56 PM, h4writer said:

Running code in JIT or the interpreter shouldn't make a difference. So we definitely care for these issues! That was an issue reported by Gary and is about an problem in JM/TM vs the interpreter. Both are removed now in favor of IM. I'm not gonna pretend we don't have these issues in IM, but we try to fix them. Gkw, decoder and Jesse are running special bots to find these issues. Now and than they pop up during differential testing. So reporting bugs about that is highly welcome!

Just reading the bugs, I don't think we can do much about floating point issues on different platforms, though that wouldn't be a total performance loss. Also I don't know how widespread the issues are and what the error range is. I assume they are very very small and in that case I don't think it will make such a difference?

Thanks for responding and it's good to hear that's still important :) Actually even very small differences across platforms matter to us, because we do a hash of our binary simulation state to detect if a client goes out of sync. Even one bit difference is an OOS (as I've discovered recently where NaN can have different internal representations possibly due to JIT behavior/bugs? - which is fine according to the spec, but a headache for us).

This is why we are implementing our own Math functions for trig, pow, exp, etc. Assuming the last comment from this bug report is still valid, and like Philip I can't blame you all if it is.

Link to comment
Share on other sites
h4writer Discens

h4writer

Posted
Posted (edited)
  On 27/06/2013 at 3:03 AM, historic_bruno said:

Thanks for responding and it's good to hear that's still important :) Actually even very small differences across platforms matter to us, because we do a hash of our binary simulation state to detect if a client goes out of sync. Even one bit difference is an OOS (as I've discovered recently where NaN can have different internal representations possibly due to JIT behavior/bugs? - which is fine according to the spec, but a headache for us).

This is why we are implementing our own Math functions for trig, pow, exp, etc. Assuming the last comment from this bug report is still valid, and like Philip I can't blame you all if it is.

About the rounding errors. Yes I think the last comment is still valid and I don't think that will change soon.

I'm sure that we use one NaN representation in the JIT. Since that makes our live easier. I actually assumed that is also the case in the interpreter? If NaN is the same across platform is another question.

I can ask around if you want to know.

  On 27/06/2013 at 6:22 AM, Yves said:

I also wondered what this setting in the configure script does:


  --enable-more-deterministic
						  Enable changes that make the shell more deterministic"

That is a flag mostly for fuzzers. It contains little hacks to make life easier for them. To make scripts more deterministic in output/behaviour.

E.g. last bug I can remember (not implemented yet) is don't show the compilation time of asm.js in the warning. (Since that is not deterministic and a nightmare for fuzzers)

But it will mostly make sure that the error messages are always the same. We try to always give as much as possible information in the error message. So we try to give the value that is fault. But in the JIT we often cannot give that. So we get different error messages, depending on which engine is running. Very annoying for the fuzzer.

This is definitely not for production ;)

Edited by h4writer
Link to comment
Share on other sites
Yves Primus Pilus

Yves

Posted
  • Author
Posted

I tried running a threadsafe build today.

First it segfaulted instantly when loading the library in debug mode. I figured out that it's our override of the free function that causes it.

Uncommenting the whole #IF block here works around the issue.

Now it triggers a Spidermonkey assertion in Debug mode when starting a match (the main-menu loads fine):

  Reveal hidden contents

Apparently I'm not the only one with this problem. It seems that having multiple runtimes in one thread isn't supported anymore.

Is that true?

Link to comment
Share on other sites
h4writer Discens

h4writer

Posted
Posted

I do not know the details (yet) about that part. So I could ask it on irc myself. But I suggest you do it yourself? Since that way you will get the most information about what is possible and not and how to work around it. I think Luke (:luke) would be the best person to ask it. He is stationed in Mountain View, but is online from 18h CEST. If getting online on those times is hard, you can always mail him. (luke AT mozilla . com). I introduced him yesterday to this project, since he didn't know it yet ;).

https://blog.mozilla.org/luke/2012/01/24/jsruntime-is-now-officially-single-threaded/

Link to comment
Share on other sites
Yves Primus Pilus

Yves

Posted
  • Author
Posted

I've created a standalone test script. It calculates a specified number of random paths (random start-point and random end-point) using hardcoded JSON map data (I copied that from our acropolis scenario map).

It's probably a bit buggy and definitely ugly, but it can reliably reproduce the BoundCheck messages which is most important.

Btw. the JSON map data is quite large. If you want to edit the script you need a text editor that doesn't have problems with 800'000+ characters per line. ;)


export IONFLAGS=aborts,bailouts,bl-bails
./js --ion-parallel-compile=on tests/pathfind.js

pathfind.js.zip

Link to comment
Share on other sites
Yves Primus Pilus

Yves

Posted
  • Author
Posted

I've made my working copy ready for the threadsafe build. I also found that there were some structured clones left from my tests that could be replaced by wrappers. Removing these structured clones improved performance quite a bit but the threadsafe build has nearly no impact at all in this scenario.

It should be able to do Ion compiling and garbage collection in another thread now, so it has nothing to do with moving the AI or the Simulation code to another thread. Since I'm now only testing the Simulation and the AI context is constantly in use I expect some more improvements for the threadsafe build in normal games.

From the documentation:

  Quote
at any given moment there can either be multiple threads in active requests, or one thread doing GC and all requests suspended.

This means to me that if we move the AI to a separate thread and into a separate runtime it can do all the garbage collection in another thread between simulation turns when no active requests are needed. That should make quite a difference because a lot of time is spent doing garbage collection.

There's an issue with the threadsafe build when running the HWDetect script at startup. I've filed a bug for that here.

post-7202-0-23895900-1372964145_thumb.gi

Here are some other abort messages from Ion, if anyone can provide some helpful information (I haven't analyzed them much yet).

  Reveal hidden contents

Especially these are annoying because they don't point to a line of code and because there are so many of them:


[Abort] OSR script has argsobj

Link to comment
Share on other sites
h4writer Discens

h4writer

Posted
Posted

Sorry about the delay. Next week I'll be more responsive. I've got all the important stuff done now.

I looked to the BoundsCheck issue and it definitely looks like an issue of IonMonkey. It should remember an element was set out of bound and when recompiling using a alternate version that doesn't bail in this case. I still have to look why and create a patch. I hope I can create a patch this weekend. Else I will provide one in the beginning of next week. Thanks for reporting!

About "OSR script has argsobj". You can see which script it is, by providing IONFLAGS=scripts,aborts. The line before the abort is the script where it is failing on. I would definitely suggest to not use "arguments" as much as possible. It is known to decrease performance on all engines a lot...

Another issue that causes JS engines to deoptimize a lot is using "delete". Never use that! It pollutes the type information. It is much better to set it to undefined or null.

http://jsperf.com/delete-vs-undefined-vs-null/16 (I'm not a fan of jsperf and microbenchmarks, but here it is quite accurate)

Also it seems like we haven't added JSOP_DELPROP to ionmonkey yet. So that means the whole script will run in baseline compiler and will be much slower. So I would suggest to adjust that!

Link to comment
Share on other sites

Join the conversation

You can post now and register later. If you have an account, sign in now to post with your account.

Guest
Reply to this topic...

×   Pasted as rich text.   Paste as plain text instead

  Only 75 emoji are allowed.

×   Your link has been automatically embedded.   Display as a link instead

×   Your previous content has been restored.   Clear editor

×   You cannot paste images directly. Upload or insert images from URL.

×
 Share
Go to topic listing
×
×
  • Create New...