106830 2013-01-14 14:46:29 -0800 Use MADV_FREE_REUSABLE to return JIT memory to OS 2013-01-16 10:32:02 -0800 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 psolanki psolanki barraclough dglazkov fpizlo ggaren oliver psolanki webkit.review.bot oldest_to_newest 806649 0 psolanki 2013-01-14 14:46:29 -0800 On OS X versions that support it, use MADV_FREE_REUSABLE to return JIT memory back to OS. 806651 1 psolanki 2013-01-14 14:46:59 -0800 <rdar://problem/11437701> 806653 2 psolanki 2013-01-14 14:49:03 -0800 This basically undoes <http://trac.webkit.org/changeset/116593> made in <http://bugs.webkit.org/show_bug.cgi?id=86047>. 806655 3 182632 psolanki 2013-01-14 14:52:31 -0800 Created attachment 182632 Patch 806752 4 182632 webkit.review.bot 2013-01-14 16:30:07 -0800 Comment on attachment 182632 Patch Attachment 182632 did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15867662 New failing tests: inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html 806911 5 182632 ggaren 2013-01-14 19:42:12 -0800 Comment on attachment 182632 Patch Thanks for tackling this. This patch has a bug: notifyNeedPage() forgets to MADV_FREE_REUSE. Let me suggest another approach, which also fixes the bug: (a) Make a USE(MADV_FREE_FOR_JIT_MEMORY) Platform.h #define, with a comment about MADV_FREE_REUSABLE not working on older OS's. (b) Set that to true on older OS's (c) Change "#if OS(DARWIN)" to "#if USE(MADV_FREE_FOR_JIT_MEMORY)" in this file. 807563 6 psolanki 2013-01-15 12:26:54 -0800 (In reply to comment #5) > (From update of attachment 182632 [details]) > Thanks for tackling this. > > This patch has a bug: notifyNeedPage() forgets to MADV_FREE_REUSE. Good catch. I'll fix this. > Let me suggest another approach, which also fixes the bug: > > (a) Make a USE(MADV_FREE_FOR_JIT_MEMORY) Platform.h #define, with a comment about MADV_FREE_REUSABLE not working on older OS's. > (b) Set that to true on older OS's > (c) Change "#if OS(DARWIN)" to "#if USE(MADV_FREE_FOR_JIT_MEMORY)" in this file. Generally, I hate adding more ifdefs to Platform.h since they cause full world builds. And in this case it feels even more painful since the ifdef will be used only in one source file. Do we have a precedent for using #if local to just one source file? Otherwise I can add it in Platform.h like you suggest. 807616 7 ggaren 2013-01-15 13:03:00 -0800 > Generally, I hate adding more ifdefs to Platform.h since they cause full world builds. And in this case it feels even more painful since the ifdef will be used only in one source file. Do we have a precedent for using #if local to just one source file? I think putting the #if inside this file, instead of Platform.h, is probably fine in this case. There's a small precedent for that in FastMalloc.cpp. (Once things start to span multiple files, Platform.h is probably best, to avoid surprises.) Also, let's USE(MADV_FREE_FOR_JIT_MEMORY) for PLATFORM(IOS) as well, as we discussed. 807755 8 182859 psolanki 2013-01-15 15:38:21 -0800 Created attachment 182859 Patch 807841 9 182859 ggaren 2013-01-15 17:20:22 -0800 Comment on attachment 182859 Patch r=me 807851 10 psolanki 2013-01-15 17:35:04 -0800 Committed r139812: <http://trac.webkit.org/changeset/139812> 808424 11 182859 darin 2013-01-16 09:18:22 -0800 Comment on attachment 182859 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=182859&action=review > Source/JavaScriptCore/jit/ExecutableAllocatorFixedVMPool.cpp:44 > +// MADV_FREE_REUSABLE does not work for JIT memory on older OSes so use MADV_FREE in that case. I don’t understand the mention of MADV_FREE_REUSABLE here. I don’t see any code in this file using MADV_FREE_REUSABLE! 808435 12 ggaren 2013-01-16 09:39:47 -0800 > I don’t understand the mention of MADV_FREE_REUSABLE here. I don’t see any code in this file using MADV_FREE_REUSABLE! MADV_FREE_REUSABLE is the default implementation of PageReservation::commit() and PageReservation::decommit(). Maybe the #ifdef should have been about those functions instead. 808504 13 psolanki 2013-01-16 10:32:02 -0800 (In reply to comment #12) > > I don’t understand the mention of MADV_FREE_REUSABLE here. I don’t see any code in this file using MADV_FREE_REUSABLE! > > MADV_FREE_REUSABLE is the default implementation of PageReservation::commit() and PageReservation::decommit(). Maybe the #ifdef should have been about those functions instead. Thats right. I guess the comment could have been clearer. Maybe something like "PageReservation::decommit() uses MADV_FREE_REUSABLE which does not work with JIT memory on older OSes. Use MADV_FREE in such cases." I can make that change if that sounds good. 182632 2013-01-14 14:52:31 -0800 2013-01-15 12:27:12 -0800 Patch bug-106830-20130114144932.patch text/plain 1681 psolanki U3VidmVyc2lvbiBSZXZpc2lvbjogMTM5NjY5CmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvQ2hhbmdlTG9nIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwppbmRleCBj OTk3MGI3ZTAwMTQ5NjNjYWI4YjAyMWIzYWZjOGE2NGRhNTVjMjcxLi5mMTU1MDkyODkyODM4Y2Zm MDgzYmZkMjNkMjEwNjFkOGEyN2RlMWZhIDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwxNyBAQAorMjAxMy0wMS0xNCAgUHJhdGlrIFNvbGFua2kgIDxwc29sYW5raUBhcHBsZS5j b20+CisKKyAgICAgICAgVXNlIE1BRFZfRlJFRV9SRVVTQUJMRSB0byByZXR1cm4gSklUIG1lbW9y eSB0byBPUworICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9 MTA2ODMwCisgICAgICAgIDxyZGFyOi8vcHJvYmxlbS8xMTQzNzcwMT4KKworICAgICAgICBSZXZp ZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBVc2UgTUFEVl9GUkVFX1JFVVNBQkxF IHRvIHJldHVybiBKSVQgbWVtb3J5IG9uIE9TZXMgdGhhdCBoYXZlIHRoZQorICAgICAgICB1bmRl cmx5aW5nIG1hZHZpc2UgYnVnIGZpeGVkLgorCisgICAgICAgICogaml0L0V4ZWN1dGFibGVBbGxv Y2F0b3JGaXhlZFZNUG9vbC5jcHA6CisgICAgICAgIChKU0M6OkZpeGVkVk1Qb29sRXhlY3V0YWJs ZUFsbG9jYXRvcjo6bm90aWZ5UGFnZUlzRnJlZSk6CisKIDIwMTMtMDEtMTMgIEZpbGlwIFBpemxv ICA8ZnBpemxvQGFwcGxlLmNvbT4KIAogICAgICAgICBERkcgcGhhc2VzIHRoYXQgc3RvcmUgcGVy LW5vZGUgaW5mb3JtYXRpb24gc2hvdWxkIHN0b3JlIGl0IGluIE5vZGUgaXRzZWxmIHJhdGhlciB0 aGFuIHVzaW5nIGEgc2Vjb25kYXJ5IHZlY3RvcgpkaWZmIC0tZ2l0IGEvU291cmNlL0phdmFTY3Jp cHRDb3JlL2ppdC9FeGVjdXRhYmxlQWxsb2NhdG9yRml4ZWRWTVBvb2wuY3BwIGIvU291cmNlL0ph dmFTY3JpcHRDb3JlL2ppdC9FeGVjdXRhYmxlQWxsb2NhdG9yRml4ZWRWTVBvb2wuY3BwCmluZGV4 IDdlZTNlMDQ5N2IxODAxYzhmZTQ1ZmY3Yzg2YzZiODYxMjEyYWQ2M2UuLjhiY2VlNTFmZWJhYTBk YmRjNTMwMzEyMTM4ZWUxYzZiNjk0NjVmNzYgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9KYXZhU2NyaXB0 Q29yZS9qaXQvRXhlY3V0YWJsZUFsbG9jYXRvckZpeGVkVk1Qb29sLmNwcAorKysgYi9Tb3VyY2Uv SmF2YVNjcmlwdENvcmUvaml0L0V4ZWN1dGFibGVBbGxvY2F0b3JGaXhlZFZNUG9vbC5jcHAKQEAg LTg1LDcgKzg1LDExIEBAIHByb3RlY3RlZDoKICAgICB7CiAjaWYgT1MoREFSV0lOKQogICAgICAg ICBmb3IgKDs7KSB7CisjaWYgUExBVEZPUk0oTUFDKSAmJiBfX01BQ19PU19YX1ZFUlNJT05fTUlO X1JFUVVJUkVEID49IDEwOTAKKyAgICAgICAgICAgIGludCByZXN1bHQgPSBtYWR2aXNlKHBhZ2Us IHBhZ2VTaXplKCksIE1BRFZfRlJFRV9SRVVTQUJMRSk7CisjZWxzZQogICAgICAgICAgICAgaW50 IHJlc3VsdCA9IG1hZHZpc2UocGFnZSwgcGFnZVNpemUoKSwgTUFEVl9GUkVFKTsKKyNlbmRpZgog ICAgICAgICAgICAgaWYgKCFyZXN1bHQpCiAgICAgICAgICAgICAgICAgcmV0dXJuOwogICAgICAg ICAgICAgQVNTRVJUKHJlc3VsdCA9PSAtMSk7Cg== 182859 2013-01-15 15:38:21 -0800 2013-01-16 09:18:22 -0800 Patch bug-106830-20130115153524.patch text/plain 2067 psolanki U3VidmVyc2lvbiBSZXZpc2lvbjogMTM5Nzk4CmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvQ2hhbmdlTG9nIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwppbmRleCBh OTA2MDk5YmMxZTgxY2FiODlhZWJkNzc1YzZmNTQ3NjQ1NzI4NzMxLi4yNjJiMDQ3ZmZkYWRjNGQz MmY1ODI4YTIyNGE1NWYyMmUwYWI5YTdkIDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwxNyBAQAorMjAxMy0wMS0xNSAgUHJhdGlrIFNvbGFua2kgIDxwc29sYW5raUBhcHBsZS5j b20+CisKKyAgICAgICAgVXNlIE1BRFZfRlJFRV9SRVVTQUJMRSB0byByZXR1cm4gSklUIG1lbW9y eSB0byBPUworICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9 MTA2ODMwCisgICAgICAgIDxyZGFyOi8vcHJvYmxlbS8xMTQzNzcwMT4KKworICAgICAgICBSZXZp ZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBVc2UgTUFEVl9GUkVFX1JFVVNBQkxF IHRvIHJldHVybiBKSVQgbWVtb3J5IG9uIE9TZXMgdGhhdCBoYXZlIHRoZSB1bmRlcmx5aW5nIG1h ZHZpc2UgYnVnCisgICAgICAgIGZpeGVkLgorCisgICAgICAgICogaml0L0V4ZWN1dGFibGVBbGxv Y2F0b3JGaXhlZFZNUG9vbC5jcHA6CisgICAgICAgIChKU0M6OkZpeGVkVk1Qb29sRXhlY3V0YWJs ZUFsbG9jYXRvcjo6bm90aWZ5UGFnZUlzRnJlZSk6CisKIDIwMTMtMDEtMTUgIFphbiBEb2JlcnNl ayAgPHphbmRvYmVyc2VrQGdtYWlsLmNvbT4KIAogICAgICAgICBbQXV0b3Rvb2xzXSBVbmlmeSBK YXZhU2NyaXB0Q29yZSBzb3VyY2VzIGxpc3QsIHJlZ2FyZGxlc3Mgb2YgdGFyZ2V0IE9TCmRpZmYg LS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvaml0L0V4ZWN1dGFibGVBbGxvY2F0b3JGaXhl ZFZNUG9vbC5jcHAgYi9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvaml0L0V4ZWN1dGFibGVBbGxvY2F0 b3JGaXhlZFZNUG9vbC5jcHAKaW5kZXggN2VlM2UwNDk3YjE4MDFjOGZlNDVmZjdjODZjNmI4NjEy MTJhZDYzZS4uY2UzZGI5YWJmNGM5MDAxZjc4ODk3NTYwM2I4MGUwZTYxZjYwM2VhOSAxMDA2NDQK LS0tIGEvU291cmNlL0phdmFTY3JpcHRDb3JlL2ppdC9FeGVjdXRhYmxlQWxsb2NhdG9yRml4ZWRW TVBvb2wuY3BwCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9qaXQvRXhlY3V0YWJsZUFsbG9j YXRvckZpeGVkVk1Qb29sLmNwcApAQCAtNDAsNiArNDAsMTEgQEAKICNpbmNsdWRlIDxzdGRpby5o PgogI2VuZGlmCiAKKyNpZiAhUExBVEZPUk0oSU9TKSAmJiBQTEFURk9STShNQUMpICYmIF9fTUFD X09TX1hfVkVSU0lPTl9NSU5fUkVRVUlSRUQgPCAxMDkwCisvLyBNQURWX0ZSRUVfUkVVU0FCTEUg ZG9lcyBub3Qgd29yayBmb3IgSklUIG1lbW9yeSBvbiBvbGRlciBPU2VzIHNvIHVzZSBNQURWX0ZS RUUgaW4gdGhhdCBjYXNlLgorI2RlZmluZSBXVEZfVVNFX01BRFZfRlJFRV9GT1JfSklUX01FTU9S WSAxCisjZW5kaWYKKwogdXNpbmcgbmFtZXNwYWNlIFdURjsKIAogbmFtZXNwYWNlIEpTQyB7CkBA IC03NCw3ICs3OSw3IEBAIHByb3RlY3RlZDoKICAgICAKICAgICB2aXJ0dWFsIHZvaWQgbm90aWZ5 TmVlZFBhZ2Uodm9pZCogcGFnZSkKICAgICB7Ci0jaWYgT1MoREFSV0lOKQorI2lmIFVTRShNQURW X0ZSRUVfRk9SX0pJVF9NRU1PUlkpCiAgICAgICAgIFVOVVNFRF9QQVJBTShwYWdlKTsKICNlbHNl CiAgICAgICAgIG1fcmVzZXJ2YXRpb24uY29tbWl0KHBhZ2UsIHBhZ2VTaXplKCkpOwpAQCAtODMs NyArODgsNyBAQCBwcm90ZWN0ZWQ6CiAgICAgCiAgICAgdmlydHVhbCB2b2lkIG5vdGlmeVBhZ2VJ c0ZyZWUodm9pZCogcGFnZSkKICAgICB7Ci0jaWYgT1MoREFSV0lOKQorI2lmIFVTRShNQURWX0ZS RUVfRk9SX0pJVF9NRU1PUlkpCiAgICAgICAgIGZvciAoOzspIHsKICAgICAgICAgICAgIGludCBy ZXN1bHQgPSBtYWR2aXNlKHBhZ2UsIHBhZ2VTaXplKCksIE1BRFZfRlJFRSk7CiAgICAgICAgICAg ICBpZiAoIXJlc3VsdCkK