Closed
Bug 463190
Opened 16 years ago
Closed 16 years ago
TM: GC hangs when re-allocating double pool
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: gal, Assigned: gal)
References
Details
Attachments
(1 file, 3 obsolete files)
11.93 KB,
patch
|
brendan
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•16 years ago
|
||
Assignee: general → gal
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•16 years ago
|
||
Attachment #346586 -
Attachment is obsolete: true
Assignee | ||
Updated•16 years ago
|
Attachment #346588 -
Flags: review?(brendan)
Assignee | ||
Comment 3•16 years ago
|
||
Attachment #346588 -
Attachment is obsolete: true
Attachment #346591 -
Flags: review?(brendan)
Attachment #346588 -
Flags: review?(brendan)
Assignee | ||
Comment 4•16 years ago
|
||
Attachment #346591 -
Attachment is obsolete: true
Attachment #346592 -
Flags: review?(brendan)
Attachment #346591 -
Flags: review?(brendan)
Comment 5•16 years ago
|
||
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
Assignee | ||
Comment 6•16 years ago
|
||
http://hg.mozilla.org/tracemonkey/rev/1f5cc90151f5
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Comment 7•16 years ago
|
||
Comment on attachment 346592 [details] [diff] [review] v4 r+ as landed. /be
Attachment #346592 -
Flags: review?(brendan) → review+
Updated•16 years ago
|
Flags: in-testsuite-
Flags: in-litmus-
You need to log in
before you can comment on or make changes to this bug.
Description
•