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)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla1.9.2a1
People
(Reporter: igor, Assigned: brendan)
References
Details
(Whiteboard: fixed-in-tracemonkey)
Attachments
(1 file, 8 obsolete files)
(deleted),
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.
Reporter | ||
Comment 1•16 years ago
|
||
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.
Updated•15 years ago
|
Assignee: general → gal
Comment 2•15 years ago
|
||
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.
Updated•15 years ago
|
Attachment #386227 -
Attachment is patch: true
Attachment #386227 -
Attachment mime type: application/octet-stream → text/plain
Comment 3•15 years ago
|
||
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)
Updated•15 years ago
|
Attachment #386227 -
Flags: review?(igor)
Comment 4•15 years ago
|
||
Comment on attachment 386227 [details] [diff] [review] patch Asking for review for feedback only, the patch is definitively not ready yet.
Reporter | ||
Comment 5•15 years ago
|
||
(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.
Comment 6•15 years ago
|
||
My understanding is that if we sweep the property tree, we also have to re-number the shapes. I might be wrong.
Comment 7•15 years ago
|
||
Attachment #386227 -
Attachment is obsolete: true
Attachment #386227 -
Flags: review?(igor)
Reporter | ||
Updated•15 years ago
|
Attachment #386301 -
Attachment is patch: true
Attachment #386301 -
Attachment mime type: application/octet-stream → text/plain
Assignee | ||
Comment 8•15 years ago
|
||
(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
Reporter | ||
Comment 9•15 years ago
|
||
(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?
Comment 10•15 years ago
|
||
Attachment #386301 -
Attachment is obsolete: true
Comment 11•15 years ago
|
||
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 )
Comment 12•15 years ago
|
||
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.
Comment 13•15 years ago
|
||
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
Comment 14•15 years ago
|
||
igor just pointed out that trace-test passes with jit off with the patch
Reporter | ||
Comment 15•15 years ago
|
||
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).
Reporter | ||
Comment 16•15 years ago
|
||
(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.
Comment 17•15 years ago
|
||
Attachment #386304 -
Attachment is obsolete: true
![]() |
||
Comment 18•15 years ago
|
||
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)?
Comment 19•15 years ago
|
||
#18, yeah, definitively.
Comment 20•15 years ago
|
||
(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?
Assignee | ||
Comment 21•15 years ago
|
||
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)
Assignee | ||
Updated•15 years ago
|
Severity: normal → major
Priority: -- → P1
Target Milestone: --- → mozilla1.9.2a1
Assignee | ||
Updated•15 years ago
|
![]() |
||
Comment 22•15 years ago
|
||
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.
![]() |
||
Comment 23•15 years ago
|
||
Though again the code looked clearer to me in bug 497789...
Assignee | ||
Comment 24•15 years ago
|
||
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
Assignee | ||
Comment 25•15 years ago
|
||
Attachment #389748 -
Attachment is obsolete: true
Attachment #389748 -
Flags: review?(jorendorff)
Assignee | ||
Comment 26•15 years ago
|
||
(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
Assignee | ||
Updated•15 years ago
|
Attachment #389877 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•15 years ago
|
Attachment #389877 -
Flags: review?(jorendorff)
Assignee | ||
Comment 27•15 years ago
|
||
(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
![]() |
||
Comment 28•15 years ago
|
||
> 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 29•15 years ago
|
||
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.
Assignee | ||
Comment 30•15 years ago
|
||
Attachment #389877 -
Attachment is obsolete: true
Attachment #390350 -
Flags: review?(jorendorff)
Attachment #389877 -
Flags: review?(jorendorff)
Attachment #389877 -
Flags: review?(bzbarsky)
Comment 31•15 years ago
|
||
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+
Assignee | ||
Comment 32•15 years ago
|
||
http://hg.mozilla.org/tracemonkey/rev/047b9102dddb /be
Whiteboard: fixed-in-tracemonkey
Assignee | ||
Comment 33•15 years ago
|
||
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
Comment 34•15 years ago
|
||
had to back this out. http://hg.mozilla.org/tracemonkey/rev/9d65d1fccbd4 crashing on the talos Tp set.
Whiteboard: fixed-in-tracemonkey
Assignee | ||
Comment 35•15 years ago
|
||
Will reland later tonight -- sorry for the trouble. /be
Attachment #390350 -
Attachment is obsolete: true
Attachment #390397 -
Flags: review?(mrbkap)
Assignee | ||
Comment 36•15 years ago
|
||
Attachment #390397 -
Attachment is obsolete: true
Attachment #390422 -
Flags: review?(mrbkap)
Attachment #390397 -
Flags: review?(mrbkap)
Assignee | ||
Comment 37•15 years ago
|
||
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 38•15 years ago
|
||
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+
Assignee | ||
Comment 39•15 years ago
|
||
http://hg.mozilla.org/tracemonkey/rev/ce025660fb6b /be
Whiteboard: fixed-in-tracemonkey
Assignee | ||
Comment 40•15 years ago
|
||
And sayrer followed up with http://hg.mozilla.org/tracemonkey/rev/19327c55a2f0 Really want to get this to stick! /be
Assignee | ||
Comment 41•15 years ago
|
||
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
Comment 42•15 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/ce025660fb6b
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•