980630 - Remove type nuking

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

Attachment: patch

When an OOM occurs during inference we nuke all type information in the zone and disable inference.  This behavior is incompatible with bug 972817, and it isn't really necessary anymore: it was originally in place because with a web of interdependent constraints we are left in an inconsistent state if an OOM occurs in the middle of solving those constraints, but now constraints are not interdependent and can only trigger recompilation of particular scripts or invalidation of new script information.

The attached patch removes type nuking, and instead relies on other mechanisms such as marking type sets as unknown when OOM occurs.  Inference APIs are generally left alone, and still generally infallible.  There are a few places where OOM can leave us in a place that isn't easy to recover from, such as when copying data during GC sweeping or when adding scripts to the pending recompilation list.  In these places we just MOZ_CRASH; it doesn't seem worth it to add a new mechanism to recover here when crashes here aren't easy to trigger and the rest of the browser just crashes on OOM anyways.

Comment 1

[Christian Holler (:decoder)]

Can we please not add any calls to MOZ_CRASH due to OOM, without outputting an appropriate message before that? This makes it easier for automation to detect these cases and ignore such intended crashes. Here's an example how GGC does this:

http://mxr.mozilla.org/mozilla-central/source/js/src/gc/Verifier.cpp#887

> Assertion failure: [unhandlable oom] Failed to allocate for MonoTypeBuffer::put., at ...

Sticking to this format with [unhandlable oom] would be nice :)

Comment 2

[Andrew McCreight [:mccr8]]

There is also NS_ABORT_OOM(), which takes the number of bytes of the attempted allocation as an argument.  Right now, this just does MOZ_CRASH(), but I'm sure it could be made to print something out in a debug build or whatever.  I guess it isn't available in SpiderMonkey.

Comment 3

[Jan de Mooij [:jandem]]

Comment on attachment 8387201 [details] [diff] [review]
patch

Review of attachment 8387201 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great. r=me with comments addressed and the MOZ_CRASH() calls fixed.

It seems we could just move js::gc::CrashAtUnhandlableOOM() into the "js::" namespace and out of gc/Verifier.cpp, and call it here too.

