1352948 - Shell's evalInWorker is incompatible with the JIT

Status: RESOLVED INVALID

(Core :: JavaScript Engine: JIT, defect, P1)

Description

[Lars T Hansen [:lth]]

Attachment: rc.js

This is probably not an important bug but I'm not sure.  I'm marking it s-s because the TC is spun off of a bug (bug 1352681) that is currently s-s.

The problem here seems to be that type analysis is not thread-safe, and running code on multiple workers in the shell exposes this problem, see TC.  Much of the evalInWorker code seems to be thread-unsafe already so nobody should be shocked by this but I'm bringing it up in case it turns out to be surprising after all.

Comment 1

[Jan de Mooij [:jandem]]

(In reply to Lars T Hansen [:lth] from comment #0)
> The problem here seems to be that type analysis is not thread-safe, and
> running code on multiple workers in the shell exposes this problem, see TC. 

Can you be more specific? If type analysis is not thread-safe that's a big deal because workers are somewhat similar to workers in the browser.

Comment 2

[Lars T Hansen [:lth]]

Running in a release shell on x64 it crashes after a few seconds (reliably, so far, though the stack is not always the same):

One stack:

* thread #2: tid = 0xdaa92, 0x0000000100593e0d js`js::TypeMonitorResult(JSContext*, JSScript*, unsigned char*, JS::Value const&) + 93, stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x0000000100593e0d js`js::TypeMonitorResult(JSContext*, JSScript*, unsigned char*, JS::Value const&) + 93
    frame #1: 0x00000001002e777c js`js::jit::DoTypeMonitorFallback(JSContext*, void*, js::jit::ICTypeMonitor_Fallback*, JS::Handle<JS::Value>, JS::MutableHandle<JS::Value>) + 492
    frame #2: 0x00000bb3a613296c

Another stack:

* thread #2: tid = 0xdd559, 0x00000001005a350d js`js::TypeMonitorResult(JSContext*, JSScript*, unsigned char*, JS::Value const&) [inlined] JSObject::isSingleton(this=0x0000000000000000) const at jsobj.h:164, stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x00000001005a350d js`js::TypeMonitorResult(JSContext*, JSScript*, unsigned char*, JS::Value const&) [inlined] JSObject::isSingleton(this=0x0000000000000000) const at jsobj.h:164 [opt]
    frame #1: 0x00000001005a350d js`js::TypeMonitorResult(JSContext*, JSScript*, unsigned char*, JS::Value const&) [inlined] js::TypeSet::ObjectType(obj=0x0000000000000000) at TypeInference-inl.h:157 [opt]
    frame #2: 0x00000001005a350d js`js::TypeMonitorResult(JSContext*, JSScript*, unsigned char*, JS::Value const&) [inlined] js::TypeSet::GetValueType(JS::Value const&) + 61 at TypeInference-inl.h:182 [opt]
    frame #3: 0x00000001005a34d0 js`js::TypeMonitorResult(cx=0x0000000101acd000, script=0x00000001036090d0, pc=":", rval=<unavailable>) + 32 at TypeInference.cpp:3363 [opt]
    frame #4: 0x00000001002f1f0c js`js::jit::DoTypeMonitorFallback(JSContext*, void*, js::jit::ICTypeMonitor_Fallback*, JS::Handle<JS::Value>, JS::MutableHandle<JS::Value>) [inlined] js::TypeScript::Monitor(cx=0x0000000101acd000, pc=<unavailable>, rval=0x000070000e3b5398) + 14 at TypeInference-inl.h:575 [opt]
    frame #5: 0x00000001002f1efe js`js::jit::DoTypeMonitorFallback(cx=0x0000000101acd000, payload=<unavailable>, stub=0x0000000103a076c8, value=<unavailable>, res=<unavailable>) + 478 at SharedIC.cpp:2317 [opt]
    frame #6: 0x00002701d65506ff

No luck so far crashing in a debug build although there are some fascinating differences in the process monitor CPU utilization graph from run to run in debug builds - some runs pin all the cores immediately, other runs have pronounced sawtooth behavior for quite a long time, maybe GC.

Comment 3

[Jan de Mooij [:jandem]]

Hm I wonder if deserialize/serialize is buggy. We definitely shouldn't be crashing like that though.

Marking sec-high for now so this stays on the radar.

Comment 4

[Marcia Knous [:marcia]]

Tracking 55+ for this sec high issue.

Comment 5

[Lars T Hansen [:lth]]

Attachment: rcx.js

Here's a variant of the test that does not use serialize/deserialize but which also has some weird behavior.  It doesn't crash, but all the workers terminate with the message:

  uncaught exception: out of memory
  (Unable to print stack trace)

before the main thread soldiers on by itself.  That's on x64 in both release and debug.  It's a weird error since the test case does not have any recursion.  The problem is not the print statement.

(Unboxed array storage overflowing the stack?)

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Patch

There's an OOM bug in JSStructuredCloneReader::readSharedArrayBuffer.

If OOM is really not expected here we should check why we're reporting OOM.

Comment 7

[Lars T Hansen [:lth]]

Comment on attachment 8854344 [details] [diff] [review]
Patch

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

Thanks.  There are several bugs in how RCs are handled for SAB and SC, see bug 1352681, hoping to land those patches this week.  Looks like I caught this problem there as well.

Comment 8

[Jan de Mooij [:jandem]]

Maybe we can fold this with your other patches so we don't have to track/land this separately?

Comment 9

[Lars T Hansen [:lth]]

Yeah, I can try to land whatever's left here when I land the others.

Comment 10

[Lars T Hansen [:lth]]

The logic in the current patch on this bug is superseded by the logic in bug 1352681.

However, with those fixes in place the test cases above both still fail in the workers with stack overflow messages (x64 release).

Comment 11

[Lars T Hansen [:lth]]

Attachment: rcy.js

The current failures don't appear to have anything with SharedArrayBuffer.  Consider the test case rcy.js, attached.  The OOM on the helper threads originates in GCRuntime::tryNewTenuredThing<JSObject,AllowGC> with a thingSize of 32, and it happens because, even after a last-ditch GC, we can't allocate the thing.  (Happens in x64 debug build so straightforward to debug, I set a breakpoint in ReportOutOfMemory.)  But the JS appears only to be using about 100+ MB of RAM, so we should not be near any limits.  So something strange is going on here, could be related to the shell workers of course.

Comment 12

[Jon Coppeard (:jonco)]

(In reply to Lars T Hansen [:lth] from comment #11)
It looks like shell workers have a max heap size of 8MB so I'd say this is expected.

Comment 13

[Lars T Hansen [:lth]]

Nothing further to do here.  The OOM check landed separately and the other symptoms here appear to have reasonable explanations.  Closing this as INVALID for that reason.

Comment 14

[Lars T Hansen [:lth]]

Marking unaffected per comment 13.