Bug 180248

Summary: Get rid of UnconditionalFinalizers and WeakReferenceHarvesters
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: JavaScriptCoreAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: ews-watchlist, keith_miller, mark.lam, msaboff, saam, sam, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on: 180456, 180636, 186102    
Bug Blocks: 179876, 180250    
Attachments:
Description Flags
Patch sam: review+

Filip Pizlo
Reported 2017-12-01 08:30:22 PST
Adding them to the set of things the GC tracks requires global synchronization that limits parallelism. Also, types that use those APIs tend to use them unconditionally, so we could just put the type into a subspace and iterate the subspace during finalization (to emulate UnconditionalFinalizer) and during constraint solving (to emulate WeakReferenceHarvester). The outcome of this change should be an increase in GC throughput. It might even improve splay perf.
Attachments
Patch (8.18 KB, patch)
2018-06-03 08:00 PDT, Yusuke Suzuki 		sam: review+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Filip Pizlo
Comment 1 2017-12-09 11:46:49 PST
So far, this isn't yielding any speed-ups. It's also not yielding slow-downs either. So maybe the lock contention measurements that I made previously were showing the kind of contention that isn't actually a perf problem. But this is a clean way of introducing IsoSubspaces. In the world of IsoSubspaces for everything, we shouldn't need UnconditionalFinalizers or WeakReferenceHarvesters.
Yusuke Suzuki
Comment 2 2017-12-09 12:05:22 PST
Great, now, I have WeakMap/WeakSet redesign patch in my hand[1]. I'll apply your change to my new WeakMap/WeakSet too :) [1]: https://bugs.webkit.org/show_bug.cgi?id=179929
Filip Pizlo
Comment 3 2017-12-09 14:13:25 PST
(In reply to Yusuke Suzuki from comment #2) > Great, now, I have WeakMap/WeakSet redesign patch in my hand[1]. I'll apply > your change to my new WeakMap/WeakSet too :) > > [1]: https://bugs.webkit.org/show_bug.cgi?id=179929 Note that I’m about to change how unconditional finalization works again. The current approach is not a good fit for InferredValue. In the new API, you’ll be able to opt cells into and out of being part of the GC’s unconditional finalizes set.
Yusuke Suzuki
Comment 4 2018-05-30 09:26:19 PDT
The fancy thing is that now JSWeakMap is the last user of WeakReferenceHarvester. (JSWeakSet does not use it since WeakSet does not need to harvest weak references. All the keys are weak.)
Yusuke Suzuki
Comment 5 2018-06-03 08:00:04 PDT
Created attachment 341864 [details] Patch
Yusuke Suzuki
Comment 6 2018-06-04 00:42:48 PDT
Committed r232463: <https://trac.webkit.org/changeset/232463>
Radar WebKit Bug Importer
Comment 7 2018-06-04 00:43:19 PDT
<rdar://problem/40764004>
Keith Miller
Comment 8 2018-06-04 08:17:09 PDT
Nice!
Yusuke Suzuki
Comment 9 2018-06-06 19:32:48 PDT
(In reply to Keith Miller from comment #8) > Nice! Yay!
Note You need to log in before you can comment on or make changes to this bug.