Closed Bug 870496 Opened 8 years ago Closed 8 years ago

Update NewObjectCache on minor GC

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: terrence, Assigned: terrence)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch v0 (obsolete) — Splinter Review
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.
Attachment #747552 - Flags: review?(wmccloskey)
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.
Attachment #747552 - Flags: review?(wmccloskey)
Attached patch v1Splinter Review
(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.
Attachment #747552 - Attachment is obsolete: true
Attachment #749551 - Flags: review?(wmccloskey)
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.
Attachment #749551 - Flags: review?(wmccloskey) → review+
(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.
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
https://hg.mozilla.org/mozilla-central/rev/c54119fefdc3
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.