[PATCH] SpiderMonkey 38 Upgrade

[leper]

SpiderMonkey 38 was released some time ago, and it brings some improvements like eg better ES6 support. I've started working on the upgrade and it compiles without problems right now, but there is still 1 test failures (which is to be expected and cannot easily be fixed since the function we relied on was removed). Some review, testing and suggestions would be nice. There is a commit for each upstream change that required a change in our code, or did not fit in any of those. The last commit contains some TODOs. The debug api was removed and JS_SetFunctionCallback which could have been used for our needs was also removed in SM37 (hinting at both the JS Debugger and the tracelogger). Given the high number of changes needed and for easier testing/reviewing the code is in the sm38 branch of my github repo. ​https://github.com/leper/0ad/tree/sm38 (Note that this will be rebased (both on top of 0ad/0ad and interactively).)

[Yves]

I've looked through all the commits separately and couldn't find many issues, well done! It was quite convenient that you have split up the changes into different commits so well.

General - Issues/TODOs/Comments:

• build-osx-libs.sh: LIB_DIRECTORY="mozjs31"
• Copyright 2015 -> 2016
• Maybe it's time to remove the JS Debugger completely
• Compare performance and memory usage
• Commit changes separately which also work for SM31
• update-workspaces.sh (and premake4.lua): with_system_mozjs31
• The build flags should be checked because some might have been removed or default values could have been changed.
• One example of changed build flags is the tracelogger, which is now built by default, so it doesn't make sense anymore to explicitly enable it. According to H4writer, the overhead is minimal and I'd say it's probably fine to have it enabled. The advantage is that it should now be usable on Windows too without having to manually build SpiderMonkey.Relevant bug: ​1071546
• Some cleanup changes are mixed with other changes. There are only a few, but it would be nice to have them in a separate commit. I've mentioned it below because I was looking at all patches separately anyway.

Some comments to specific changes

​0f5b2e57 Remove workaround.

• Cleanup (tabs in declaration of m_Classk, m_Construtor and m_Prototype) -> separate commit

​564131b5 TODOs.

• Cleanup (tab removed)
• Also see comments for 92fe1422

​9d6a7823 JS_GetStringCharsAndLength was removed, use JS_Get{Latin1,TwoByte}StringCharsAndLength instead

• What about using "out.assign(ch, ch + length);" instead of "out = std::wstring(ch, ch + length);"?

I doubt it makes a big difference for performance with move constructors, but maybe a little and it looks a bit better style-wise for me.

• I've red the article about the new string types, very interesting! I wonder how much it reduces memory usage.
• I wondered whether the property names are stored in Latin1 if possible or if they are always TwoByte if you set them with JS_SetUCProperty because it

takes a char16_t* parameter. I've spent some time digging through the SpiderMonkey code and figured out that it actually "deflates" them automatically, which is very good because we can take advantage of the improved strings for property names without having to worry about that.

​aa596b92 Remove workaround for crash when using multiple ctxts per runtime

• Cleanup (tabs) -> separate commit

​c22317ca The GC API changed ...

• Cleanup (space) -> separate commit
• Looks good for now. I think fine-tuning GC isn't quite worth the efforts yet because it still changes too much upstream.

​92fe1422 The JS_LookupProperty{,ById} API was removed.

• I suggest we use JS_GetPropertyDescriptor to check for getters, similar to how it was before this r7579
• The two comments about rooting ("Note that we don't do any rooting [...]") are outdated and wrong and should be removed. We should always do proper rooting instead of relying on the observation that some functions do not GC with the current SpiderMonkey implementation (these cases with strings or array buffers where we get a direct pointer are an exception of course, but there we have the these nogc guards that should protect us from most problems)

​95834e9d Cleanup, can be dropped or moved elsewhere.

• As you mentioned in the commit message (should go with the other cleanup changes)

​288af6f3 JS_NewObject -> JS_NewPlainObject.

• Cleanup (space) -> separate commit

​02ea3451 Some JSClass stubs were removed, and the pointers are nullable.

• Cleanup (spaces) -> separate commit

​8dec5d5a Update ObjectToIDMap to the ESR38 version.

• I haven't checked if the code makes sense, I assume it was just copied from the new version.
• I'm not sure if we need the sweep, clear implementation
• find and add swapped places, which makes the diff less readable

​a03f7fc1 The old debug api was removed.

• Script profiling functionality was not completely removed from Profiler1 (as you already mentioned). It should be completely removed or implemented again for SM38. I'd vote for removing it. It is nice in some cases, but the tracelogger is better in most cases and now it should be more easily usable from Windows too. It's quite a lot of work to fix it again with each API change and brings too few unique benefits now.

[leper]

In 17630:

JS_DumpHeap is removed in SpiderMonkey 38, since we have nothing using it remove it. Refs #3708.

​https://bugzilla.mozilla.org/show_bug.cgi?id=1105069
​https://bugzilla.mozilla.org/show_bug.cgi?id=1122842

[leper]

In 17633:

SpiderMonkey 38 removes the JS_LookupProperty{,ById} API. Refs #3708.

Use JS_GetProperty{,ById} instead.
Ensure that we break if someone tries to serialize a getter by using something similar to what we used pre r7579.
​https://bugzilla.mozilla.org/show_bug.cgi?id=1094176

[leper]

In 17655:

Remove JSDebugger. Closes #2419, #2973. Refs #410, #2348, #3708.

​https://bugzilla.mozilla.org/show_bug.cgi?id=1069694

[Yves]

In 17695:

Replace HandleWrapper and avoid repoint function

JS::Handle<T>::repoint gets removed with SpiderMonkey 38, so the existing solution has to be replaced. The new approach should also be a bit clearer. Named Return Value Optimization (NRVO) should avoid a superfluous temporary for the return value in the generic template function implementation of AssignOrFromJSVal.

Refs #3708

[Itms]

Is any more review needed here? And do we want that in before A20?