Bug 142115 – Stale WeakGCMap entries are keeping tons of WeakBlocks alive unnecessarily.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Andreas Kling]

I've noticed that it's very common for the two WeakGCMaps in VM::prototypeMap to have a large number of zombie Weaks that never get cleared due to the GC strategy of WeakGCMap.
Since Weak keeps its WeakBlock alive, which in turn keeps the MarkedBlock alive (if I understand correctly) these zombies end up holding on to a lot of memory that they're not even using.

[Radar WebKit Bug Importer]

<rdar://problem/19992268>

[Andreas Kling]

Created attachment 247617 [details]
Proposed patch

[Mark Hahnenberg]

Comment on attachment 247617 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=247617&action=review

I'd be interested to see how this affects pause times, especially during EdenCollections. It seems like it might be easy to make this generational-friendly, but it also might not be a big contributor to pause times so maybe it's not worth the effort.

Overall, I sort of like the other solution you mentioned in the ChangeLog. Seems kinda hacky to just fix these two/three maps :-/

> Source/JavaScriptCore/heap/Heap.cpp:1050
> +    vm()->prototypeMap.gcMap();
> +    vm()->stringCache.gcMap();

I see that WeakGCMap had a "gcMap" method before. I think that's an odd name for a method that removes stuff from something. When I first read it, gcMap looked like a noun rather than an action (e.g. a "Weak GC Map"). Maybe while we're here we could rename these? Some ideas: clearStaleValues, pruneStaleValues, pruneStaleEntries, pruneDeadValues, pruneReapedValues, or something else along those lines.

Also, if possible, could you factor these calls out into a separate helper method with its own GCPHASE? That way if we hit some pathological case w.r.t. collection times in the future, it will show up in the phase timings.

[Andreas Kling]

(In reply to comment #3)
> Comment on attachment 247617 [details]
> Proposed patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=247617&action=review
> 
> I'd be interested to see how this affects pause times, especially during
> EdenCollections. It seems like it might be easy to make this
> generational-friendly, but it also might not be a big contributor to pause
> times so maybe it's not worth the effort.

Sure, I'll get you some timings once I implement your other suggestions.

> Overall, I sort of like the other solution you mentioned in the ChangeLog.
> Seems kinda hacky to just fix these two/three maps :-/

Yeah, I know. This was rather gimped with good back:buck ratio. I'll put something together where maps register with the heap instead.

> > Source/JavaScriptCore/heap/Heap.cpp:1050
> > +    vm()->prototypeMap.gcMap();
> > +    vm()->stringCache.gcMap();
> 
> I see that WeakGCMap had a "gcMap" method before. I think that's an odd name
> for a method that removes stuff from something. When I first read it, gcMap
> looked like a noun rather than an action (e.g. a "Weak GC Map"). Maybe while
> we're here we could rename these? Some ideas: clearStaleValues,
> pruneStaleValues, pruneStaleEntries, pruneDeadValues, pruneReapedValues, or
> something else along those lines.

Sure, those ideas are all nice!

> Also, if possible, could you factor these calls out into a separate helper
> method with its own GCPHASE? That way if we hit some pathological case
> w.r.t. collection times in the future, it will show up in the phase timings.

Okay.

[Andreas Kling]

Created attachment 247673 [details]
Proposed patch

Made it Better(tm) with Mark's suggestions.

[Andreas Kling]

Created attachment 247677 [details]
Proposed patch

Now with more build juice for EFL and Windows: added WeakGCMapInlines.h to JSCInlines.h, hopefully that will get picked up where it was missing.

[Andreas Kling]

Created attachment 247702 [details]
Proposed patch

[Andreas Kling]

Created attachment 247714 [details]
Proposed patch

[Geoffrey Garen]

Comment on attachment 247714 [details]
Proposed patch

r=me

My one objection to this solution is that it's not terribly scalable to lots of maps. That's OK for now since we don't have lots of maps.

I think the best long-term solution is for a weak block to notice when it contains only dead entries, and to allow itself to be recycled (except for those dead entries) by another MarkedBlock. A WeakBlock only needs to be tied to a MarkedBlock so long as the WeakBlock actually contains at least one pointer into the MarkedBlock.

[WebKit Commit Bot]

Comment on attachment 247714 [details]
Proposed patch

Clearing flags on attachment: 247714

Committed r181010: <http://trac.webkit.org/changeset/181010>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 142442

[Andreas Kling]

Created attachment 248283 [details]
Proposed patch

[Geoffrey Garen]

Comment on attachment 248283 [details]
Proposed patch

r=me

[Andreas Kling]

Created attachment 248286 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 248286 [details]
Patch

Clearing flags on attachment: 248286

Committed r181297: <http://trac.webkit.org/changeset/181297>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.