169704 2017-03-15 15:36:37 -0700 WebAssembly: When we GC to try to get a fast memory, we should call collectAllGarbage(), not collectSync() 2017-03-15 17:36:45 -0700 1 1 1 Unclassified WebKit JavaScriptCore WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 saam saam benjamin commit-queue fpizlo ggaren gskachkov jfbastien keith_miller mark.lam msaboff ticaiolima ysuzuki oldest_to_newest 1288286 0 saam 2017-03-15 15:36:37 -0700 Otherwise we won't actually end up getting the memory because we don't do any sweeping. 1288296 1 304563 saam 2017-03-15 15:41:17 -0700 Created attachment 304563 patch 1288297 2 304564 saam 2017-03-15 15:42:32 -0700 Created attachment 304564 patch 1288300 3 304564 mark.lam 2017-03-15 15:43:31 -0700 Comment on attachment 304564 patch r=me 1288335 4 304564 commit-queue 2017-03-15 16:17:18 -0700 Comment on attachment 304564 patch Clearing flags on attachment: 304564 Committed r214018: <http://trac.webkit.org/changeset/214018> 1288337 5 commit-queue 2017-03-15 16:17:24 -0700 All reviewed patches have been landed. Closing bug. 1288345 6 304564 fpizlo 2017-03-15 16:35:52 -0700 Comment on attachment 304564 patch View in context: https://bugs.webkit.org/attachment.cgi?id=304564&action=review > Source/JavaScriptCore/wasm/WasmMemory.cpp:123 > - vm.heap.collectSync(); > + vm.heap.collectAllGarbage(); > return dequeFastMemory(); This is sad - isn't this whole thing part of some promise anyway? Seems like waiting for the GC to finish should be part of the promise. 1288352 7 saam 2017-03-15 16:43:42 -0700 (In reply to comment #6) > Comment on attachment 304564 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=304564&action=review > > > Source/JavaScriptCore/wasm/WasmMemory.cpp:123 > > - vm.heap.collectSync(); > > + vm.heap.collectAllGarbage(); > > return dequeFastMemory(); > > This is sad - isn't this whole thing part of some promise anyway? Seems > like waiting for the GC to finish should be part of the promise. I see what you're getting at here, but I think it's somewhat tricky to do. We would need a two-level strategy. Call collectSync, then set up a promise to be called when things get swept, then set up another promise when we're done compiling. I'm not sure exactly how to do that. It might be worth doing as part of Keith's work on async compilation, but we also don't expect this code to run very often. 1288358 8 fpizlo 2017-03-15 16:55:18 -0700 (In reply to comment #7) > (In reply to comment #6) > > Comment on attachment 304564 [details] > > patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=304564&action=review > > > > > Source/JavaScriptCore/wasm/WasmMemory.cpp:123 > > > - vm.heap.collectSync(); > > > + vm.heap.collectAllGarbage(); > > > return dequeFastMemory(); > > > > This is sad - isn't this whole thing part of some promise anyway? Seems > > like waiting for the GC to finish should be part of the promise. > > I see what you're getting at here, but I think it's somewhat tricky to do. > We would need a two-level strategy. Call collectSync, then set up a promise > to be called when things get swept, then set up another promise when we're > done compiling. I'm not sure exactly how to do that. It might be worth doing > as part of Keith's work on async compilation, but we also don't expect this > code to run very often. Right, we would need the incremental sweeper to run more aggressively and we would want it to notify us when done. Worth filing a bug. 1288382 9 saam 2017-03-15 17:36:45 -0700 (In reply to comment #8) > (In reply to comment #7) > > (In reply to comment #6) > > > Comment on attachment 304564 [details] > > > patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=304564&action=review > > > > > > > Source/JavaScriptCore/wasm/WasmMemory.cpp:123 > > > > - vm.heap.collectSync(); > > > > + vm.heap.collectAllGarbage(); > > > > return dequeFastMemory(); > > > > > > This is sad - isn't this whole thing part of some promise anyway? Seems > > > like waiting for the GC to finish should be part of the promise. > > > > I see what you're getting at here, but I think it's somewhat tricky to do. > > We would need a two-level strategy. Call collectSync, then set up a promise > > to be called when things get swept, then set up another promise when we're > > done compiling. I'm not sure exactly how to do that. It might be worth doing > > as part of Keith's work on async compilation, but we also don't expect this > > code to run very often. > > Right, we would need the incremental sweeper to run more aggressively and we > would want it to notify us when done. > > Worth filing a bug. Filed: https://bugs.webkit.org/show_bug.cgi?id=169728 304563 2017-03-15 15:41:17 -0700 2017-03-15 15:42:32 -0700 patch a-backup.diff text/plain 1897 saam SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMjE0MDExKQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE4IEBA CisyMDE3LTAzLTE1ICBTYWFtIEJhcmF0aSAgPHNiYXJhdGlAYXBwbGUuY29tPgorCisgICAgICAg IFdlYkFzc2VtYmx5OiBXaGVuIHdlIEdDIHRvIHRyeSB0byBnZXQgYSBmYXN0IG1lbW9yeSwgd2Ug c2hvdWxkIGNhbGwgY29sbGVjdEFsbEdhcmJhZ2UoKSwgbm90IGNvbGxlY3RTeW5jKCkKKyAgICAg ICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE2OTcwNAorCisgICAg ICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFdlIHdlcmVuJ3QgYWx3 YXlzIHN3ZWVwaW5nIHRoZSBtZW1vcnkgbmVlZGVkIHRvIGZyZWUKKyAgICAgICAgdGhlIFdhc21N ZW1vcnkgd2Ugd2FudGVkIHRvIHVzZS4gY29sbGVjdEFsbEdhcmJhZ2UoKQorICAgICAgICB3aWxs IGRvIHRoaXMgaWYgdGhlIEpTIG9iamVjdHMgd3JhcHBpbmcgV2FzbU1lbW9yeQorICAgICAgICBh cmUgZGVhZC4KKworICAgICAgICAqIHdhc20vV2FzbU1lbW9yeS5jcHA6CisgICAgICAgIChKU0M6 Oldhc206OnRyeUdldEZhc3RNZW1vcnkpOgorCiAyMDE3LTAzLTE1ICBNYXJrIExhbSAgPG1hcmsu bGFtQGFwcGxlLmNvbT4KIAogICAgICAgICBGaXggbWlzc2luZyBleGNlcHRpb24gY2hlY2tzIGlu IEludGVycHJldGVyLmNwcC4KSW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS93YXNtL1dhc21N ZW1vcnkuY3BwCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9KYXZhU2NyaXB0Q29yZS93YXNtL1dhc21N ZW1vcnkuY3BwCShyZXZpc2lvbiAyMTM5ODMpCisrKyBTb3VyY2UvSmF2YVNjcmlwdENvcmUvd2Fz bS9XYXNtTWVtb3J5LmNwcAkod29ya2luZyBjb3B5KQpAQCAtMTE5LDE1ICsxMTksMTUgQEAgaW5s aW5lIGJvb2wgdHJ5R2V0RmFzdE1lbW9yeShWTSYgdm0sIHZvaQogICAgIC8vIElmIHdlIGhhdmUg YWxsb2NhdGVkIGFsbCB0aGUgZmFzdCBtZW1vcmllcy4uLiB0b28gYmFkLgogICAgIGlmIChhbGxv Y2F0ZWRGYXN0TWVtb3JpZXMgPT0gbWF4RmFzdE1lbW9yaWVzKSB7CiAgICAgICAgIC8vIFRoZXJl IGlzIGEgcmVhc29uYWJsZSBjaGFuY2UgdGhhdCBhbm90aGVyIG1vZHVsZSBoYXMgZGllZCBidXQg aGFzIG5vdCBiZWVuIGNvbGxlY3RlZCB5ZXQuIERvbid0IGxvc2UgaG9wZSB5ZXQhCi0gICAgICAg IHZtLmhlYXAuY29sbGVjdFN5bmMoKTsKKyAgICAgICAgdm0uaGVhcC5jb2xsZWN0QWxsR2FyYmFn ZSgpOwogICAgICAgICByZXR1cm4gZGVxdWVGYXN0TWVtb3J5KCk7CiAgICAgfQogCiAgICAgaWYg KG1tYXBCeXRlcyhmYXN0TWVtb3J5TWFwcGVkQnl0ZXMsIG1lbW9yeSkpIHsKICAgICAgICAgbWFw cGVkQ2FwYWNpdHkgPSBmYXN0TWVtb3J5TWFwcGVkQnl0ZXM7CiAgICAgICAgIG1vZGUgPSBNZW1v cnk6OlNpZ25hbGluZzsKLSAgICAgICAgYWxsb2NhdGVkRmFzdE1lbW9yaWVzKys7CiAgICAgICAg IExvY2tIb2xkZXIgbG9ja2VyKG1lbW9yeUxvY2spOworICAgICAgICBhbGxvY2F0ZWRGYXN0TWVt b3JpZXMrKzsKICAgICAgICAgYXV0byByZXN1bHQgPSBhY3RpdmVGYXN0TWVtb3JpZXMobG9ja2Vy KS5hZGQobWVtb3J5KTsKICAgICAgICAgQVNTRVJUX1VOVVNFRChyZXN1bHQsIHJlc3VsdC5pc05l d0VudHJ5KTsKICAgICB9Cg== 304564 2017-03-15 15:42:32 -0700 2017-03-15 16:17:18 -0700 patch a-backup.diff text/plain 2009 saam SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMjE0MDExKQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDIxIEBA CisyMDE3LTAzLTE1ICBTYWFtIEJhcmF0aSAgPHNiYXJhdGlAYXBwbGUuY29tPgorCisgICAgICAg IFdlYkFzc2VtYmx5OiBXaGVuIHdlIEdDIHRvIHRyeSB0byBnZXQgYSBmYXN0IG1lbW9yeSwgd2Ug c2hvdWxkIGNhbGwgY29sbGVjdEFsbEdhcmJhZ2UoKSwgbm90IGNvbGxlY3RTeW5jKCkKKyAgICAg ICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE2OTcwNAorCisgICAg ICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFdlIHdlcmVuJ3QgYWx3 YXlzIHN3ZWVwaW5nIHRoZSBtZW1vcnkgbmVlZGVkIHRvIGZyZWUKKyAgICAgICAgdGhlIFdhc21N ZW1vcnkgd2Ugd2FudGVkIHRvIHVzZS4gY29sbGVjdEFsbEdhcmJhZ2UoKQorICAgICAgICB3aWxs IGRvIHRoaXMgaWYgdGhlIEpTIG9iamVjdHMgd3JhcHBpbmcgV2FzbU1lbW9yeQorICAgICAgICBh cmUgZGVhZC4KKworICAgICAgICBUaGlzIHBhdGNoIGFsc28gbW92ZXMgdGhlIGluY3JlbWVudCBv ZiB0aGUgYWxsb2NhdGVkRmFzdE1lbW9yaWVzCisgICAgICAgIGludGVnZXIgdG8gYmUgdGhyZWFk IHNhZmUuCisKKyAgICAgICAgKiB3YXNtL1dhc21NZW1vcnkuY3BwOgorICAgICAgICAoSlNDOjpX YXNtOjp0cnlHZXRGYXN0TWVtb3J5KToKKwogMjAxNy0wMy0xNSAgTWFyayBMYW0gIDxtYXJrLmxh bUBhcHBsZS5jb20+CiAKICAgICAgICAgRml4IG1pc3NpbmcgZXhjZXB0aW9uIGNoZWNrcyBpbiBJ bnRlcnByZXRlci5jcHAuCkluZGV4OiBTb3VyY2UvSmF2YVNjcmlwdENvcmUvd2FzbS9XYXNtTWVt b3J5LmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvSmF2YVNjcmlwdENvcmUvd2FzbS9XYXNtTWVt b3J5LmNwcAkocmV2aXNpb24gMjEzOTgzKQorKysgU291cmNlL0phdmFTY3JpcHRDb3JlL3dhc20v V2FzbU1lbW9yeS5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTExOSwxNSArMTE5LDE1IEBAIGlubGlu ZSBib29sIHRyeUdldEZhc3RNZW1vcnkoVk0mIHZtLCB2b2kKICAgICAvLyBJZiB3ZSBoYXZlIGFs bG9jYXRlZCBhbGwgdGhlIGZhc3QgbWVtb3JpZXMuLi4gdG9vIGJhZC4KICAgICBpZiAoYWxsb2Nh dGVkRmFzdE1lbW9yaWVzID09IG1heEZhc3RNZW1vcmllcykgewogICAgICAgICAvLyBUaGVyZSBp cyBhIHJlYXNvbmFibGUgY2hhbmNlIHRoYXQgYW5vdGhlciBtb2R1bGUgaGFzIGRpZWQgYnV0IGhh cyBub3QgYmVlbiBjb2xsZWN0ZWQgeWV0LiBEb24ndCBsb3NlIGhvcGUgeWV0IQotICAgICAgICB2 bS5oZWFwLmNvbGxlY3RTeW5jKCk7CisgICAgICAgIHZtLmhlYXAuY29sbGVjdEFsbEdhcmJhZ2Uo KTsKICAgICAgICAgcmV0dXJuIGRlcXVlRmFzdE1lbW9yeSgpOwogICAgIH0KIAogICAgIGlmICht bWFwQnl0ZXMoZmFzdE1lbW9yeU1hcHBlZEJ5dGVzLCBtZW1vcnkpKSB7CiAgICAgICAgIG1hcHBl ZENhcGFjaXR5ID0gZmFzdE1lbW9yeU1hcHBlZEJ5dGVzOwogICAgICAgICBtb2RlID0gTWVtb3J5 OjpTaWduYWxpbmc7Ci0gICAgICAgIGFsbG9jYXRlZEZhc3RNZW1vcmllcysrOwogICAgICAgICBM b2NrSG9sZGVyIGxvY2tlcihtZW1vcnlMb2NrKTsKKyAgICAgICAgYWxsb2NhdGVkRmFzdE1lbW9y aWVzKys7CiAgICAgICAgIGF1dG8gcmVzdWx0ID0gYWN0aXZlRmFzdE1lbW9yaWVzKGxvY2tlciku YWRkKG1lbW9yeSk7CiAgICAgICAgIEFTU0VSVF9VTlVTRUQocmVzdWx0LCByZXN1bHQuaXNOZXdF bnRyeSk7CiAgICAgfQo=