GNOME Bugzilla – Bug 690982
gjs: port to spidermonkey js188/esr17
Last modified: 2013-08-21 07:08:52 UTC
Just for kicks I tried to build gjs against the spidermonkey engine from Firefox 17ESR. Hoping to see if it might fix some of the JS leaks. Building spidermonkey from mercurial repos itself was rather trivial, there really is only one patch required to get a standalone version. Potentially this is something that could be done with jhbuild (except it requires autoconf2.13 to build configure script)?. There are however a lot of API changes in the 17ESR engine, here are a few that I have noticed so far. In particular I have no idea how to fix the removal of the JSCLASS_NEW_RESOLVE_GETS_START() flag. - Many typedefs have been removed. https://developer.mozilla.org/en-US/docs/SpiderMonkey/1.8.8#typedef_Changes - JSVAL_IS_OBJECT() has been removed. Instead use !JSVAL_IS_PRIMITIVE(v) to detect objects and JSVAL_IS_NULL(v) to detect null. - JSCLASS_NEW_RESOLVE_GETS_START() is removed - JS_SetPrivate(), JS_GetClass and a lot of other methods no longer takes context as 1st argument. - JS_NewCompartmentAndGlobalObject() is removed, Use JS_NewGlobalObject() instead. - JS_NewNumberValue appears to be changed to a static method JS_NumberValue(double) - JS_GC() epects runtime as argument instead of context.
Created attachment 232499 [details] [review] libname patch this patch is against the 17.0-1 RELEASE snapshot https://hg.mozilla.org/releases/mozilla-esr17/file/d8475f8bd98f
Created attachment 232501 [details] [review] filter-out %.$(LIBS_DESC_SUFFIX) this fixes an error in the install script (although there is still the issues of public headers from js/src/dist/include not getting installed)
Thank you for your work! The API changes seem trivial. You can safely forget about NEW_RESOLVE_GETS_START, nothing in gjs uses the start value.
Created attachment 232579 [details] [review] remove unused and obsolete flag. JSCLASS_NEW_RESOLVE_GETS_START has been removed in esr17, additionally gjs does not use the start value.
Review of attachment 232579 [details] [review]: OK
js/byteArray test is failing after the removal of JSCLASS_NEW_RESOLVE_GETS_START flag ;(
(In reply to comment #6) > js/byteArray test is failing after the removal of > JSCLASS_NEW_RESOLVE_GETS_START flag ;( byte_array_new_resolve is wrong in what is doing. *objp == obj in all interesting cases, because we ignore property resolves on the prototype (they wouldn't make sense anyway), so it should call priv_from_js(context, obj), and stop assuming that *objp is already set. Especially when it returns true without defining a property (which upsets the JS engine a lot).
Created attachment 232600 [details] [review] more typedef changes for types that have been removed from esr17
Created attachment 232603 [details] [review] api change to JS_NumberValue JS_NewNumberValue is depreciated and removed in js188
Created attachment 232680 [details] [review] remove unused and obsolete flag. JSCLASS_NEW_RESOLVE_GETS_START has been removed in esr17, additionally gjs does not use the start value.
Created attachment 232681 [details] [review] [PATCH 1/2] byteArray [PATCH 1/2] byteArray: Remove prototype checks from func implementations As we don't use JSCLASS_CONSTRUCT_PROTOTYPE anymore, we don't have a private on the prototype, and thus we can simply quit early here.
Created attachment 232682 [details] [review] bytearray: correct new resolve behaviour
Created attachment 232683 [details] [review] byteArray: Remove prototype checks from func implementations As we don't use JSCLASS_CONSTRUCT_PROTOTYPE anymore, we don't have a private on the prototype, and thus we can simply quit early here.
Created attachment 232684 [details] [review] bytearray: correct new resolve behaviour
Review of attachment 232600 [details] [review]: Needs a better commit message and a small whitespace fix, otherwise fine. ::: gjs/profiler.c @@ +49,2 @@ GjsProfileData *last_function_entered; /* weak ref to by_file */ + int64_t last_function_exit_time; Not aligned.
Review of attachment 232683 [details] [review]: I'll upload a better version of this.
Review of attachment 232684 [details] [review]: This one too.
Created attachment 232688 [details] [review] byteArray: Remove prototype checks from func implementations As we don't use JSCLASS_CONSTRUCT_PROTOTYPE anymore, we don't have a private on the prototype, which means that all instances of a NULL priv are the prototype. As far as I can tell, the "wrong class" case is impossible, as all of these functions are only installed on an object with a JSClass of ByteArray.
Created attachment 232689 [details] [review] byteArray: Do not use JSCLASS_NEW_RESOLVE_GETS_START It is a flag that has been removed in recent versions of libmozjs, and is not something that we should depend on. This is a simple case that tries to emulate the behavior of JSCLASS_NEW_RESOLVE_GETS_START; a future code cleanup will simplify code here more.
Created attachment 232690 [details] [review] byteArray: Clean up resolve op Only set the *objp value after success, leaving it at NULL otherwise.
Created attachment 232691 [details] [review] Don't use depreciated types Mozilla have removed jsdouble and int64 from js188, these are replaced with double and int64_t respectively. These changes are safe on js185 however.
Review of attachment 232691 [details] [review]: "deprecated", and OK
Review of attachment 232691 [details] [review]: commit 6c3b08843bf1bffb41666c6330b9dd3e43b817e0 Author: Tim Lunn <tim@feathertop.org> Date: Thu Jan 3 12:20:09 2013 +1100 Don't use deprecated types Mozilla have removed jsdouble and int64 from js188, these are replaced with double and int64_t respectively. These changes are safe on js185 however. https://bugzilla.gnome.org/show_bug.cgi?id=690982
Another API change is the introduction of these new types typedef struct { JSObject **_; } JSHandleObject; typedef struct { jsid *_; } JSHandleId; this change affects basically all callbacks, such as (* JSPropertyOp)(JSContext *cx, JSHandleObject obj, JSHandleId id, JSMutableHandleValue vp); Although I am not sure how to handle this change in a 'nice' way.
Review of attachment 232689 [details] [review]: I only vaguely understand this, to be honest. But I think it looks OK if you squash the second patch with this one; this patch on its own seems like it'd be buggy.
Review of attachment 232690 [details] [review]: To be squashed
Review of attachment 232688 [details] [review]: Sounds right.
Review of attachment 232689 [details] [review]: We discussed this on IRC, the patches look fine separately too.
Review of attachment 232690 [details] [review]: Or not squashed.
Attachment 232688 [details] pushed as cd83eff - byteArray: Remove prototype checks from func implementations Attachment 232689 [details] pushed as 27ccb39 - byteArray: Do not use JSCLASS_NEW_RESOLVE_GETS_START Attachment 232690 [details] pushed as 56b8b71 - byteArray: Clean up resolve op
Review of attachment 232680 [details] [review]: commit 947e260a0f6a60cad10811eddeddaa0f3ca22450 Author: Tim Lunn <tim@feathertop.org> Date: Fri Jan 4 08:01:21 2013 +1100 remove unused and obsolete flag. JSCLASS_NEW_RESOLVE_GETS_START has been removed in esr17, additionally gjs does not use the start value.
Created attachment 233117 [details] [review] JS_BufferIsCompilableUnit API change
Created attachment 233118 [details] [review] api change to JS_NumberValue JS_NewNumberValue is depreciated and removed in js188
Created attachment 233119 [details] [review] change build version to mozjs188
Created attachment 233120 [details] [review] define JSVAL_IS_OBJECT JSVAL_IS_OBJECT has been removed, for now just redefine it in compat.h
Created attachment 233121 [details] [review] JS_GetClass remove context argument JS_GetClass no longer takes *context as first argument, consequently the JS_GET_CLASS Macro is also removed.
Created attachment 233122 [details] [review] JS_SetPrivate/GetPrivate no longer require context argument
Created attachment 233123 [details] [review] JSCLass callbacks now use JSHandleObject/Id types All functions which are callbacks for JSClass, now have new types which wraps the Objects in an extra pointer. We need to cast and then derefence these objects.
Created attachment 233124 [details] [review] testBoxed: add test for getter
Created attachment 233125 [details] [review] update for JSClass prototype changes
Created attachment 233126 [details] [review] Misc API changes Enable E4X, GC functions take runtime, remove non-public header and some deprecated API.
Created attachment 233127 [details] [review] byteArray: update PropertySpec for new API For some reason the setter fails to get called when JSPROP_SHARED is set.
Review of attachment 233118 [details] [review]: So, while I don't want to have build hell, until js188 has an official tarball, I don't want to approve this. This seems like a very wrappable change, so how do you feel about a HAVE_JS_NEWNUMBERVALUE, and a JS_NewNumberValue wrapper in compat.h?
Review of attachment 233122 [details] [review]: ::: gi/boxed.c @@ +450,3 @@ object, priv); + proto = JS_GetPrototype(object); Mention the GetPrototype changes in the commit message, please.
(In reply to comment #43) > Review of attachment 233118 [details] [review]: > > So, while I don't want to have build hell, until js188 has an official tarball, > I don't want to approve this. This seems like a very wrappable change, so how > do you feel about a HAVE_JS_NEWNUMBERVALUE, and a JS_NewNumberValue wrapper in > compat.h? Sure, will do.
Created attachment 233270 [details] [review] JS_BufferIsCompilableUnit API change
Created attachment 233271 [details] [review] change build version to mozjs188
Created attachment 233272 [details] [review] JS_GetClass remove context argument JS_GetClass no longer takes *context as first argument, consequently the JS_GET_CLASS Macro is also removed.
Created attachment 233273 [details] [review] JS_SetPrivate/GetPrivate no longer require context argument
Created attachment 233274 [details] [review] JSCLass callbacks now use JSHandleObject/Id types All functions which are callbacks for JSClass, now have new types which wraps the Objects in an extra pointer. We need to cast and then derefence these objects.
Created attachment 233275 [details] [review] testBoxed: add test for getter
Created attachment 233276 [details] [review] update for JSClass prototype changes
Created attachment 233277 [details] [review] Misc API changes Enable E4X, remove non-public header and some deprecated API.
Created attachment 233278 [details] [review] byteArray: update PropertySpec for new API For some reason the setter fails to get called when JSPROP_SHARED is set.
Created attachment 233279 [details] [review] temporarily disable two failing tests
Created attachment 233280 [details] [review] Add wrappers for removed api, JS_NewNumberValue and JSVAL_IS_OBJECT()
Created attachment 233281 [details] [review] Replace context with runtime in GC Functions/Callback.
Gnome-shell is now running stable and with no obvious issues on js188.
Review of attachment 233275 [details] [review]: OK.
Review of attachment 233277 [details] [review]: ::: modules/jsUnit.js @@ +451,3 @@ // calls to setUp() and tearDown() function gjstestRun(window_, setUp, tearDown) { + var propName; Split this fix off and push it please.
Created attachment 233409 [details] [review] I don't have git access but here is the split off patch. Fix assignment to undeclared variable warning.
After a few days of testing this, I can report: - I haven't seen a single GC deadlock! Even with GC on idle re-enabled. - Memory usage is dramatically lower (w/ GC on idle). I am on ~180MB after 48hrs, previously that would have been atleast 400+MB (although it has been a while since I g-s has actually run that long, due to GC deadlocks). - Most of the leaks in mozjs appear to be gone.
Review of attachment 233409 [details] [review]: commit 20971324011b2504b715ee671bc541eb974ffb08 Author: Tim Lunn <tim@feathertop.org> Date: Mon Jan 14 13:09:21 2013 +1100 Fix assignment to undeclared variable warning.
Review of attachment 233275 [details] [review]: commit d03c9ab24eeb67390005d0d8d2daf16639dacf6a Author: Tim Lunn <tim@feathertop.org> Date: Tue Jan 8 07:51:05 2013 +1100 testBoxed: add test for getter
I pushed a lot of these patches to: http://git.gnome.org/browse/gjs/log/?h=wip/mozjs-188 Let's start iterating from there. The only patch that didn't apply is: http://bugzilla-attachments.gnome.org/attachment.cgi?id=233276
I have created a repo on github with all patches applied and some updated commit messages. https://github.com/darkxst/gjs-js188 As far as gjs is concerned this is mostly complete, I have not seen any issues. There are some compiler warnings however that could be silenced by casting callbacks in class declarations etc. Additionally polkit might need some attention, I think the only api change that appears to affect it is the type changes, however I had to jump through some hoops to get it building due to the dl_open() stuff.
When I tried to compile the branch I got: libtool: compile: x86_64-pc-linux-gnu-gcc -DHAVE_CONFIG_H -I. -pthread -I/usr/include/gobject-introspection-1.0 -I/usr/lib64/libffi-3.0.11/include -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/js -I/usr/include/nspr -DGJS_TOP_SRCDIR=\".\" -DGJS_JS_DIR=\"/usr/share/gjs-1.0\" -DGJS_NATIVE_DIR=\"/usr/lib64/gjs-1.0\" -DPKGLIBDIR=\"/usr/lib64/gjs\" -I./gi -DGJS_COMPILATION -Wall -Wmissing-prototypes -Wnested-externs -Wpointer-arith -Wno-sign-compare -O2 -march=native -pipe -ggdb -w -Wa,--compress-debug-sections -c gi/boxed.c -fPIC -DPIC -o .libs/libgjs_la-boxed.o gi/boxed.c: In function 'get_nested_interface_object': gi/boxed.c:623:24: error: incompatible type for argument 3 of 'JS_SetReservedSlot' /usr/include/js/jsapi.h:4822:1: note: expected 'jsval' but argument is of type 'int' gi/boxed.c:623:24: error: too many arguments to function 'JS_SetReservedSlot'/usr/include/js/jsapi.h:4822:1: note: declared here
Maciej, which branch? the wip branch is still missing a patch, however the github one should work.
(In reply to comment #68) > Maciej, which branch? the wip branch is still missing a patch, however the > github one should work. github one built but gnome-shell (3.6 as I'm trying to use it on 'production' system which crashes every few hours due to bug #688197) rebuilt against new version failed: JS ERROR: !!! Exception was: Error: No JS module 'dbus' found in search path JS ERROR: !!! message = '"No JS module 'dbus' found in search path"' JS ERROR: !!! fileName = '"/usr/share/gnome-shell/js/ui/keyboard.js"' JS ERROR: !!! lineNumber = '5' JS ERROR: !!! stack = '"@/usr/share/gnome-shell/js/ui/keyboard.js:5 @/usr/share/gnome-shell/js/ui/main.js:19 @<main>:1 "'
That's a completely unrelated error, and it's an incompatibility of gnome-shell 3.6 with gjs master. You can fix it fast by removing the imports.dbus line from /usr/share/gnome-shell/js/ui/main.js (it doesn't really use that module, it's a left over import)
Indeed I have not tried building 3.6 against the new js188/gjs stack, but would be interested to hear if it works out.
Sorry - I didn't wanted to spam the list but it looks like modifying keyboard.js everything works fine (however I haven't check the reproduction of bug yet). I've updated only glib, gir, spidermonkey and gjs.
Spidermonkey release should be coming fairly soon, there is a meta bug here tracking the patches that need to land first https://bugzilla.mozilla.org/show_bug.cgi?id=837921 I have a snapshot with most of these patches applied here https://github.com/darkxst/js17 gjs: the patchset here is basically complete and working, although the build version patch will need to be updated to use the new library naming (libmozjs-17.0). I also patched polkit to work with the esr17 spidermonkey, and submitted upstream these 2 bugs: https://bugs.freedesktop.org/show_bug.cgi?id=59830 https://bugs.freedesktop.org/show_bug.cgi?id=59781
At a high level what I fear with this is that particularly now that polkit requires mozjs, we're going to create major headaches for distributors. At least in Fedora for example, many things link to mozjs185. Stuff like libproxy, that is installed by default there. Now we don't of course need to port everything at once, and there's going to be some overlap time where there are 2 versions. One option is to create a medium term branch (we'd have to debate which is master) for the life of one GNOME release cycle. The other option of course is #ifdef but I suspect that'd be rather horrible. Does Ubuntu have any plans wrt this?
I think polkit would be easy enough to handle with ifdef's. For gjs branching is really the only option. Ubuntu will be shipping both mozjs versions, since there are a bunch of rdepends (outside of GNOME) that aren’t likely to be ported in a hurry.
I've pushed a rebased version of these patches to: https://git.gnome.org/browse/gjs/log/?h=wip/js17 Let's make that branch the canonical one?
Ok, I've merged this as: https://git.gnome.org/browse/gjs/log/?h=gnome-3-8-js17 And now I'm going to merge the changes to master.
...does this mean that 3.8 in Ubuntu 13.04 will ship with Mozjs17 by default? There's sort of a huge memory leak issue depending on it.
Chad, its in gnome3-staging ppa now, will get moved into normal gnome3 ppa after a bit more testing. Had hoped to get it into the Raring archives, but everything happened far to late for that to probably be possible.
That's cool. It's in *a* PPA that I can test and use. Thanx a ton!
I see it in the Gnome-Shell PPA. Question: does this build enable GC on idle? It starts G-S with *much* lower initial memory usage, but I'm still having the memory-creep-with-opening/closing-lotsa-windows issue on this build.
Yes it has the GC on idle re-enabled. I don't think anything has been done about the window clones leak issues
It has? Where?
Jasper, on the Gnome3 PPA, since we are running the new spidermonkey there.
It seems that this post is a huge success story for 3.8, at least on gentoo :-) https://bugs.gentoo.org/show_bug.cgi?id=480752#c4