Bug 142115

Summary: Stale WeakGCMap entries are keeping tons of WeakBlocks alive unnecessarily.
Product: WebKit Reporter: Andreas Kling <kling>
Component: JavaScriptCoreAssignee: Andreas Kling <kling>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough, commit-queue, ggaren, kling, mhahnenb, webkit-bug-importer
Priority: P2 Keywords: InRadar, Performance
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 142442    
Bug Blocks:    
Attachments:
Description Flags
Proposed patch
none
Proposed patch
none
Proposed patch
none
Proposed patch
none
Proposed patch
none
Proposed patch
ggaren: review+
Patch none

Description Andreas Kling 2015-02-27 16:01:23 PST
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.
Comment 1 Radar WebKit Bug Importer 2015-02-27 16:02:01 PST
<rdar://problem/19992268>
Comment 2 Andreas Kling 2015-02-28 16:19:11 PST
Created attachment 247617 [details]
Proposed patch
Comment 3 Mark Hahnenberg 2015-02-28 18:17:45 PST
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.
Comment 4 Andreas Kling 2015-02-28 18:49:51 PST
(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.
Comment 5 Andreas Kling 2015-03-02 08:58:20 PST
Created attachment 247673 [details]
Proposed patch

Made it Better(tm) with Mark's suggestions.
Comment 6 Andreas Kling 2015-03-02 09:46:20 PST
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.
Comment 7 Andreas Kling 2015-03-02 14:55:25 PST
Created attachment 247702 [details]
Proposed patch
Comment 8 Andreas Kling 2015-03-02 16:16:45 PST
Created attachment 247714 [details]
Proposed patch
Comment 9 Geoffrey Garen 2015-03-02 16:24:36 PST
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 10 WebKit Commit Bot 2015-03-04 12:00:24 PST
Comment on attachment 247714 [details]
Proposed patch

Clearing flags on attachment: 247714

Committed r181010: <http://trac.webkit.org/changeset/181010>
Comment 11 WebKit Commit Bot 2015-03-04 12:00:30 PST
All reviewed patches have been landed.  Closing bug.
Comment 12 WebKit Commit Bot 2015-03-07 14:15:26 PST
Re-opened since this is blocked by bug 142442
Comment 13 Andreas Kling 2015-03-09 15:43:31 PDT
Created attachment 248283 [details]
Proposed patch
Comment 14 Geoffrey Garen 2015-03-09 16:12:39 PDT
Comment on attachment 248283 [details]
Proposed patch

r=me
Comment 15 Andreas Kling 2015-03-09 16:19:12 PDT
Created attachment 248286 [details]
Patch
Comment 16 WebKit Commit Bot 2015-03-09 17:10:07 PDT
Comment on attachment 248286 [details]
Patch

Clearing flags on attachment: 248286

Committed r181297: <http://trac.webkit.org/changeset/181297>
Comment 17 WebKit Commit Bot 2015-03-09 17:10:12 PDT
All reviewed patches have been landed.  Closing bug.