If we move a cached object, then we get incorrect objects on future lookups. We could alternatively purge, but it seems better to keep these live, given the frequency of minor GC. This algorithm does the re-hashing in-place, taking advantage of the fact that we can safely clobber entries.

Comment on attachment 747552 [details] [diff] [review] v0 Review of attachment 747552 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jscntxt.h @@ +297,5 @@ > class NewObjectCache > { > /* Statically asserted to be equal to sizeof(JSObject_Slots16) */ > static const unsigned MAX_OBJ_SIZE = 4 * sizeof(void*) + 16 * sizeof(Value); > + static const unsigned CACHE_SIZE = 41; // TODO: reconsider size No need for this. You can just use mozilla::ArrayLength everywhere. ::: js/src/jscntxtinlines.h @@ +29,5 @@ > JS_STATIC_ASSERT(gc::FINALIZE_OBJECT_LAST == gc::FINALIZE_OBJECT16_BACKGROUND); > } > > +inline void > +NewObjectCache::traceForMinorGC(JSTracer *trc) Rather than do this, I think it would be simpler to clear all cache entries where entry->key is in the nursery. So maybe call this clearNurseryObjects.

(In reply to Bill McCloskey (:billm) from comment #1) > ::: js/src/jscntxtinlines.h > @@ +29,5 @@ > > JS_STATIC_ASSERT(gc::FINALIZE_OBJECT_LAST == gc::FINALIZE_OBJECT16_BACKGROUND); > > } > > > > +inline void > > +NewObjectCache::traceForMinorGC(JSTracer *trc) > > Rather than do this, I think it would be simpler to clear all cache entries > where entry->key is in the nursery. So maybe call this clearNurseryObjects. Well, I can't really complain about simpler.

Comment on attachment 749551 [details] [diff] [review] v1 Review of attachment 749551 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/tests/js1_8_5/regress/regress-595365-2.js @@ +43,5 @@ > var o_shape128 = shapeOf(o); > +print(o_shape128); > +print(shapeOf(o2)); > +print(o_shape128 == shapeOf(o2)); > +print(o_shape128 === shapeOf(o2)); I don't think this file is supposed to be here.

(In reply to Bill McCloskey (:billm) from comment #3) > ::: js/src/tests/js1_8_5/regress/regress-595365-2.js > > I don't think this file is supposed to be here. D'oh! I removed that hunk and double-checked it was gone... then apparently forgot to qref.

Green try run at: https://tbpl.mozilla.org/?tree=Try&rev=17dcf510b7bb https://hg.mozilla.org/integration/mozilla-inbound/rev/f849dec1a6df

Reluctantly backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/b54ce66659aa - one of the four things in that push, despite having been green on try, picked up causing failures in Android 2.2 reftest-1, reftest-2, and reftest-4 by the time it landed. Since I always blame the need to clobber when it's Android, I clobbered and retriggered on your push, and they still failed.

I'm going to reland this now because this patch only affects ggc enabled builds: https://hg.mozilla.org/integration/mozilla-inbound/rev/c54119fefdc3 I trychose the wrong tests apparently, but here is a green try anyway: https://tbpl.mozilla.org/?tree=Try&rev=a704f20883bf