60990 2011-05-17 14:43:25 -0700 JSGlobalContextRelease should not trigger a synchronous garbage collection 2011-05-18 14:54:59 -0700 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 sam sam ap aroben barraclough ggaren oldest_to_newest 405296 0 sam 2011-05-17 14:43:25 -0700 JSGlobalContextRelease should not trigger a synchronous garbage collection 405298 1 93818 sam 2011-05-17 14:47:26 -0700 Created attachment 93818 Patch 405339 2 93818 oliver 2011-05-17 15:31:19 -0700 Comment on attachment 93818 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=93818&action=review > Source/JavaScriptCore/API/JSContextRef.cpp:142 > + // garbarge collect soon. garbarge? 405341 3 sam 2011-05-17 15:32:51 -0700 Committed r86712: <http://trac.webkit.org/changeset/86712> 405348 4 aroben 2011-05-17 15:39:52 -0700 What's the motivation for this? Neither this bug nor the ChangeLog say! 405357 5 sam 2011-05-17 15:49:03 -0700 (In reply to comment #4) > What's the motivation for this? Neither this bug nor the ChangeLog say! The motivation is to coalesce garbage collections that are triggered as a result of releasing a global context. Otherwise, repeated calls to JSGlobalContextRelease would each trigger a full collection, where one at the end would be much better. As a general rule, synchronous full garbage collection should be avoided as it is very expensive, and should definitely not happen unexpectedly as was happening with JSGlobalContextRelease. 405482 6 ap 2011-05-17 20:39:10 -0700 We discussed this in person, but I'm not sure how the landed version avoids leaking objects in a worker context. Worker threads don't even have CFRunLoops to schedule timers on. Is this patch correct? 405501 7 ggaren 2011-05-17 21:36:25 -0700 > We discussed this in person, but I'm not sure how the landed version avoids leaking objects in a worker context. Worker threads don't even have CFRunLoops to schedule timers on. When a worker thread exits, it releases the last reference to its context group (JSGlobalData), which calls Heap::destroy(). 405504 8 sam 2011-05-17 21:43:16 -0700 (In reply to comment #6) > We discussed this in person, but I'm not sure how the landed version avoids leaking objects in a worker context. Worker threads don't even have CFRunLoops to schedule timers on. > > Is this patch correct? What Geoff said. 405513 9 ap 2011-05-17 21:57:57 -0700 Thanks. I verified that "run-webkit-tests fast/workers --leaks" reports no leaks. What about arbitrary clients of JSC API? These still don't need to have a CFRunLoop, which DefaultGCActivityCallback seems to require. 405677 10 aroben 2011-05-18 06:38:39 -0700 (In reply to comment #5) > (In reply to comment #4) > > What's the motivation for this? Neither this bug nor the ChangeLog say! > > The motivation is to coalesce garbage collections that are triggered as a result of releasing a global context. Otherwise, repeated calls to JSGlobalContextRelease would each trigger a full collection, where one at the end would be much better. > > As a general rule, synchronous full garbage collection should be avoided as it is very expensive, and should definitely not happen unexpectedly as was happening with JSGlobalContextRelease. Thanks, Sam! 405976 11 ggaren 2011-05-18 13:10:30 -0700 > What about arbitrary clients of JSC API? These still don't need to have a CFRunLoop, which DefaultGCActivityCallback seems to require. As we've discussed before, on CF platforms, DefaultGCActivityCallback is optimized for CFRunLoop, but doesn't require it. 405987 12 ap 2011-05-18 13:20:59 -0700 For my education, could you please describe how it works without CFRunLoop? The only call to heap->collectAllGarbage() in CGActivityCallbackCF.cpp is in DefaultGCActivityCallbackPlatformData::trigger(), and the only call to trigger() is through a CFRunLoopTimer. This is the only GCActivityCallback subclass in JSC. 405988 13 ggaren 2011-05-18 13:23:27 -0700 (In reply to comment #12) > For my education, could you please describe how it works without CFRunLoop? If a CF-configured client doesn't run the CFRunLoop, DefaultGCActivityCallback's optimization to GC from the runloop just doesn't kick in. 406008 14 ap 2011-05-18 13:54:58 -0700 > DefaultGCActivityCallback's optimization to GC from the runloop just doesn't kick in This patch replaced code that works in such threads (globalData.heap.collectAllGarbage()) with code that's effectively no-op. Are you saying that this is OK? 406009 15 ggaren 2011-05-18 14:00:30 -0700 > This patch replaced code that works in such threads (globalData.heap.collectAllGarbage()) with code that's effectively no-op. Are you saying that this is OK? I don't think that's an accurate characterization. The old code didn't "work." It sometimes eagerly reclaimed memory, sometimes didn't eagerly reclaim memory, and sometimes caused O(N^2) hangs. The new code does coalesced attempts at reclaiming memory on threads that run the CFRunLoop, and nothing on threads that don't. Whether that's OK or not depends on the thread and its requirements. On all the threads that I've observed in WebKit, the new code resolves the O(N^2) issue without a regression in memory use. 406041 16 ap 2011-05-18 14:54:59 -0700 The code was added a long time ago (http://trac.webkit.org/changeset/35917), and unfortunately, the commit that added it didn't have a lot of detail in ChangeLog. All I remember is that non-WebKit JavaScriptCore API clients' needs dictated when JSGlobalContextRelease should perform GC. 93818 2011-05-17 14:47:26 -0700 2011-05-17 15:31:19 -0700 Patch bug-60990-20110517144725.patch text/plain 1989 sam SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gODY3MDcpCisrKyBTb3VyY2Uv SmF2YVNjcmlwdENvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTQgQEAK KzIwMTEtMDUtMTcgIFNhbSBXZWluaWcgIDxzYW1Ad2Via2l0Lm9yZz4KKworICAgICAgICBSZXZp ZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBKU0dsb2JhbENvbnRleHRSZWxlYXNl IHNob3VsZCBub3QgdHJpZ2dlciBhIHN5bmNocm9ub3VzIGdhcmJhZ2UgY29sbGVjdGlvbgorICAg ICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NjA5OTAKKworICAg ICAgICAqIEFQSS9KU0NvbnRleHRSZWYuY3BwOgorICAgICAgICBDaGFuZ2Ugc3luY2hyb25vdXMg Y2FsbCB0byBjb2xsZWN0QWxsR2FyYmFnZSB0byBhIGNhbGwgdG8gdHJpZ2dlciB0aGUKKyAgICAg ICAgYWN0aXZpdHlDYWxsYmFjay4KKwogMjAxMS0wNS0xNiAgT2xpdmVyIEh1bnQgIDxvbGl2ZXJA YXBwbGUuY29tPgogCiAgICAgICAgIFJldmlld2VkIGJ5IEdhdmluIEJhcnJhY2xvdWdoLgpJbmRl eDogU291cmNlL0phdmFTY3JpcHRDb3JlL0FQSS9KU0NvbnRleHRSZWYuY3BwCj09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0K LS0tIFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9BUEkvSlNDb250ZXh0UmVmLmNwcAkocmV2aXNpb24g ODY2MzEpCisrKyBTb3VyY2UvSmF2YVNjcmlwdENvcmUvQVBJL0pTQ29udGV4dFJlZi5jcHAJKHdv cmtpbmcgY29weSkKQEAgLTEzOCwxNCArMTM4LDE3IEBAIHZvaWQgSlNHbG9iYWxDb250ZXh0UmVs ZWFzZShKU0dsb2JhbENvbnQKICAgICAvLyAqIElmIHRoaXMgaXMgdGhlIGxhc3QgcmVmZXJlbmNl IHRvIGFueSBjb250ZXh0cyBpbiB0aGUgZ2l2ZW4gY29udGV4dCBncm91cCwKICAgICAvLyAgIGNh bGwgZGVzdHJveSBvbiB0aGUgaGVhcCAodGhlIGdsb2JhbCBkYXRhIGlzIGJlaW5nICBmcmVlZCku CiAgICAgLy8gKiBJZiB0aGlzIHdhcyB0aGUgbGFzdCByZWZlcmVuY2UgdG8gdGhlIGdsb2JhbCBv YmplY3QsIHRoZW4gdW5wcm90ZWN0aW5nCi0gICAgLy8gICBpdCBtYXkgIHJlbGVhc2UgYSBsb3Qg b2YgR0MgbWVtb3J5IC0gcnVuIHRoZSBnYXJiYWdlIGNvbGxlY3RvciBub3cuCisgICAgLy8gICBp dCBtYXkgIHJlbGVhc2UgYSBsb3Qgb2YgR0MgbWVtb3J5IC0gdGlja2xlIHRoZSBhY3Rpdml0eSBj YWxsYmFjayB0bworICAgIC8vICAgZ2FyYmFyZ2UgY29sbGVjdCBzb29uLgogICAgIC8vICogSWYg dGhlcmUgYXJlIG1vcmUgcmVmZXJlbmNlcyByZW1haW5pbmcgdGhlIHRoZSBnbG9iYWwgb2JqZWN0 LCB0aGVuIGRvIG5vdGhpbmcKICAgICAvLyAgIChzcGVjaWZpY2FsbHkgdGhhdCBpcyBtb3JlIHBy b3RlY3RzLCB3aGljaCB3ZSBhc3N1bWUgY29tZSBmcm9tIG90aGVyIEpTR2xvYmFsQ29udGV4dFJl ZnMpLgogICAgIGlmIChyZWxlYXNpbmdDb250ZXh0R3JvdXApIHsKICAgICAgICAgZ2xvYmFsRGF0 YS5jbGVhckJ1aWx0aW5TdHJ1Y3R1cmVzKCk7CiAgICAgICAgIGdsb2JhbERhdGEuaGVhcC5kZXN0 cm95KCk7Ci0gICAgfSBlbHNlIGlmIChyZWxlYXNpbmdHbG9iYWxPYmplY3QpCi0gICAgICAgIGds b2JhbERhdGEuaGVhcC5jb2xsZWN0QWxsR2FyYmFnZSgpOworICAgIH0gZWxzZSBpZiAocmVsZWFz aW5nR2xvYmFsT2JqZWN0KSB7CisgICAgICAgIGdsb2JhbERhdGEuaGVhcC5hY3Rpdml0eUNhbGxi YWNrKCktPnN5bmNocm9uaXplKCk7CisgICAgICAgICgqZ2xvYmFsRGF0YS5oZWFwLmFjdGl2aXR5 Q2FsbGJhY2soKSkoKTsKKyAgICB9CiAKICAgICBnbG9iYWxEYXRhLmRlcmVmKCk7CiAK