[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/na-Itms/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?

[Itms]

I fixed some little things (the debug build now runs without issues), the current state of the work is here: ​https://github.com/na-Itms/0ad/tree/sm38

I'm taking a look at the TODOs from the latest commit. After that I'll squash my work into SVN patches. In the meantime the work is ready for some reviewing!

[Itms]

Ready for reviewing: you can use ​https://github.com/na-Itms/0ad/tree/sm38 for eased reviewing, and you should test the ​squash branch which contains what should be committed:

• Two cleanup patches beforehand
• The new SpiderMonkey files
• All the code changes for the new API
• Two commits making use of new SpiderMonkey features
• An incomplete commit removing one of our workarounds. I still need to figure out how to remove it in the component manager without breaking anything

Except for that last bullet point, I would like to commit that immediately when I come back from holidays, in two weeks, so please review, test and give comments!

[Yves]

• In build.sh there are some more occurences of the SpiderMonkey version number that could probably be replaced by a variable (but it's that way since more than one version, so not really an issue of the update).
• The wiki page about the Tracelogger (EngineProfiling#SpiderMonkeyTracelogger) needs an update. It has to be enabled using some environment variables now instead of the build options and viewing the graph requires you to start a webserver from a python script (see ​https://github.com/h4writer/tracelogger).
• We should call ​JS_Shutdown. Apparently that wasn't really required so far, but the tracelogger requires it (tl-data.json misses a "]" at the end otherwise).
• It seems like performance didn't improve or even got a bit worse with the upgrade. My hypothesis was that it should improve because we have used more new features that broke JIT compiling of SM31 over time and SM38 should support more of these cases. This should not prevent committing, but it could be worth to take a closer look and figure out if there's something we can do about it. I had to spend quite a bit of time on performance analysis for the last two upgrades and it made quite a difference. For v31 I had to add an additional patch that didn't make it into v31 for example.

The performance measurements show the number of seconds a non-visual replay of approximately 10'000 turns took. The hashes are different, but when I run the visual replay simultaneously with both versions, I can't spot any differences and the minimap looks exactly the same in the end. The "petra bugfix" mentioned there is something that got fixed in SVN already, so nothing to worry about. sm38 + petra bugfix in workers.js hash: acb4f8e26b36ee0bef10b0add2be8d83 234.285325253 seconds 243.424827335 seconds 249.598170723 seconds

SVN r18513 + petra bugfix in workers.js hash: dd9b4305f4939dc632fbdc9a4f3c1053 233.902549491 seconds 233.991263663 seconds 233.655866539 seconds

As already mentioned in the previous review (but I've done some more research and have some more details):

• SpiderMonkey build flags in build.sh and build-osx-libs.sh should be checked again.
  • SpiderMonkey always builds with generationl GC (GGC) enabled, so "--enable-gcgenerational" can be removed. Relevant Bugzilla bugs: ​1107349
  • Tracelogging is now built by default, so the comments about "--enable-trace-logging" can be removed or have to be modified. The line about DTRACE_LOG_DIR should still work, but since the tracelogger is now built by default, we might want to use the default value of SpiderMonkey but still keep the line around in build.sh if someone needs to change the path (comment it). Relevant Bugzilla bugs: ​1071546

[Itms]

Thanks a lot for the complete review :)

I will work on this as soon as I'm back.

[Itms]

In 18579:

Style cleanup by leper, preparing the SpiderMonkey update. Refs #3708

[Itms]

In 18580:

The old debug API has been removed in SpiderMonkey 38, so remove profiler1 script profiling.

Patch by leper, refs #3708

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

[Itms]

Attached patch properly calls ​JS_ShutDown. It is not possible to call JS_Init again after JS_ShutDown, so we must have to handle this limitation.

[Yves]

"++m_RuntimeInstances;" shouldn't be inside the if-block. If I fix that, it triggers the assertion on line 118 of ScriptRuntime.cpp when pyrogenesis is started without a "-mod=public" argument (because the engine tries to shut down and restart with the public mod).

[Itms]

Guess it needs something less subtle :P

[Yves]

JS_Shutdown patch for SVN with validation and RAII approach

[Yves]

In 18584:

Properly shut down SpiderMonkey using JS_ShutDown.

This also adds some validation to ensure the correct order of JS_Init, JS_NewRuntime, JS_DestroyRuntime and JS_ShutDown calls.

Refs #3708

[Itms]

This is ready to commit: ​https://github.com/na-Itms/0ad/tree/sm38

The performance stagnation noticed by Yves comes from SM taking a long time to compile some functions. I could not determine what caused those functions to be so difficult to compile. On top of that I encountered a lot of issues because SM38 is already an old version, so the codebase has changed too much and the tracelogging tools are not compatible anymore.

I suggest committing that (the performance is not made worse either) and starting to work immediately on the SM45 upgrade, for which the JIT compiler improved and for which it is still possible to propose patches if we have problems.

Since the last review, I updated the build scripts (​bc850dc) and added a source/tools directory for outputting the traces, along with a useful script that I will document in EngineProfiling (e95ffbc ​3a54d0a, see next comment).

[Itms]

Little update to the tracelogging script (I'm still not good at writing sh scripts!) The new commit is ​3a54d0a.

[Yves]

Modified version of the patch in bug 1227028 (modified context lines in Ion.cpp)

[Yves]

I finally managed to pick the right set of patches to fix the problem with missing stop information in the generated data and to implement regular flushing of tracelogger data to the disk to prevent out of memory issues.

I'm still not sure if the generated tracelogger data is correct, though.

Add this to patch.sh:

# TraceLogger: Assertion failure: i < size_, at js/src/vm/TraceLoggingTypes.h:210". 
# Also fix stop-information to make reduce.py work correctly.
patch -p1  < ../Bug_1223767.diff

# TraceLogger: Limit the memory tracelogger can take.
# This causes tracelogger to flush data to the disk regularly and prevents out of 
# memory issues if a lot of data gets logged.
# Flushing causes a segfault during shrinking GC after this patch, but one of the
# following patches resolves that issue (don't know which one exactly).
patch -p1  < ../Bug_1227914.diff

# Fix tracelogger destructor that touches possibly uninitialised hash table.
# This is also a prereq for the patch in bug 1223636 because some of its context
# lines are modified in this patch.
patch -p1  < ../Bug_1155618.diff

# Tracelogger - Don't treat extraTextId as containing only extra ids
# This fixes an assertion failure: id == nextTextId at js/src/vm/TraceLoggingGraph.cpp
patch -p1  < ../Bug_1223636.diff

# TraceLogger - Fix when to keep the payload of a TraceLogger event
# This fixes an assertion failure: textId < uint32_t(1 << 31) at js/src/vm/TraceLoggingGraph.h
# The original patch is slightly modified because some context lines in Ion.cpp did not match.
patch -p1  < ../Bug_1227028_Mod.diff

The patches: ​Bug_1223767.diff ​Bug_1227914.diff ​Bug_1155618.diff ​Bug_1223636.diff ​Bug 1227028 but there's a modified version that applies: ​Bug_1227028_Mod.diff

EDIT: Actually I'm quite sure the data is still incorrect. What you see on the right side in the tracelogger gui when you click somewhere on the graph should be a callstack. This shows impossible callstacks. Also there are some functions that were compiled 0 times according to the tracelogger but still run in IonMonkey. I've tested with a small log just from opening the game and closing it again after waiting a minute on the main menu. This log looks correct, even after calling reduce.py on it.

[Yves]

Some more patches to append to patch.sh:

# TraceLogger - Handle failing to add to pointermap gracefully
patch -p1  < ../Bug_1266649.diff

# Tracelogger: Don't cache based on pointers to movable gc things
patch -p1  < ../Bug_1280648_Mod.diff

patch -p1  < ../Bug_1224123.diff # Tracelogger: fix the use of LastEntryId in tracelogger.h (prereq for Bug_1231170.diff)
patch -p1  < ../Bug_1231170.diff # Use size in debugger instead of the current id to track last logged item
patch -p1  < ../Bug_1221844_1.diff # Part 1: TraceLogger: Move TraceLogger_Invalidation to LOG_ITEM.
patch -p1  < ../Bug_1221844.diff # Part 2: TraceLogger: Add some debug checks to logTimestamp.
patch -p1  < ../Bug_1255766.diff
patch -p1  < ../Bug_1259403_1.diff # Tracelogger: Only increase capacity by multiples of 2
patch -p1  < ../Bug_1259403_2.diff # Tracelogger: Always make sure there are 3 free slots for events

This solves incorrect stacks displayed in the tracelogger and probably more issues related to it. Somehow it did not exit some nodes when it should have and caused the stack getting nested deeper and deeper the longer the trace ran.

I'm trying to figure out if the data is correct now. It still seems strange that it shows 0 times compiled but > 0% runtime in Ion mode. It might be because it counts the compiling in the helper threads sometimes, but I don't know if that's true. It also seems suspicious that Timer.js:67 still makes up >10% total runtime in a non-visual replay and I'm not sure how it counts "internal" time exactly. Trying to figure these things out now.

[leper]

Regarding r18584: Why is this using GetSingleton() explicitly, instead of doing what all the other "singletons" we have are doing? Also the getter already ensures that it is initialized. Why is it using a list instead of a vector (doing swap and pop) given that the order isn't important? Using full include paths would be nice, even if some of the surrounding code doesn't do that.

[Yves]

I'm glad to see your comments on tickets and your commits again! :)

Replying to leper:

Regarding r18584: Why is this using GetSingleton() explicitly, instead of doing what all the other "singletons" we have are doing?

Do you mean adding a define like g_ScriptEngine that for ScriptEngine::GetSingleton()? Honestly, I didn't notice that it's done that way in other cases. However, I'm not quite sure if it's a good thing or not. It's a bit like trying to hide that it's a singleton.

Replying to leper:

Also the getter already ensures that it is initialized.

I assume you mean the two lines with "ENSURE" in ScriptRuntime.cpp. I've noticed that and even asked Itms about it during the review. Neither of us had a strong opinion about it, but the pro-argument was that this check is something we explicitly want and therefore it's good to write the check explicitly, even if it's a bit of a duplicate. It also gives a more specific error message.

Replying to leper:

Why is it using a list instead of a vector (doing swap and pop) given that the order isn't important?

My thought was that the order of adding and removing runtimes can be arbitrary and using a vector could cause "holes" of unused memory. It's just one, two or maybe three elements, so that's like 128 bit of memory wasted in the worst case and less if we consider that lists also need pointers to the next element. If you think it should be changed, that's OK for me.

Replying to leper:

Using full include paths would be nice, even if some of the surrounding code doesn't do that.

Ah, I've missed that for script ScriptTypes.h. We have actually discussed during the review if the full include path should be used everywhere or just for other projects than the current one. We didn't really come to a conclusion because we don't have that defined anywhere in our guidelines (I couldn't find it at least) and it's also not consistently done so far. I've "kind of" decided to use the full path, but I didn't carefully check all files because we don't really have a rule for that.

[Yves]

Replying to Yves:

I'm trying to figure out if the data is correct now. It still seems strange that it shows 0 times compiled but > 0% runtime in Ion mode. It might be because it counts the compiling in the helper threads sometimes, but I don't know if that's true. It also seems suspicious that Timer.js:67 still makes up >10% total runtime in a non-visual replay and I'm not sure how it counts "internal" time exactly. Trying to figure these things out now.

I've asked h4writer on the #jsapi channel today. Reduce.py ignores the data for everything that's shorter than a certain threshold. This threshold goes up the more data you collect, which means it's very high in our case. This causes compilation events to be ignored and explains why the count can be 0 even though it runs in ion mode. Much worse is the fact that this also applies to the number for total runtime per function. Functions with a high runtime per call show a higher runtime than functions which run the same time in total but are called more often and take less time per call. Reduce.py writes a special corrections file by default to fix these issues, but there was a bug and the tracelogger didn't actually use these corrections to display the data. This is now fixed an the data really looks a lot different and much more plausible. Instead of > 10%, Timer.js:67 now only takes 2.84%.

Internal time is still around 30%, which seems quite a lot to me, but he said that's possible. The tracelogger now doesn't include this internal time in the total time of functions anymore, which is much more intuitive. Before this change, there was a function that took around 8% total time but spent more than 99% flushing tracelogger data (so it wasn't expensive at all but just "unlucky" that flushing happened during the runtime of that function).

To sum it up, I'd say the data looks plausible now if you apply all the patches I've listed above and update the tracelogger (the client from ​https://github.com/h4writer/tracelogger) to the most recent version.

[Yves]

​Bug_1266649.diff Bug_1280648_Mod.diff: attached to the ticket ​Bug_1224123.diff ​Bug_1231170.diff ​Bug_1221844_1.diff ​Bug_1221844.diff ​Bug_1255766.diff ​Bug_1259403_1.diff ​Bug_1259403_2.diff

[Yves]

Actually this one has to be modified too

[Yves]

commands.txt I've used for testing the tracelogger

[Itms]

In 18689:

SpiderMonkey 38 upgrade: 35/35

Some comments for the next SpiderMonkey upgrade. That's all folks, fixes #3708