488731 - Avoid shape regeneration and property cache purge during the GC

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Igor Bukanov]

+++ 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.

Comment 1

[Igor Bukanov]

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.

Comment 2

[Andreas Gal :gal]

Attachment: patch

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.

Comment 3

[Andreas Gal :gal]

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)

Comment 4

[Andreas Gal :gal]

Comment on attachment 386227 [details] [diff] [review]
patch

Asking for review for feedback only, the patch is definitively not ready yet.

Comment 5

[Igor Bukanov]

(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

[Andreas Gal :gal]

My understanding is that if we sweep the property tree, we also have to re-number the shapes. I might be wrong.

Comment 7

[Andreas Gal :gal]

Attachment: more narrowly disable just shape regeneration (still crashing on trace-test)

Comment 8

[Brendan Eich [:brendan]]

(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

Comment 9

[Igor Bukanov]

(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

[Andreas Gal :gal]

Attachment: must flush the enumcache, even if we don't renumber

Comment 11

[Andreas Gal :gal]

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

[Andreas Gal :gal]

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

[Andreas Gal :gal]

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

[Andreas Gal :gal]

igor just pointed out that trace-test passes with jit off with the patch

Comment 15

[Igor Bukanov]

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).

Comment 16

[Igor Bukanov]

(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

[Andreas Gal :gal]

Attachment: patch

Comment 18

[Boris Zbarsky [:bzbarsky]]

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

[Andreas Gal :gal]

#18, yeah, definitively.

Comment 20

[Jason Orendorff [:jorendorff]]

(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?

Comment 21

[Brendan Eich [:brendan]]

Attachment: updated patch based on bug 497789, with crucial js_DestroyScript mod

Splitting out parts of my mega-patch for bug 497789.

bz, can you give this a spin to vouch for it? Thanks,

/be

Comment 22

[Boris Zbarsky [:bzbarsky]]

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

[Boris Zbarsky [:bzbarsky]]

Though again the code looked clearer to me in bug 497789...

Comment 24

[Brendan Eich [:brendan]]

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

Comment 25

[Brendan Eich [:brendan]]

Attachment: refreshed to tm tip

Comment 26

[Brendan Eich [:brendan]]

(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

Comment 27

[Brendan Eich [:brendan]]

(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

[Boris Zbarsky [:bzbarsky]]

> 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

[Jason Orendorff [:jorendorff]]

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.

Comment 30

[Brendan Eich [:brendan]]

Attachment: fixes per jorendorff's review

Comment 31

[Jason Orendorff [:jorendorff]]

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.

Comment 32

[Brendan Eich [:brendan]]

http://hg.mozilla.org/tracemonkey/rev/047b9102dddb

/be

Comment 33

[Brendan Eich [:brendan]]

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

[Robert Sayre]

had to back this out.

http://hg.mozilla.org/tracemonkey/rev/9d65d1fccbd4

crashing on the talos Tp set.

Comment 35

[Brendan Eich [:brendan]]

Attachment: comment 27 problem fixed (had the fix when i wrote that comment, lost it somehow)

Will reland later tonight -- sorry for the trouble.

/be

Comment 36

[Brendan Eich [:brendan]]

Attachment: refreshed to tm tip, with shutdown leak of rt->nativeEnumerators fixed

Comment 37

[Brendan Eich [:brendan]]

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

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 39

[Brendan Eich [:brendan]]

http://hg.mozilla.org/tracemonkey/rev/ce025660fb6b

/be

Comment 40

[Brendan Eich [:brendan]]

And sayrer followed up with

http://hg.mozilla.org/tracemonkey/rev/19327c55a2f0

Really want to get this to stick!

/be

Comment 41

[Brendan Eich [:brendan]]

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

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/ce025660fb6b