Closed Bug 463190 Opened 11 years ago Closed 11 years ago

TM: GC hangs when re-allocating double pool

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: gal, Assigned: gal)

References

Details

Attachments

(1 file, 3 obsolete files)

The logic to re-allocate the double pool looks overly complicated and seems to iloop under certain conditions. It should be greatly simplified. The entire didGC logic is unnecessary. If the pool can't be filled right away, just signal and error and don't fill it complete and don't enter the native code.
Attached patch patch (obsolete) — Splinter Review
Assignee: general → gal
Status: NEW → ASSIGNED
Attached patch v2 (obsolete) — Splinter Review
Attachment #346586 - Attachment is obsolete: true
Attachment #346588 - Flags: review?(brendan)
Attached patch v3 (obsolete) — Splinter Review
Attachment #346588 - Attachment is obsolete: true
Attachment #346591 - Flags: review?(brendan)
Attachment #346588 - Flags: review?(brendan)
Attached patch v4Splinter Review
Attachment #346591 - Attachment is obsolete: true
Attachment #346592 - Flags: review?(brendan)
Attachment #346591 - Flags: review?(brendan)
Comment on attachment 346592 [details] [diff] [review]
v4

>+            ptr = tm->recoveryDoublePool;
>+            if ((rt->gcNumber - gcNumber) > 1)

Over-paren nit, but maybe the right thing is to cast both sides like so:

            if (uint32(rt->gcNumber - gcNumber) > uint32(1))

and comment briefly about how the condition works even if rt->gcNumber wraps around to 0 (nothing keeps rt->gcNumber from wrapping to gcNumber but that's terribly unlikely).

>+                goto oom;
>+            continue;
>+        }
>+        ++ptr;
>+    }
>+    tm->recoveryDoublePoolPtr = ptr;
>+    return true;

One blank line here, please.

>+oom:
>+    /*
>+     * Already massive GC pressure, no need to hold doubles back
>+     * we won't run any native code anyway.
>+     */
>+    tm->recoveryDoublePoolPtr = tm->recoveryDoublePool;
>+    return false;
> }
[...]
>+/**
>+ * Checks whether the shape of the global object has changed and if so 
>+ * flushes the JIT cache.
>+ */
>+static inline bool
>+js_CheckGlobalObjectShape(JSContext* cx, JSTraceMonitor* tm, JSObject* globalObj)
>+{
>+    /* Check the global shape. */
>+    if (tm->globalSlots->length() && (OBJ_SHAPE(globalObj) != tm->globalShape)) {
>+        AUDIT(globalShapeMismatchAtEntry);
>+        debug_only_v(printf("Global shape mismatch (%u vs. %u), flushing cache.\n",
>+                            OBJ_SHAPE(globalObj), tm->globalShape);)
>+        js_FlushJITCache(cx);
>+        return true;
>+    }
>+    return false;

How about flipping return value convention to match other "Check" functions -- false for fail, true for pass.

>+        /* Make sure inner tree call will not run into an out-of-memory condition. */
>+        if (tm->recoveryDoublePoolPtr < tm->recoveryDoublePool + MAX_NATIVE_STACK_SLOTS) 
>+            if (!js_ReplenishRecoveryPool(cx, tm)) {
>+                js_AbortRecording(cx, "Couldn't call inner tree (out of memory)");
>+                return false; 
>+            }

The braces should go around the outer if's consequent, for dangling else avoidance and to match prevailing style (any multiline condition, then, or else causes bracing for all branches).

>+        
>+        /* Make sure the shape of the global object still matches (this might flush 
>+           the JIT cache). */
>+        JSObject* globalObj = JS_GetGlobalForObject(cx, cx->fp->scopeChain);
>+        if (js_CheckGlobalObjectShape(cx, tm, globalObj)) {
>+            js_AbortRecording(cx, "Couldn't call inner tree (prep failed)");
>+            return false;
>+        }

These two paragraphs could be factored into a common helper like the js_CheckIfFlushNeeded this patch removes. You could make it return js_CheckGlobalObjectShape(...) and test here, ignore later. Seems worth it for code sharing and footprint wins.

>+#ifdef DEBUG    
>     memset(stack_buffer, 0xCD, sizeof(stack_buffer));
>     memset(global, 0xCD, (globalFrameSize+1)*sizeof(double));
>+#endif    

Angels! ;-)

Good opt perf save here. Any more like it?

/be
http://hg.mozilla.org/tracemonkey/rev/1f5cc90151f5
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Comment on attachment 346592 [details] [diff] [review]
v4

r+ as landed.

/be
Attachment #346592 - Flags: review?(brendan) → review+
Flags: in-testsuite-
Flags: in-litmus-
You need to log in before you can comment on or make changes to this bug.