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.
<rdar://problem/19992268>
Created attachment 247617 [details] Proposed patch
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.
(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.
Created attachment 247673 [details] Proposed patch Made it Better(tm) with Mark's suggestions.
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.
Created attachment 247702 [details] Proposed patch
Created attachment 247714 [details] Proposed patch
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.
Comment on attachment 247714 [details] Proposed patch Clearing flags on attachment: 247714 Committed r181010: <http://trac.webkit.org/changeset/181010>
All reviewed patches have been landed. Closing bug.
Re-opened since this is blocked by bug 142442
Created attachment 248283 [details] Proposed patch
Comment on attachment 248283 [details] Proposed patch r=me
Created attachment 248286 [details] Patch
Comment on attachment 248286 [details] Patch Clearing flags on attachment: 248286 Committed r181297: <http://trac.webkit.org/changeset/181297>