Closed Bug 980630 Opened 6 years ago Closed 6 years ago

Remove type nuking

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: bhackett, Assigned: bhackett)

References

Details

Attachments

(1 file)

Attached patch patchSplinter Review
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.
Attachment #8387201 - Flags: review?(jdemooij)
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 :)
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 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.
Attachment #8387201 - Flags: review?(jdemooij) → review+
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
Oops, sorry.  While fixing a GGC build break I broke everything else.

https://hg.mozilla.org/integration/mozilla-inbound/rev/6d54e6b70eb2
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
https://hg.mozilla.org/mozilla-central/rev/7d406c9622a1
Assignee: nobody → bhackett1024
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Duplicate of this bug: 953289
You need to log in before you can comment on or make changes to this bug.