870496 - Update NewObjectCache on minor GC

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Terrence Cole [:terrence]]

Attachment: v0

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 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 2

[Terrence Cole [:terrence]]

Attachment: v1

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

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 4

[Terrence Cole [:terrence]]

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

Comment 5

[Terrence Cole [:terrence]]

Green try run at: https://tbpl.mozilla.org/?tree=Try&rev=17dcf510b7bb

https://hg.mozilla.org/integration/mozilla-inbound/rev/f849dec1a6df

Comment 6

[Phil Ringnalda (:philor)]

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.

Comment 7

[Terrence Cole [:terrence]]

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

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c54119fefdc3