1449833 - Use StaticPrefs in nsJSEnvironment.cpp

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement)

Description

[Nicholas Nethercote [inactive]]

See bug 1448219 for details.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Bug 1449833 - Use StaticPrefs in nsJSEnvironment.cpp.

Review commit: https://reviewboard.mozilla.org/r/232366/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/232366/

Comment 2

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8963452 [details]
Bug 1449833 - Use StaticPrefs in nsJSEnvironment.cpp.

https://reviewboard.mozilla.org/r/232366/#review237788

::: dom/base/nsJSEnvironment.cpp:2321
(Diff revision 1)
>          }
>        }
>  
>        if (!sShuttingDown) {
> -        if (sPostGCEventsToObserver || Telemetry::CanRecordExtended()) {
> +        if (StaticPrefs::javascript_options_mem_notify() ||
> +            Telemetry::CanRecordExtended()) {

I don't know Gecko style that well, but in SpiderMonkey we would want the { on a separate line for a multi-line conditional.

::: modules/libpref/init/StaticPrefList.h:119
(Diff revision 1)
> +// appropriate pref is set.
> +#ifdef ANDROID
> +  // Disable the JS engine's GC on memory pressure, since we do one in the
> +  // mobile browser (bug 669346).
> +  // XXX: that GC appears to no longer be present. Should this value be
> +  //      changed, or the pref removed entirely?

I don't know what's going on here, but it seems important to make this sensible. I'll needinfo people for it.

Comment 3

[Steve Fink [:sfink] [:s:]]

jonco, smaug - this patch is r=me other than the question about what's going on with memory pressure-triggered GCs (see the XXX comment above). I'm checking if either of you are familiar with what we want to have happen there.

Comment 4

[Jon Coppeard (:jonco)]

(In reply to Steve Fink [:sfink] [:s:] from comment #2)
> I don't know Gecko style that well, but in SpiderMonkey we would want the {
> on a separate line for a multi-line conditional.

I think that is a SpiderMonkey thing, but it's something I've really come to appreciate.

> > +  // Disable the JS engine's GC on memory pressure, since we do one in the
> > +  // mobile browser (bug 669346).

Oh wow, I wasn't aware of this.  If possible we should make this work the same as the desktop browser, but someone who works on the mobile browser should check this is OK.

(Actually we should get rid of this GC and do compacting on demand, as suggested in bug 1421966).

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

{ doesn't go to its own line with 'if'

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

The weird Android code seems to live now in
https://searchfox.org/mozilla-central/rev/7e663b9fa578d425684ce2560e5fa2464f504b34/mobile/android/chrome/content/MemoryObserver.js#12-15

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

So at least the XXX comment should be tweaked a bit. Best would be to get rid of the explicit gc call in Android call and not have the special case in the prefs.

Comment 8

[Nicholas Nethercote [inactive]]

Comment on attachment 8963452 [details]
Bug 1449833 - Use StaticPrefs in nsJSEnvironment.cpp.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/232366/diff/1-2/

Comment 9

[Nicholas Nethercote [inactive]]

I have filed bug 1450787 as a follow-up to decide what to do with javascript.options.gc_on_memory_pressure, and updated the comment to point to that bug.

FWIW, the '{' on it's own line thing is quite useful when you have four space indents, but less useful when you have two space indents, which is the standard for Gecko.

With four space indents, the condition and body look very similar:

> if (condition1() &&
>     condition2()) {
>     body1()
>     body2()
> }

With two space indents, it's less of a problem:

> if (condition1() &&
>     condition2()) {
>   body1()
>   body2()
> }

Comment 10

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8963452 [details]
Bug 1449833 - Use StaticPrefs in nsJSEnvironment.cpp.

https://reviewboard.mozilla.org/r/232366/#review240400

Sorry for the delay, I hadn't noticed the patch had been updated.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/69f03ba7f761
Use StaticPrefs in nsJSEnvironment.cpp. r=sfink

Comment 12

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/69f03ba7f76178f01fa572385284f93946f380d6
Bug 1449833 - Use StaticPrefs in nsJSEnvironment.cpp. r=sfink

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/69f03ba7f761