Disable zealous GC in release builds.

[asajeffrey]

[jdm]

@bors-servo: r+

[bors-servo]

📌 Commit 049d78d has been approved by jdm

[bors-servo]

⌛ Testing commit 049d78d with merge 6abb142...

[jdm]

@bors-servo: r-

[michaelwu]

It's not enabled. An additional env flag is required to turn it on.

[bors-servo]

☀️ Test successful - travis

[asajeffrey]

Running the Dromaeo JS tests with and without gczeal enabled at compile-time... Without gczeal: http://i.imgur.com/iXNJf9e.png. With gczeal: http://i.imgur.com/FP17NH3.png. Looks like about 5% difference in performance on tests which perform allocation.

[jdm]

Ok! A measurable difference is a valid reason to turn it off, I think.

[jdm]

@bors-servo: r+

[bors-servo]

📌 Commit 049d78d has been approved by jdm

[michaelwu]

@bors-servo: r-
(don't think it matters but I'll r- to be sure)

There are functions which are only built when GC zeal is enabled, but which are specified in the bindings. You'll have to remove the GC zeal functions in the bindings to make this work everywhere. Unfortunately, the platforms we actually test on normally don't show this problem, but it can break windows and android.

I agree with this change, btw. It was only due to linking that zeal needed to be enabled everywhere, but I don't think we need to set the zeal from code. Setting zeal via env vars should be enough.

[asajeffrey]

Hmm, that's annoying. We're living with a 5% slowdown because of issues on platforms we don't test against. We should at least open an issue for this and leave it open until we fix the bindings.

[michaelwu]

Fix the bindings and this can land. This affects Android, Windows, and CEF bindings. I think you just need to delete the gc zeal functions in all the jsapi_* files.

[asajeffrey]

OK, I'll have a look at the jsapi files later today.

[asajeffrey]

@michaelwu are you meaning:

$ grep -ir zeal rust-mozjs/
rust-mozjs/src/jsapi_linux_32.rs: #[link_name = "Z12JS_GetGCZealP9JSContextPhPjS2"]
rust-mozjs/src/jsapi_linux_32.rs: pub fn JS_GetGCZeal(cx: *mut JSContext, zeal: *mut u8,
rust-mozjs/src/jsapi_linux_32.rs: #[link_name = "_Z12JS_SetGCZealP9JSContexthj"]
rust-mozjs/src/jsapi_linux_32.rs: pub fn JS_SetGCZeal(cx: *mut JSContext, zeal: u8, frequency: u32);
rust-mozjs/src/jsapi_macos_64.rs: #[link_name = "Z12JS_GetGCZealP9JSContextPhPjS2"]
rust-mozjs/src/jsapi_macos_64.rs: pub fn JS_GetGCZeal(cx: *mut JSContext, zeal: *mut u8,
rust-mozjs/src/jsapi_macos_64.rs: #[link_name = "_Z12JS_SetGCZealP9JSContexthj"]
rust-mozjs/src/jsapi_macos_64.rs: pub fn JS_SetGCZeal(cx: *mut JSContext, zeal: u8, frequency: u32);
rust-mozjs/src/lib.rs:pub const JS_DEFAULT_ZEAL_FREQ: u32 = 100;
rust-mozjs/src/jsapi_windows_gcc_64.rs: #[link_name = "Z12JS_GetGCZealP9JSContextPhPjS2"]
rust-mozjs/src/jsapi_windows_gcc_64.rs: pub fn JS_GetGCZeal(cx: *mut JSContext, zeal: *mut u8,
rust-mozjs/src/jsapi_windows_gcc_64.rs: #[link_name = "_Z12JS_SetGCZealP9JSContexthj"]
rust-mozjs/src/jsapi_windows_gcc_64.rs: pub fn JS_SetGCZeal(cx: *mut JSContext, zeal: u8, frequency: u32);
rust-mozjs/src/jsapi_linux_64.rs: #[link_name = "Z12JS_GetGCZealP9JSContextPhPjS2"]
rust-mozjs/src/jsapi_linux_64.rs: pub fn JS_GetGCZeal(cx: *mut JSContext, zeal: *mut u8,
rust-mozjs/src/jsapi_linux_64.rs: #[link_name = "_Z12JS_SetGCZealP9JSContexthj"]
rust-mozjs/src/jsapi_linux_64.rs: pub fn JS_SetGCZeal(cx: *mut JSContext, zeal: u8, frequency: u32);

Should I just put #[cfg(debugmozjs)] in front of each declaration?

[michaelwu]

According to https://github.com/servo/mozjs/blob/master/mozjs/js/src/jsapi.h#L4809 there's one function you're missing.

Don't make it conditional. Just delete it. The bindings generator can't generate conditional things so it's not worth it.

[asajeffrey]

OK, I removed them from the generated rust-mozjs/src/jsapi_ARCH.rs files, it works locally (where ARCH is linux_64). Are these files regenerated from mozjs/mozjs/js/src/jsapi.h? If so, should the zeal functions also be removed from jsapi.h, jsapi.hpp and jsapi.cpp?

[michaelwu]

No, don't modify the c++ files. The functions are already removed when gc zeal isn't enabled. If they weren't removed then we wouldn't have to remove them from the bindings.

[asajeffrey]

Will do. Of course, this is over in another repo, so I'll submit another PR.

[asajeffrey]

@michaelwu: I submitted a PR removing the gczeal bindings servo/rust-mozjs#212

[michaelwu]

@bors-servo r+

[bors-servo]

📌 Commit 049d78d has been approved by michaelwu

[asajeffrey]

@michaelwu: Thanks!

[larsbergstrom]

@bors-servo retry try- r=michaelwu

[bors-servo]

📌 Commit 049d78d has been approved by michaelwu

[bors-servo]

⌛ Testing commit 049d78d with merge 3ed24a9...

[bors-servo]

☀️ Test successful - travis