::: js/src/jsinfer.cpp
@@ +2537,5 @@
>  
>      if (!objectTypeTable) {
>          objectTypeTable = cx->new_<ObjectTypeTable>();
>          if (!objectTypeTable || !objectTypeTable->init()) {
>              objectTypeTable = nullptr;

Pre-existing nit, but add js_delete(objectTypeTable); here to avoid a leak if init fails.

@@ +2629,5 @@
>  
>      if (!objectTypeTable) {
>          objectTypeTable = cx->new_<ObjectTypeTable>();
>          if (!objectTypeTable || !objectTypeTable->init()) {
>              objectTypeTable = nullptr;

Same here.

@@ +4009,4 @@
>                      *pentry = object;
> +                } else {
> +                    *oom = true;
> +                }

Nit: no {}

::: js/src/jsstr.cpp
@@ +3208,5 @@
>                      if (!sub || !splits.append(StringValue(sub)))
>                          return nullptr;
>                  } else {
>                      /* Only string entries have been accounted for so far. */
> +                    if (cx->typeInferenceEnabled())

This "if" is unnecessary as AddTypeProperty also checks this.

Comment 4

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/295dc1e971bf

Comment 5

[Doug Turner (:dougt)]

make[5]: Leaving directory `/home/buildslave/work/ubuntu12_04_64/build/obj-buildbot-auto/toolkit/crashreporter/google-breakpad/src/common/dwarf'
In file included from /home/buildslave/work/ubuntu12_04_64/build/js/src/gc/Marking.cpp:19:0,
                 from /home/buildslave/work/ubuntu12_04_64/build/obj-buildbot-auto/js/src/Unified_cpp_js_src1.cpp:67:
/home/buildslave/work/ubuntu12_04_64/build/js/src/jsinferinlines.h: In function ‘void js::types::EnsureTrackPropertyTypes(JSContext*, JSObject*, jsid)’:
/home/buildslave/work/ubuntu12_04_64/build/js/src/jsinferinlines.h:386:94: error: ‘CrashAtUnhandlableOOM’ was not declared in this scope
make[5]: *** [Unified_cpp_js_src1.o] Error 1
make[5]: Leaving directory `/home/buildslave/work/ubuntu12_04_64/build/obj-buildbot-auto/browser/app'
In file included from /home/buildslave/work/ubuntu12_04_64/build/js/src/jit/VMFunctions.cpp:19:0,
                 from /home/buildslave/work/ubuntu12_04_64/build/obj-buildbot-auto/js/src/Unified_cpp_js_src5.cpp:15:
/home/buildslave/work/ubuntu12_04_64/build/js/src/jsinferinlines.h: In function ‘void js::types::EnsureTrackPropertyTypes(JSContext*, JSObject*, jsid)’:
/home/buildslave/work/ubuntu12_04_64/build/js/src/jsinferinlines.h:386:94: error: ‘CrashAtUnhandlableOOM’ was not declared in this scope
make[5]: *** [Unified_cpp_js_src5.o] Error 1
make[5]: Leaving directory `/home/buildslave/work/ubuntu12_04_64/build/obj-buildbot-auto/webapprt/gtk2'
In file included from /home/buildslave/work/ubuntu12_04_64/build/js/src/jit/Ion.cpp:50:0,
                 from /home/buildslave/work/ubuntu12_04_64/build/obj-buildbot-auto/js/src/Unified_cpp_js_src3.cpp:41:
/home/buildslave/work/ubuntu12_04_64/build/js/src/jsinferinlines.h: In function ‘void js::types::EnsureTrackPropertyTypes(JSContext*, JSObject*, jsid)’:
/home/buildslave/work/ubuntu12_04_64/build/js/src/jsinferinlines.h:386:94: error: ‘CrashAtUnhandlableOOM’ was not declared in this scope
make[5]: *** [Unified_cpp_js_src3.o] Error 1
In file included from /home/buildslave/work/ubuntu12_04_64/build/js/src/jit/Lowering.cpp:18:0,
                 from /home/buildslave/work/ubuntu12_04_64/build/obj-buildbot-auto/js/src/Unified_cpp_js_src4.cpp:15:
/home/buildslave/work/ubuntu12_04_64/build/js/src/jsinferinlines.h: In function ‘void js::types::EnsureTrackPropertyTypes(JSContext*, JSObject*, jsid)’:
/home/buildslave/work/ubuntu12_04_64/build/js/src/jsinferinlines.h:386:94: error: ‘CrashAtUnhandlableOOM’ was not declared in this scope
make[5]: *** [Unified_cpp_js_src4.o] Error 1
In file included from /home/buildslave/work/ubuntu12_04_64/build/js/src/jsobjinlines.h:22:0,
                 from /home/buildslave/work/ubuntu12_04_64/build/js/src/jit/AsmJSLink.cpp:26,
                 from /home/buildslave/work/ubuntu12_04_64/build/obj-buildbot-auto/js/src/Unified_cpp_js_src2.cpp:2:
/home/buildslave/work/ubuntu12_04_64/build/js/src/jsinferinlines.h: In function ‘void js::types::EnsureTrackPropertyTypes(JSContext*, JSObject*, jsid)’:
/home/buildslave/work/ubuntu12_04_64/build/js/src/jsinferinlines.h:386:94: error: ‘CrashAtUnhandlableOOM’ was not declared in this scope
make[5]: *** [Unified_cpp_js_src2.o] Error 1

Comment 6

[Brian Hackett [Laid off!]]

Oops, sorry.  While fixing a GGC build break I broke everything else.

https://hg.mozilla.org/integration/mozilla-inbound/rev/6d54e6b70eb2

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

And out:

https://hg.mozilla.org/integration/mozilla-inbound/rev/64b582140fc1

https://tbpl.mozilla.org/php/getParsedLog.php?id=35845447&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=35845281&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=35845256&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=35845298&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=35845242&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=35845240&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=35845282&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=35845293&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=35845244&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=35845254&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=35845247&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=35845287&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=35845294&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=35845289&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=35845252&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=35845243&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=35845272&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=35845241&tree=Mozilla-Inbound

Comment 8

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7d406c9622a1

Comment 9

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/7d406c9622a1