#2462 closed enhancement (fixed)
[PATCH] SpiderMonkey ESR31 upgrade
Reported by: | Yves | Owned by: | Yves |
---|---|---|---|
Priority: | Should Have | Milestone: | Alpha 18 |
Component: | Core engine | Keywords: | spidermonkey javascript |
Cc: | Patch: |
Description (last modified by )
Change History (47)
comment:1 by , 10 years ago
comment:2 by , 10 years ago
The better to start on the upgrade now rather than wait until after it's been released ;)
comment:3 by , 10 years ago
But in that case you take the risk that you need to change drastically if the guys @Mozilla do so.
comment:4 by , 10 years ago
There are two big advantages when starting with the upgrade now.
- If we find problems that need to be fixed in SpiderMonkey, we can still get these changes in before the next release. The Firefox (and SpiderMonkey) development starts in the mozilla central repository where big features get integrated. After that it moves to the "Aurora" stage where usually only important bugfixes get applied. This transition will be around the 28th of April, so it's good to start now.
- When updating around once a week, it's easier to figure out the reason if a change breaks something. You can check last week's commit logs or ask people what happened last week. That's harder if you have to search in a period of multiple months.
Sometimes there are bugs and it can take a while to figure out it's not a problem in our code, but the advantages outweight this problem.
comment:5 by , 10 years ago
Description: | modified (diff) |
---|
comment:6 by , 10 years ago
C++11 support is required to use SpiderMonkey 31. I have checked today if patching the headers would be an alternative to avoid that, but it would result in too deep changes. I've replaced all the static_asserts, but then didn't investigate any further when I figured out that the headers also use move constructors. I've created ticket #2669 for the C++11 migration.
comment:13 by , 10 years ago
Description: | modified (diff) |
---|
comment:22 by , 10 years ago
Milestone: | Alpha 17 → Alpha 18 |
---|
comment:27 by , 9 years ago
Keywords: | review added |
---|---|
Summary: | SpiderMonkey ESR31 upgrade → [PATCH] SpiderMonkey ESR31 upgrade |
The ESR31 branch is ready for review.
Open tests:
- Test on the Mac
Notes:
- I'm not using a new directory for the SpiderMonkey headers, this means they are part of the diff. You can make the diff more readable this way:
git diff master --name-status | grep -v 'libraries/source/spidermonkey/include-win32' | xargs git diff master --
TODOs (before committing):
- Add new Windows binaries compiled with the for .. of bugfix
- Revert VS2010 compatibility patches (if agreed to drop support for VS versions <2013)
TODOs (C++11 related, before committing)
- Agree definitely on dropping support for GCC < 4.6
- Agree definitely on dropping support for Visual Studio < 2013
comment:28 by , 9 years ago
Just a note that has_broken_pch can be removed from premake4.lua and the GCC check updated to fail with GCC < 4.6.
Also should with-c++11 GCC option forced enabled?
follow-ups: 31 33 comment:30 by , 9 years ago
Some initial comments on the ESR31 branch:
- When building (Ubuntu 14.04, gcc 4.8.2):
In file included from ../../../source/third_party/tinygettext/src/dictionary_manager.cpp:20:0: ../../../source/third_party/tinygettext/include/tinygettext/dictionary_manager.hpp:88:59: warning: ‘auto_ptr’ is deprecated (declared at /usr/include/c++/4.8/backward/auto_ptr.h:87) [-Wdeprecated-declarations] void set_filesystem(std::auto_ptr<FileSystem> filesystem); ^ ../../../source/third_party/tinygettext/src/dictionary_manager.cpp:237:72: warning: ‘auto_ptr’ is deprecated (declared at /usr/include/c++/4.8/backward/auto_ptr.h:87) [-Wdeprecated-declarations] DictionaryManager::set_filesystem(std::auto_ptr<FileSystem> filesystem_) ^
I guess we either want to fix our copy of tinygettext, or disable the warnings.
source/scriptinterface/third_party/ObjectToIDMap.h
: The class isObjectIdCache
, so shouldn't this beObjectIdCache.h
?
LICENSE.txt
: Added trailing whitespace. Also, quite a few other changes to source files are adding trailing whitespace. (Not hugely important, but it makes "git diff" unhappy.)
README.txt
: Need to be careful not to apply that change to SVN.
build/premake/premake4.lua
: Since the use of--with-c++11
was removed, the correspondingnewoption
line should be removed too.
build/workspaces/clean-workspaces.sh
: Should continue to deletemozjs24
(at least for a few months), else people who update a dirty workspace will havemozjs24
hanging around forever.
libraries/osx/build-osx-libs.sh
: Why change to$CPPFLAGS
? That doesn't seem to be defined, and NSPR seems to include some C++ code so I'd guess it ought to still haveCXXFLAGS
set.
libraries/source/spidermonkey/
: Might be nice if something likeREADME.txt
explained where all the.diff
files come from and why they're needed (particularly so that people can tell when they're not needed any more and can be deleted).
comment:31 by , 9 years ago
Replying to Philip:
Some initial comments on the ESR31 branch:
- When building (Ubuntu 14.04, gcc 4.8.2):
...
I guess we either want to fix our copy of tinygettext, or disable the warnings.
I've seen those and I agree that we have to fix/disable them, but they are related to C++11 and not SpiderMonkey.
source/scriptinterface/third_party/ObjectToIDMap.h
: The class isObjectIdCache
, so shouldn't this beObjectIdCache.h
?
They renamed the class to ObjectToIDMap in later versions of SpiderMonkey. I've first used the newer version and then copied the code from an older version. I think I'll leave it this way and it will match after the next upgrade.
LICENSE.txt
: Added trailing whitespace. Also, quite a few other changes to source files are adding trailing whitespace. (Not hugely important, but it makes "git diff" unhappy.)
I've fixed most of these now.
README.txt
: Need to be careful not to apply that change to SVN.
I've reverted the changes in README.txt just to be sure.
build/premake/premake4.lua
: Since the use of--with-c++11
was removed, the correspondingnewoption
line should be removed too.
That was only a quick change to enable c++11 for this branch. I'm going to make a separate commit that fixes some clang issues, removes generation of unsupported workspaces and removes some workarounds for GCC < 4.2.
build/workspaces/clean-workspaces.sh
: Should continue to deletemozjs24
(at least for a few months), else people who update a dirty workspace will havemozjs24
hanging around forever.
Done
libraries/osx/build-osx-libs.sh
: Why change to$CPPFLAGS
? That doesn't seem to be defined, and NSPR seems to include some C++ code so I'd guess it ought to still haveCXXFLAGS
set.
That's an issue, indeed. I've reverted an older version and haven't noticed the code has been changed in the meantime (r15410). Fixed.
libraries/source/spidermonkey/
: Might be nice if something likeREADME.txt
explained where all the.diff
files come from and why they're needed (particularly so that people can tell when they're not needed any more and can be deleted).
There's some description in build.sh and some patches contain some information in the header comment. All of them shouldn't be needed anymore in the next version. Two are backports and the third is a mix of backports and one new fix (tracelogger flushing). I've added some more information to these comments.
comment:33 by , 9 years ago
Replying to Philip:
Some initial comments on the ESR31 branch:
- When building (Ubuntu 14.04, gcc 4.8.2):
In file included from ../../../source/third_party/tinygettext/src/dictionary_manager.cpp:20:0: ../../../source/third_party/tinygettext/include/tinygettext/dictionary_manager.hpp:88:59: warning: ‘auto_ptr’ is deprecated (declared at /usr/include/c++/4.8/backward/auto_ptr.h:87) [-Wdeprecated-declarations] void set_filesystem(std::auto_ptr<FileSystem> filesystem); ^ ../../../source/third_party/tinygettext/src/dictionary_manager.cpp:237:72: warning: ‘auto_ptr’ is deprecated (declared at /usr/include/c++/4.8/backward/auto_ptr.h:87) [-Wdeprecated-declarations] DictionaryManager::set_filesystem(std::auto_ptr<FileSystem> filesystem_) ^I guess we either want to fix our copy of tinygettext, or disable the warnings.
#2522 is relevant and should fix those warnings.
comment:36 by , 9 years ago
Replying to stanislas69:
Do I need VS2013 ?
Yes, from now on you need VS2013 to build on Windows. We have decided that it's not worth the additional efforts and trouble to support older versions with C++11. It's already a challenge to support three different platforms and compilers (not only with C++11, but it gets a bit harder now).
comment:37 by , 9 years ago
Description: | modified (diff) |
---|
follow-ups: 39 40 41 comment:38 by , 9 years ago
IGUIObject.cpp
: IssetCompileAndGo
appropriate here? I have vague memories that it was intended for functions which you compile and then execute immediately and exactly once (so it can apply optimisations at compile time that would be unsafe if some other script had a chance to run and modify the context's global state before running the newly compiled function), but GUI event handlers will be called many times.
IGUIObject::SetScriptHandler
: The comment confused me, because there's only an indirect relationship between m_ScriptHandlers and the JSRuntime. Maybe something a bit like "m_ScriptHandlers is only rooted after SetGUI() has been called (which sets up the GC trace callbacks), so we can't safely store objects in it if the GUI hasn't been set yet." would be more explicit?
IGUIObject::TraceMember
: Since we're in the brave new world of C++11, this should probably befor (auto& handler : m_ScriptHandlers) JS_CallHeapObjectTracer(trc, &handler.second, "IGUIObject::m_ScriptHandlers");
IGUIObject.h
:m_ScriptRuntime
is never used.
NetServer.cpp
:- // Clear roots before deleting their context - m_GameAttributes = CScriptValRooted(); - m_SavedCommands.clear(); - SAFE_DELETE(m_ScriptInterface);
Hmm, why is that not needed any more? It looks likeJS::PersistentRooted
s are stored in a linked list in their runtime, and the runtime is a shared_ptr owned by m_ScriptInterface so it will be destroyed here. Is there something that stops the m_GameAttributes/m_SavedCommands destructors from accessing that linked list in the now-deleted runtime?
NativeWrapperDecls.h
: s/implicitely/implicitly/
NativeWrapperDecls.h
: s/converson/conversion/
NativeWrapperDecls.h
: A few CRLFs have crept into the file (around the "TODO: We allow ..." comment), while the rest of it is just LF. (At least on git on Linux.)
NativeWrapperDecls.h
: A comment above the declaration ofWrapperIfHandle
might be nice, to explain roughly what it means.
ScriptInterface::LoadScript
: The comment saysCompileOptions
doesn't copy the string; doesJS_CompileUCFunction
still copy it (sofilenameStr
only needs to stay alive until that function returns)? If so, I guess you could doJS_CompileUCFunction(..., JS::CompileOptions(m->m.cx).setFileAndLine(utf8_from_wstring(...).c_str(), ...).setCompileAndGo(true));
if you wanted to make the code a bit shorter (though I'm not sure whether that's a good idea or not).
ScriptInterface.h
: s/explicitely/explicitly/. But I think we're not supporting VS2010 any more, so can the move operator/constructor be removed entirely?
ScriptRuntime::jshook_function
: Hmm, that's removing the use ofLocFlyweight
, which I think will break the profiler. The profiler never copies strings, it just stores (and compares)const char*
pointers, and assumes pointer equality is equivalent to string equality. But the new code is allocating a new string every time it's called, so the profiler will have pointers to deleted memory and also won't be able to match up two instances of the same string.LocFlyweight
was added to make sure each string got allocated precisely once and remained valid forever (and also avoided any memory allocation cost for temporary strings in the cases where it could look up an already-allocated string), and I think it's still necessary.
ScriptRuntime.h
: Comment ofAddDeferredFinalizationObject
has unmatched)
.
ScriptTypes.h
: Two copies of-Wignored-qualifiers
.
ScriptTypes.h
: Seems pointless to do a load of#pragma GCC diagnostic
calls just before a#pragma GCC diagnostic pop
which will undo all those changes. Should delete everything except the pop. (I guess that's a relic of older code that tried to be compatible with GCCs that didn't support pop.)
ScriptVal.h
: s/explicitely/explicitly/, or just delete the move operator.
Simulation2.cpp
: s/All 500 turns/Every 500 turns/
CCmpAIManager::Trace
:for (auto& foo : bar)
again.
CCmpCommandQueue::Serialize
: Delete the declaration of tmpRoot.
ComponentManager.h
: s/explicitely/explicitly/, or just delete all the VS2010 stuff.
comment:39 by , 9 years ago
Replying to Philip:
IGUIObject.cpp
: IssetCompileAndGo
appropriate here? I have vague memories that it was intended for functions which you compile and then execute immediately and exactly once (so it can apply optimisations at compile time that would be unsafe if some other script had a chance to run and modify the context's global state before running the newly compiled function), but GUI event handlers will be called many times.
I did some research about CompileAndGo. My understanding is that it adds information specific to one global object to the compiled scirpts and it's not allowed to use these scripts with different global objects (but you may call them multiple times with the same global object). According to the description in bug 1095015, it's maybe even allowed to use different global objects, but that doesn't apply to us anyway. I'd say CompileAndGo is right there, but I'm not quite sure about what it really does exactly (when looking at bug 1095015 and related ones, I'd say not even Mozilla is quite sure about that and they are going to change it in the future).
IGUIObject::SetScriptHandler
: The comment confused me, because there's only an indirect relationship between m_ScriptHandlers and the JSRuntime. Maybe something a bit like "m_ScriptHandlers is only rooted after SetGUI() has been called (which sets up the GC trace callbacks), so we can't safely store objects in it if the GUI hasn't been set yet." would be more explicit?
Indeed, that comment is better. Changed.
IGUIObject::TraceMember
: Since we're in the brave new world of C++11, this should probably befor (auto& handler : m_ScriptHandlers) JS_CallHeapObjectTracer(trc, &handler.second, "IGUIObject::m_ScriptHandlers");
IGUIObject.h
:m_ScriptRuntime
is never used.
Changed.
NetServer.cpp
:- // Clear roots before deleting their context - m_GameAttributes = CScriptValRooted(); - m_SavedCommands.clear(); - SAFE_DELETE(m_ScriptInterface);Hmm, why is that not needed any more? It looks likeJS::PersistentRooted
s are stored in a linked list in their runtime, and the runtime is a shared_ptr owned by m_ScriptInterface so it will be destroyed here. Is there something that stops the m_GameAttributes/m_SavedCommands destructors from accessing that linked list in the now-deleted runtime?
I think you are right, I didn't find anything that prevents this from happening. The problem is essentially still the same, but now with the runtime instead of the context and now it happens in SpiderMonkey code instead of our code. I've fixed this and added some comments to other places where something similar could happen.
NativeWrapperDecls.h
: s/implicitely/implicitly/
NativeWrapperDecls.h
: s/converson/conversion/
Fixed.
NativeWrapperDecls.h
: A few CRLFs have crept into the file (around the "TODO: We allow ..." comment), while the rest of it is just LF. (At least on git on Linux.)
Should be fixed automatically when it's committed to SVN (svn:eol-style native).
NativeWrapperDecls.h
: A comment above the declaration ofWrapperIfHandle
might be nice, to explain roughly what it means.
Done.
comment:40 by , 9 years ago
Replying to Philip:
ScriptRuntime::jshook_function
: Hmm, that's removing the use ofLocFlyweight
, which I think will break the profiler. [...]
Fixed here along with multiple other isses with the profiler. I'm too busy/lazy to split that up into multiple patches and figure out what can be applied to SVN first. I will probably commit them together with the SpiderMonkey upgrade.
comment:41 by , 9 years ago
Replying to Philip:
ScriptRuntime.h
: Comment ofAddDeferredFinalizationObject
has unmatched)
.
ScriptTypes.h
: Two copies of-Wignored-qualifiers
.
ScriptTypes.h
: Seems pointless to do a load of#pragma GCC diagnostic
calls just before a#pragma GCC diagnostic pop
which will undo all those changes. Should delete everything except the pop. (I guess that's a relic of older code that tried to be compatible with GCCs that didn't support pop.)
ScriptVal.h
: s/explicitely/explicitly/, or just delete the move operator.
Simulation2.cpp
: s/All 500 turns/Every 500 turns/
CCmpAIManager::Trace
:for (auto& foo : bar)
again.
CCmpCommandQueue::Serialize
: Delete the declaration of tmpRoot.
ComponentManager.h
: s/explicitely/explicitly/, or just delete all the VS2010 stuff.
Fixed. I've removed the VS2010 compatibility workarounds.
comment:44 by , 9 years ago
Keywords: | review removed |
---|
comment:45 by , 9 years ago
The OS X Yosemite / clang build is very noisy due to warnings like these:
In file included from ../../../source/simulation2/components/CCmpPathfinder_Vertex.cpp:35: In file included from ../../../source/simulation2/components/CCmpPathfinder_Common.h:30: In file included from ../../../source/simulation2/system/Component.h:23: In file included from ../../../source/simulation2/system/ComponentManager.h:23: In file included from ../../../source/scriptinterface/ScriptInterface.h:25: In file included from ../../../source/scriptinterface/ScriptTypes.h:68: ../../../libraries/source/spidermonkey/include-unix-release/jsapi.h:645:1: warning: 'JSFreeOp' defined as a struct here but previously declared as a class [-Wmismatched-tags] struct JSFreeOp { ^ ../../../libraries/source/spidermonkey/include-unix-release/js/Class.h:27:1: note: did you mean struct here? class JSFreeOp; ^~~~~ struct In file included from ../../../source/gui/COList.cpp:18: In file included from ../../../source/gui/COList.h:23: In file included from ../../../source/gui/GUI.h:46: In file included from ../../../source/gui/GUIutil.h:42: In file included from ../../../source/gui/CGUI.h:43: In file included from ../../../source/scriptinterface/ScriptInterface.h:25: In file included from ../../../source/scriptinterface/ScriptTypes.h:68: In file included from ../../../libraries/source/spidermonkey/include-unix-release/jsapi.h:24: In file included from ../../../libraries/source/spidermonkey/include-unix-release/js/CallArgs.h:38: ../../../libraries/source/spidermonkey/include-unix-release/js/RootingAPI.h:1157:12: warning: class 'PersistentRootedMarker' was previously declared as a struct [-Wmismatched-tags] friend class js::gc::PersistentRootedMarker<T>; ^ ../../../source/scriptinterface/ScriptInterface.h:459:13: note: in instantiation of template class 'JS::PersistentRooted<JS::Value>' requested here handle.set(a); ^ In file included from ../../../source/ps/SavedGame.cpp:20: In file included from ../../../source/ps/SavedGame.h:21: In file included from ../../../source/scriptinterface/ScriptInterface.h:25: In file included from ../../../source/scriptinterface/ScriptTypes.h:68: In file included from ../../../libraries/source/spidermonkey/include-unix-release/jsapi.h:21: ../../../libraries/source/spidermonkey/include-unix-release/jsalloc.h:22:1: warning: class 'ContextFriendFields' was previously declared as a struct [-Wmismatched-tags] class ContextFriendFields; ^ ../../../libraries/source/spidermonkey/include-unix-release/jspubtd.h:326:8: note: previous use is here struct ContextFriendFields ^ ../../../libraries/source/spidermonkey/include-unix-release/jsalloc.h:22:1: note: did you mean struct here? class ContextFriendFields; ^~~~~ struct
If we can't fix them properly, they should probably be disabled.
comment:46 by , 9 years ago
I am not able to build with an external mozjs31 library. I specified --with-system-mozjs31 but after collada is built is starts building bundled spidermonkey. I always used the external spidermonkey with mozjs24.
I thought SpiderMonkey 3.1 will be released in July 2014?
"The next release will be SpiderMonkey 31, around July 2014." https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey