Closed Bug 488731 Opened 16 years ago Closed 15 years ago

Avoid shape regeneration and property cache purge during the GC

Categories

(Core :: JavaScript Engine, defect, P1)

Product:
Core
Component:
JavaScript Engine
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.9.2a1

People

(Reporter: igor, Assigned: brendan)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 8 obsolete files)

refreshed to tm tip, with shutdown leak of rt->nativeEnumerators fixed
12.14 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
+++ This bug was initially created as a clone of Bug #488414 +++ Currently GC always purge the property cache and regenerate all scope shapes. This implies flushing of the JITed code and deoptimizing currently running scripts. We should consider changing that and regenerating the shapes only when we are really running out of them. AFAICS most of the complexity doing that comes from tracking the property cache entries containig the GC things, like object keys or function values or JSScopeProperty instances. Such entries together with corresponding JIT-ed traces must be released if the GC things they contains are freed.
A simple heuristic for the GC to decide if its time to regenerate shapes is to check how close the last shape number to the overflow limit. If it is not, then shapes should stay. In that case, after the mark and finalization stage but before the sweeping, the GC can scan all the property cache entries and invalidate any of them that points to now dead GC things. Since the cache contains just 4096 entries per thread, its scanning should not affect the GC time given that the working set for the GC is much bigger. Then the GC should also somehow find all the traces that embeds the dead GC things from the cache or scripts and invalidate them. I am not sure how to that efficiently. One possibility is for the recorder to create a separated descriptor for such trace. The descriptor can just point to the offsets in the trace where pointers to the GC things are stored. This way the GC can enumerate the descriptor and check if they refer to dead things.
Assignee: general → gal
Attached patch patch (obsolete) — — Splinter Review
Don't sweep the property tree and regenerate shapes, except if the property cache was disabled due to overflowing the shape numbering. We probably want to sweep a bit earlier already, i.e. if a certain number of deletions are reached, but I am uploading this patch early to get some feedback from igor and see how much more is needed to make this work.
Attachment #386227 - Attachment is patch: true
Attachment #386227 - Attachment mime type: application/octet-stream → text/plain
The patch dies in js_Enumerate. It seems we yanked out some cached value from under it during GC but the shape didn't change, which isn't detected properly. I will look into it tomorrow. Any hints are very welcome. (gdb) up #1 0x000b4194 in js_Enumerate (cx=0x30bc80, obj=0x2d3260, enum_op=JSENUMERATE_INIT, statep=0xbfffecf8, idp=0x0) at ../jsobj.cpp:4907 4907 JS_ASSERT(ne->length >= 1); (gdb) list 4902 * We can safely read ne->shape without taking the GC lock as 4903 * ne is deleted only when running the GC and ne->shape is 4904 * read-only after initialization. 4905 */ 4906 ne = (JSNativeEnumerator *) *cachep; 4907 JS_ASSERT(ne->length >= 1); 4908 if (ne->shape == shape) { 4909 /* 4910 * Check that ne is not running with another enumerator 4911 * and, if so, reuse and mark it as running from now. (gdb) up #2 0x0009e3a1 in InitNativeIterator (cx=0x30bc80, iterobj=0x2d3280, obj=0x2d3260, flags=1) at ../jsiter.cpp:150 150 OBJ_ENUMERATE(cx, obj, JSENUMERATE_INIT, &state, NULL); (gdb) down #1 0x000b4194 in js_Enumerate (cx=0x30bc80, obj=0x2d3260, enum_op=JSENUMERATE_INIT, statep=0xbfffecf8, idp=0x0) at ../jsobj.cpp:4907 4907 JS_ASSERT(ne->length >= 1); (gdb) p ne.length $1 = 0 (gdb) p *ne $2 = { cursor = 9360784, length = 0, shape = 9360752, next = 0x0, ids = {9360720} } (gdb) list 4902 * We can safely read ne->shape without taking the GC lock as 4903 * ne is deleted only when running the GC and ne->shape is 4904 * read-only after initialization. 4905 */ 4906 ne = (JSNativeEnumerator *) *cachep; 4907 JS_ASSERT(ne->length >= 1); 4908 if (ne->shape == shape) { 4909 /* 4910 * Check that ne is not running with another enumerator 4911 * and, if so, reuse and mark it as running from now. (gdb) 4912 */ 4913 length = ne->length; 4914 if (js_CompareAndSwap(&ne->cursor, 0, length)) 4915 break; 4916 length = 0; 4917 } 4918 ne = NULL; 4919 } 4920 ENUM_CACHE_METER(nativeEnumMisses); 4921 (gdb)
Attachment #386227 - Flags: review?(igor)
Comment on attachment 386227 [details] [diff] [review] patch Asking for review for feedback only, the patch is definitively not ready yet.
(In reply to comment #2) > Created an attachment (id=386227) [details] I do not see why the patch needs to disable sweeping of the sure why the patch needs to disable sweeping of properties. Is it just not to deal with tracking the property cache entries and traces embedding them? If so, this invites for leaks.
My understanding is that if we sweep the property tree, we also have to re-number the shapes. I might be wrong.
Attachment #386227 - Attachment is obsolete: true
Attachment #386227 - Flags: review?(igor)
Attachment #386301 - Attachment is patch: true
Attachment #386301 - Attachment mime type: application/octet-stream → text/plain
(In reply to comment #6) > My understanding is that if we sweep the property tree, we also have to > re-number the shapes. I might be wrong. You're wrong :-P. /be
(In reply to comment #6) > My understanding is that if we sweep the property tree, we also have to > re-number the shapes. No, this is not true. Sweeping means deleting unreferenced JSScope and JSScopeProperty instances. That does not imply that we need to re-shape the property try. > Created an attachment (id=386301) [details] > more narrowly disable just shape regeneration (still crashing on trace-test) The patch does not protect JSScopeProperty instances that are embedded in the trace form being deleted during the sweep phase. Perhaps that is the reason for the crash?
Attachment #386301 - Attachment is obsolete: true
The crash was due to the enumcache, which I didn't flush because I thought its only flushed due to the renumbering, but seems not. This patch gets us further into trace-tests, but then massively explodes: TEST-UNEXPECTED-FAIL | trace-test.js | Math.cos(0.7853981633974) : expected number ( 0.7071067811865 ) != actual number ( 0.7853981633974 ) TEST-UNEXPECTED-FAIL | trace-test.js | Math.cos(1.570796326795) : expected number ( 0 ) != actual number ( 1.570796326795 ) TEST-UNEXPECTED-FAIL | trace-test.js | Math.cos(2.356194490192) : expected number ( -0.7071067811865 ) != actual number ( 2.356194490192 ) TEST-UNEXPECTED-FAIL | trace-test.js | Math.cos(3.14159265359) : expected number ( -1 ) != actual number ( 3.14159265359 ) TEST-UNEXPECTED-FAIL | trace-test.js | Math.cos(3.926990816987) : expected number ( -0.7071067811865 ) != actual number ( 3.926990816987 ) TEST-UNEXPECTED-FAIL | trace-test.js | Math.cos(4.712388980385) : expected number ( 0 ) != actual number ( 4.712388980385 ) TEST-UNEXPECTED-FAIL | trace-test.js | Math.cos(5.497787143782) : expected number ( 0.7071067811865 ) != actual number ( 5.497787143782 ) TEST-UNEXPECTED-FAIL | trace-test.js | Math.cos(Math.PI*2) : expected number ( 1 ) != actual number ( 6.283185307179586 ) TEST-UNEXPECTED-FAIL | trace-test.js | Math.cos(Math.PI/4) : expected number ( 0.7071067811865476 ) != actual number ( 0.7853981633974483 ) TEST-UNEXPECTED-FAIL | trace-test.js | Math.cos(Math.PI/2) : expected number ( 0 ) != actual number ( 1.5707963267948966 ) TEST-UNEXPECTED-FAIL | trace-test.js | Math.cos(3*Math.PI/4) : expected number ( -0.7071067811865476 ) != actual number ( 2.356194490192345 ) TEST-UNEXPECTED-FAIL | trace-test.js | Math.cos(Math.PI) : expected number ( -1 ) != actual number ( 3.141592653589793 ) TEST-UNEXPECTED-FAIL | trace-test.js | Math.cos(5*Math.PI/4) : expected number ( -0.7071067811865476 ) != actual number ( 3.9269908169872414 ) TEST-UNEXPECTED-FAIL | trace-test.js | Math.cos(3*Math.PI/2) : expected number ( 0 ) != actual number ( 4.71238898038469 )
If we allow sweeping of properties, we might delete sprops, which means we would have to flush all traces that embed them (none right now, but jason might change that soon). Or we don't embed sprops.
TEST-UNEXPECTED-FAIL | trace-test.js | Math.ceil('-0') : expected number ( -0 ) != actual number ( 0 ) TEST-PASS | trace-test.js | Infinity/Math.ceil('0') TEST-PASS | trace-test.js | Infinity/Math.ceil('-0') TEST-PASS | trace-test.js | Math.ceil(0) TEST-UNEXPECTED-FAIL | trace-test.js | Math.ceil(-0) : expected number ( -0 ) != actual number ( 0 ) TEST-PASS | trace-test.js | Infinity/Math.ceil(0) TEST-PASS | trace-test.js | Infinity/Math.ceil(-0) TEST-PASS | trace-test.js | Math.ceil(Number.POSITIVE_INFINITY) TEST-UNEXPECTED-FAIL | trace-test.js | Math.ceil(Number.NEGATIVE_INFINITY) : expected number ( -Infinity ) != actual number ( Infinity ) TEST-PASS | trace-test.js | Math.ceil(-Number.MIN_VALUE) TEST-PASS | trace-test.js | Infinity/Math.ceil(-Number.MIN_VALUE) TEST-PASS | trace-test.js | Math.ceil(1) TEST-UNEXPECTED-FAIL | trace-test.js | Math.ceil(-1) : expected number ( -1 ) != actual number ( 1 ) TEST-UNEXPECTED-FAIL | trace-test.js | Math.ceil(-0.9) : expected number ( -0 ) != actual number ( 0.9 ) TEST-PASS | trace-test.js | Infinity/Math.ceil(-0.9) TEST-UNEXPECTED-FAIL | trace-test.js | Math.ceil(0.9) : expected number ( 1 ) != actual number ( 0.9 ) TEST-UNEXPECTED-FAIL | trace-test.js | Math.ceil(-1.1) : expected number ( -1 ) != actual number ( 1.1 ) TEST-UNEXPECTED-FAIL | trace-test.js | Math.ceil(1.1) : expected number ( 2 ) != actual number ( 1.1 ) TEST-PASS | trace-test.js | Math.ceil(Number.POSITIVE_INFINITY) TEST-UNEXPECTED-FAIL | trace-test.js | Math.ceil(Number.NEGATIVE_INFINITY) : expected number ( -Infinity ) != actual number ( Infinity ) TEST-PASS | trace-test.js | Math.ceil(-Number.MIN_VALUE) TEST-PASS | trace-test.js | Math.ceil(1) TEST-UNEXPECTED-FAIL | trace-test.js | Math.ceil(-1) : expected number ( -1 ) != actual number ( 1 ) TEST-UNEXPECTED-FAIL | trace-test.js | Math.ceil(-0.9) : expected number ( -0 ) != actual number ( 0.9 ) TEST-UNEXPECTED-FAIL | trace-test.js | Math.ceil(0.9) : expected number ( 1 ) != actual number ( 0.9 ) TEST-UNEXPECTED-FAIL | trace-test.js | Math.ceil(-1.1) : expected number ( -1 ) != actual number ( 1.1 ) TEST-UNEXPECTED-FAIL | trace-test.js | Math.ceil(1.1) : expected number ( 2 ) != actual number ( 1.1 ) TEST-PASS | trace-test.js | Math.cos(void 0) TEST-UNEXPECTED-FAIL | trace-test.js | Math.cos(false) : expected number ( 1 ) != actual number ( 0 ) TEST-UNEXPECTED-FAIL | trace-test.js | Math.cos(null) : expected number ( 1 ) != actual number ( 0 ) TEST-UNEXPECTED-FAIL | trace-test.js | Math.cos('0') : expected number ( 1 ) != actual number ( 0 ) TEST-UNEXPECTED-FAIL | trace-test.js | Math.cos("Infinity") : expected number ( NaN ) != actual number ( Infinity ) TEST-UNEXPECTED-FAIL | trace-test.js | Math.cos('3.14159265359') : expected number ( -1 ) != actual number ( 3.14159265359 ) TEST-PASS | trace-test.js | Math.cos(Number.NaN) TEST-UNEXPECTED-FAIL | trace-test.js | Math.cos(0) : expected number ( 1 ) != actual number ( 0 ) TEST-UNEXPECTED-FAIL | trace-test.js | Math.cos(-0) : expected number ( 1 ) != actual number ( 0 ) TEST-UNEXPECTED-FAIL | trace-test.js | Math.cos(Number.POSITIVE_INFINITY) : expected number ( NaN ) != actual number ( Infinity ) TEST-UNEXPECTED-FAIL | trace-test.js | Math.cos(Number.NEGATIVE_INFINITY) : expected number ( NaN ) != actual number ( Infinity ) TEST-UNEXPECTED-FAIL | trace-test.js | Math.cos(0.7853981633974) : expected number ( 0.7071067811865 ) != actual number ( 0.7853981633974 ) TEST-UNEXPECTED-FAIL | trace-test.js | Math.cos(1.570796326795) : expected number ( 0 ) != actual number ( 1.570796326795 ) TEST-UNEXPECTED-FAIL | trace-test.js | Math.cos(2.356194490192) : expected number ( -0.7071067811865 ) != actual number ( 2.356194490192 ) TEST-UNEXPECTED-FAIL | trace-test.js | Math.cos(3.14159265359) : expected number ( -1 ) != actual number ( 3.14159265359 ) TEST-UNEXPECTED-FAIL | trace-test.js | Math.cos(3.926990816987) : expected number ( -0.7071067811865 ) != actual number ( 3.926990816987 ) TEST-UNEXPECTED-FAIL | trace-test.js | Math.cos(4.712388980385) : expected number ( 0 ) != actual number ( 4.712388980385 ) TEST-UNEXPECTED-FAIL | trace-test.js | Math.cos(5.497787143782) : expected number ( 0.7071067811865 ) != actual number ( 5.497787143782 ) TEST-UNEXPECTED-FAIL | trace-test.js | Math.cos(Math.PI*2) : expected number ( 1 ) != actual number ( 6.283185307179586 ) TEST-UNEXPECTED-FAIL | trace-test.js | Math.cos(Math.PI/4) : expected number ( 0.7071067811865476 ) != actual number ( 0.7853981633974483 ) TEST-UNEXPECTED-FAIL | trace-test.js | Math.cos(Math.PI/2) : expected number ( 0 ) != actual number ( 1.5707963267948966 ) TEST-UNEXPECTED-FAIL | trace-test.js | Math.cos(3*Math.PI/4) : expected number ( -0.7071067811865476 ) != actual number ( 2.356194490192345 ) TEST-UNEXPECTED-FAIL | trace-test.js | Math.cos(Math.PI) : expected number ( -1 ) != actual number ( 3.141592653589793 ) TEST-UNEXPECTED-FAIL | trace-test.js | Math.cos(5*Math.PI/4) : expected number ( -0.7071067811865476 ) != actual number ( 3.9269908169872414 ) TEST-UNEXPECTED-FAIL | trace-test.js | Math.cos(3*Math.PI/2) : expected number ( 0 ) != actual number ( 4.71238898038469 ) TEST-UNEXPECTED-FAIL | trace-test.js | Math.cos(7*Math.PI/4) : expected number ( 0.7071067811865476 ) != actual number ( 5.497787143782138 ) TEST-UNEXPECTED-FAIL | trace-test.js | Math.cos(2*Math.PI) : expected number ( 1 ) != actual number ( 6.283185307179586 ) TEST-UNEXPECTED-FAIL | trace-test.js | Math.cos(-0.7853981633974) : expected number ( 0.7071067811865 ) != actual number ( 0.7853981633974 ) TEST-UNEXPECTED-FAIL | trace-test.js | Math.cos(-1.570796326795) : expected number ( 0 ) != actual number ( 1.570796326795 ) TEST-UNEXPECTED-FAIL | trace-test.js | Math.cos(2.3561944901920) : expected number ( -0.7071067811865 ) != actual number ( 2.356194490192 ) TEST-UNEXPECTED-FAIL | trace-test.js | Math.cos(3.14159265359) : expected number ( -1 ) != actual number ( 3.14159265359 ) TEST-UNEXPECTED-FAIL | trace-test.js | Math.cos(3.926990816987) : expected number ( -0.7071067811865 ) != actual number ( 3.926990816987 ) TEST-UNEXPECTED-FAIL | trace-test.js | Math.cos(4.712388980385) : expected number ( 0 ) != actual number ( 4.712388980385 ) TEST-UNEXPECTED-FAIL | trace-test.js | Math.cos(5.497787143782) : expected number ( 0.7071067811865 ) != actual number ( 5.497787143782 ) TEST-UNEXPECTED-FAIL | trace-test.js | Math.cos(6.28318530718) : expected number ( 1 ) != actual number ( 6.28318530718 ) TEST-UNEXPECTED-FAIL | trace-test.js | Math.cos(-Math.PI/4) : expected number ( 0.7071067811865476 ) != actual number ( 0.7853981633974483 ) TEST-UNEXPECTED-FAIL | trace-test.js | Math.cos(-Math.PI/2) : expected number ( 0 ) != actual number ( 1.5707963267948966 ) TEST-UNEXPECTED-FAIL | trace-test.js | Math.cos(-3*Math.PI/4) : expected number ( -0.7071067811865476 ) != actual number ( 2.356194490192345 ) TEST-UNEXPECTED-FAIL | trace-test.js | Math.cos(-Math.PI) : expected number ( -1 ) != actual number ( 3.141592653589793 ) TEST-UNEXPECTED-FAIL | trace-test.js | Math.cos(-5*Math.PI/4) : expected number ( -0.7071067811865476 ) != actual number ( 3.9269908169872414 ) TEST-UNEXPECTED-FAIL | trace-test.js | Math.cos(-3*Math.PI/2) : expected number ( 0 ) != actual number ( 4.71238898038469 ) TEST-UNEXPECTED-FAIL | trace-test.js | Math.cos(-7*Math.PI/4) : expected number ( 0.7071067811865476 ) != actual number ( 5.497787143782138 ) TEST-UNEXPECTED-FAIL | trace-test.js | Math.cos(-Math.PI*2) : expected number ( 1 ) != actual number ( 6.283185307179586 ) TEST-UNEXPECTED-FAIL | trace-test.js | Math.exp(null) : expected number ( 1 ) != actual number ( 0 ) TEST-PASS | trace-test.js | Math.exp(void 0) TEST-UNEXPECTED-FAIL | trace-test.js | Math.exp(1) : expected number ( 2.718281828459045 ) != actual number ( 1 ) TEST-UNEXPECTED-FAIL | trace-test.js | Math.exp(true) : expected number ( 2.718281828459045 ) != actual number ( 1 ) TEST-UNEXPECTED-FAIL | trace-test.js | Math.exp(false) : expected number ( 1 ) != actual number ( 0 ) TEST-UNEXPECTED-FAIL | trace-test.js | Math.exp('1') : expected number ( 2.718281828459045 ) != actual number ( 1 ) TEST-UNEXPECTED-FAIL | trace-test.js | Math.exp('0') : expected number ( 1 ) != actual number ( 0 ) TEST-PASS | trace-test.js | Math.exp(Number.NaN) TEST-UNEXPECTED-FAIL | trace-test.js | Math.exp(0) : expected number ( 1 ) != actual number ( 0 ) TEST-UNEXPECTED-FAIL | trace-test.js | Math.exp(-0) : expected number ( 1 ) != actual number ( 0 ) TEST-PASS | trace-test.js | Math.exp(Number.POSITIVE_INFINITY) TEST-UNEXPECTED-FAIL | trace-test.js | Math.exp(Number.NEGATIVE_INFINITY) : expected number ( 0 ) != actual number ( Infinity ) TEST-PASS | trace-test.js | Math.floor(void 0) TEST-PASS | trace-test.js | Math.floor(null) TEST-PASS | trace-test.js | Math.floor(true) TEST-PASS | trace-test.js | Math.floor(false) TEST-UNEXPECTED-FAIL | trace-test.js | Math.floor("1.1") : expected number ( 1 ) != actual number ( 1.1 ) TEST-UNEXPECTED-FAIL | trace-test.js | Math.floor("-1.1") : expected number ( -2 ) != actual number ( 1.1 ) TEST-UNEXPECTED-FAIL | trace-test.js | Math.floor("0.1") : expected number ( 0 ) != actual number ( 0.1 ) TEST-UNEXPECTED-FAIL | trace-test.js | Math.floor("-0.1") : expected number ( -1 ) != actual number ( 0.1 ) TEST-PASS | trace-test.js | Math.floor(Number.NaN) TEST-PASS | trace-test.js | Math.floor(Number.NaN) == -Math.ceil(-Number.NaN) TEST-PASS | trace-test.js | Math.floor(0) Assertion failure: fp->slots + fp->script->nfixed + js_ReconstructStackDepth(cx, fp->script, fp->regs->pc) == fp->regs->sp, at ../jstracer.cpp:5340 Trace/BPT trap
igor just pointed out that trace-test passes with jit off with the patch
Comment on attachment 386304 [details] [diff] [review] must flush the enumcache, even if we don't renumber >+ if (IS_GC_MARKING_TRACER(trc) && rt->gcShapeRegenerationTracer == trc) { > uint32 shape, oldshape; > > shape = js_RegenerateShapeForGC(cx); > if (!(sprop->flags & SPROP_MARK)) { > oldshape = sprop->shape; > sprop->shape = shape; > sprop->flags |= SPROP_FLAG_SHAPE_REGEN; > if (scope->shape != oldshape) > shape = js_RegenerateShapeForGC(cx); > } > > scope->shape = shape; >+ } else { The else part should be also under IS_GC_MARKING_TRACER(trc).
(In reply to comment #15) > (From update of attachment 386304 [details] [diff] [review]) The patch must also alter jsscope.cpp not to call js_RegenerateShapeForGC there.
Attached patch patch (obsolete) — — Splinter Review
Attachment #386304 - Attachment is obsolete: true
Could we have a mode (e.g. triggered by env var) in which every gc regenerates shapes, so we can catch bugs caused by that (e.g. the current bug where the regeneration is just broken)?
#18, yeah, definitively.
(In reply to comment #12) > If we allow sweeping of properties, we might delete sprops, which means we > would have to flush all traces that embed them (none right now, but jason might > change that soon). Or we don't embed sprops. We do already embed sprops sometimes, right? Calls to js_AddProperty?
Splitting out parts of my mega-patch for bug 497789. bz, can you give this a spin to vouch for it? Thanks, /be
Assignee: gal → brendan
Attachment #386310 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #389748 - Flags: review?(jorendorff)
Severity: normal → major
Priority: -- → P1
Target Milestone: --- → mozilla1.9.2a1
Blocks: 497789, 471214
You mean run or review? Not sure where exactly to run it. Review-wise it bothers me that we're not really enforcing scope claiming an own shape if its shape doesn't match lastProp->shape (or vice versa, if that can ever happen). The patch in bug 497789 seemed, at first glance, to be more reasonable about that. If you're sure we can never end up in a situation where the hasOwnShape() lies during gc, looks fine.
Though again the code looked clearer to me in bug 497789...
Seems vacuous to assert lastProp->shape != shape in JSScope:: methods given how generateOwnShape is the only caller of setOwnShape, and js_GenerateShape returns a unique shape. The other way this could go wrong, as you suggest, is for an own-shape scope to somehow revert to shape == lastProp->shape. Again incrementing rt->shapeGen would seem to rule this out. I don't like adding assertions to getters, esp. without any evidence of bugs here. The code is a small-world network, pretty easy to search. Definitely more to do in bug 497789, but I hope to get the refreshed patch I'm about to attach in. Please make specific suggestions, though, I'll do what I can on the assertion-itis front. /be
Attached patch refreshed to tm tip (obsolete) — — Splinter Review
Attachment #389748 - Attachment is obsolete: true
Attachment #389748 - Flags: review?(jorendorff)
(In reply to comment #18) > Could we have a mode (e.g. triggered by env var) in which every gc regenerates > shapes, so we can catch bugs caused by that (e.g. the current bug where the > regeneration is just broken)? File separately? Sorry, don't want to add here, trying to keep focused on bug 497789. /be
Attachment #389877 - Flags: review?(bzbarsky)
Attachment #389877 - Flags: review?(jorendorff)
(In reply to comment #10) > Created an attachment (id=386304) [details] > must flush the enumcache, even if we don't renumber Just for posterity, there's no reason to flush rt->nativeEnumCache on every GC, the subsequent loop over rt->nativeEnumerators just has to use the same "only a GC that regenerates shapes" logic where it frees native enumerators. Otherwise, free memory read city, population you! ;-) /be
> Seems vacuous to assert lastProp->shape != shape in JSScope:: methods given how > generateOwnShape is the only caller of setOwnShape I was thinking the other direction: asserting that scope->shape == lastProp->shape if the scope is not hasOwnShape(). Note that for empty scopes (shape 0 for now) we'd need to do something else, but gc doesn't renumber those anyway. In any case, my real concern was with making it impossible for the two to get out of sync, so there's no need for searching, etc. If you think that's overkill, that's fine. I needed that in my patch because I didn't know the surrounding code and hence had no other way to make sure I was getting it right. I guess I can file the every-gc bug separately. Marked dependent on what?
Comment on attachment 389877 [details] [diff] [review] refreshed to tm tip >@@ -1623,7 +1623,7 @@ js_DestroyScript(JSContext *cx, JSScript > JS_ASSERT_IF(cx->runtime->gcRunning, !script->owner); > #endif > >- if (!cx->runtime->gcRunning) { >+ if (!cx->runtime->gcRunning || !cx->runtime->gcRegenShapes) { > JSStackFrame *fp = js_GetTopStackFrame(cx); > > if (!(fp && (fp->flags & JSFRAME_EVAL))) { A comment above this still claims, "The GC flushes all property caches". More seriously: This patch will cause us to keep JIT code across GCs. What about objects directly referenced by JIT code, as in the case of teleportation? Do we mark them? Is there anything else we embed in traces that could get collected? If we do all this already and there's no problem, then r=me with the comment fixed (just in case I don't get back to my queue later tonight). Apart from that, this looks great.
Attached patch fixes per jorendorff's review (obsolete) — — Splinter Review
Attachment #389877 - Attachment is obsolete: true
Attachment #390350 - Flags: review?(jorendorff)
Attachment #389877 - Flags: review?(jorendorff)
Attachment #389877 - Flags: review?(bzbarsky)
Comment on attachment 390350 [details] [diff] [review] fixes per jorendorff's review >@@ -1623,16 +1632,18 @@ js_DestroyScript(JSContext *cx, JSScript > JS_ASSERT_IF(cx->runtime->gcRunning, !script->owner); > #endif > >- if (!cx->runtime->gcRunning) { >+ if (!cx->runtime->gcRunning || !cx->runtime->gcRegenShapes) { I think this is redundant; you could just say: >+ if (!cx->runtime->gcRegenShapes) { If that is right, then the `if (!gcRegenShapes)` line here >+ if (!cx->runtime->gcRegenShapes) >+ js_PurgePropertyCacheForScript(cx, script); is redundant too. r=me with that addressed.
Attachment #390350 - Flags: review?(jorendorff) → review+
http://hg.mozilla.org/tracemonkey/rev/047b9102dddb /be
Whiteboard: fixed-in-tracemonkey
Followup to fix location of existing assertion to track revised logic in js_DestroyScript (I should have caught this earlier; it was always a bug in the patch, not due to the review-induced final patch changes): http://hg.mozilla.org/tracemonkey/rev/8f7b8cae113d /be
had to back this out. http://hg.mozilla.org/tracemonkey/rev/9d65d1fccbd4 crashing on the talos Tp set.
Whiteboard: fixed-in-tracemonkey
Will reland later tonight -- sorry for the trouble. /be
Attachment #390350 - Attachment is obsolete: true
Attachment #390397 - Flags: review?(mrbkap)
Attachment #390397 - Attachment is obsolete: true
Attachment #390422 - Flags: review?(mrbkap)
Attachment #390397 - Flags: review?(mrbkap)
Bugzilla interdiff lies a bit but the last two hunks are correct. Looks like the followup fix noted in comment 33 was not backed out fully. Should not be a problem, just a redundant if (!cx->runtime->gcRunning) test. Hoping mrbkap is still awake, or up early when I am :-P. /be
Comment on attachment 390422 [details] [diff] [review] refreshed to tm tip, with shutdown leak of rt->nativeEnumerators fixed r=me on the fix for the bustage.
Attachment #390422 - Flags: review?(mrbkap) → review+
http://hg.mozilla.org/tracemonkey/rev/ce025660fb6b /be
Whiteboard: fixed-in-tracemonkey
And sayrer followed up with http://hg.mozilla.org/tracemonkey/rev/19327c55a2f0 Really want to get this to stick! /be
Depends on: 506341
Won't stick without the patch for bug 506312, which undoes the "property cache purge" part of the summary. At least we don't regen shapes on every GC. /be
http://hg.mozilla.org/mozilla-central/rev/ce025660fb6b
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Blocks: 499866
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: