194634 2019-02-13 19:15:42 -0800 [JSC] Should have default NativeJITCode 2019-02-14 13:38:30 -0800 1 1 1 Unclassified WebKit JavaScriptCore WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 193606 1 ysuzuki ysuzuki ews-watchlist keith_miller mark.lam msaboff saam ticaiolima webkit-bug-importer oldest_to_newest 1506013 0 ysuzuki 2019-02-13 19:15:42 -0800 We are creating so many NativeJITCode which are identical... Just calling trampoline. 1506014 1 ysuzuki 2019-02-13 19:16:00 -0800 It takes 14KB in JSGlobalObject initialization. 1506110 2 362006 ysuzuki 2019-02-14 02:02:15 -0800 Created attachment 362006 Patch 1506116 3 362006 ticaiolima 2019-02-14 03:00:41 -0800 Comment on attachment 362006 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362006&action=review Informal review here. Patch LGTM, but I have some questions. > Source/JavaScriptCore/runtime/VM.cpp:693 > + static NativeJITCode* result; It is a little nit picking, but maybe would be good initialise this to nullptr. > Source/JavaScriptCore/runtime/VM.cpp:695 > + std::call_once(onceKey, [&] { I was wondering why do you prefer this instead of: ``` if (!result) result = new NativeJITCode(LLInt::getCodeRef<JSEntryPtrTag>(llint_native_call_trampoline), JITCode::HostCallThunk, NoIntrinsic); return makeRef(*result); ... ``` 1506120 4 362006 ticaiolima 2019-02-14 03:35:57 -0800 Comment on attachment 362006 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362006&action=review > Source/JavaScriptCore/runtime/VM.cpp:698 > + return makeRef(*result); IIUC, NativeJITCode is ThreadSafeRefCounted. How do we guarantee that ```result``` will always pointing to a valid object? Once last ```NativeExecutable``` is destroyed, it will also destroy NativeJITCode, but ```result``` will be pointing to an invalid address. 1506198 5 362006 mark.lam 2019-02-14 09:36:47 -0800 Comment on attachment 362006 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362006&action=review r=me with ref fix. >> Source/JavaScriptCore/runtime/VM.cpp:693 >> + static NativeJITCode* result; > > It is a little nit picking, but maybe would be good initialise this to nullptr. I think statics already null by default. >> Source/JavaScriptCore/runtime/VM.cpp:695 >> + std::call_once(onceKey, [&] { > > I was wondering why do you prefer this instead of: > > ``` > if (!result) > result = new NativeJITCode(LLInt::getCodeRef<JSEntryPtrTag>(llint_native_call_trampoline), JITCode::HostCallThunk, NoIntrinsic); > > return makeRef(*result); > ... > ``` A process may instantiate more than 1 VMs on multiple threads. Hence, std::call_once is necessary. >> Source/JavaScriptCore/runtime/VM.cpp:698 >> + return makeRef(*result); > > IIUC, NativeJITCode is ThreadSafeRefCounted. How do we guarantee that ```result``` will always pointing to a valid object? Once last ```NativeExecutable``` is destroyed, it will also destroy NativeJITCode, but ```result``` will be pointing to an invalid address. I think Caio is correct here. This can happen if the VM is destroyed before we create any new VMs. Let's ref the NativeJITCode inside the std::call_once so that it will never be deref'ed to 0. 1506237 6 362006 ticaiolima 2019-02-14 10:50:14 -0800 Comment on attachment 362006 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362006&action=review >>> Source/JavaScriptCore/runtime/VM.cpp:695 >>> + std::call_once(onceKey, [&] { >> >> I was wondering why do you prefer this instead of: >> >> ``` >> if (!result) >> result = new NativeJITCode(LLInt::getCodeRef<JSEntryPtrTag>(llint_native_call_trampoline), JITCode::HostCallThunk, NoIntrinsic); >> >> return makeRef(*result); >> ... >> ``` > > A process may instantiate more than 1 VMs on multiple threads. Hence, std::call_once is necessary. Thx for the explanation! >>> Source/JavaScriptCore/runtime/VM.cpp:698 >>> + return makeRef(*result); >> >> IIUC, NativeJITCode is ThreadSafeRefCounted. How do we guarantee that ```result``` will always pointing to a valid object? Once last ```NativeExecutable``` is destroyed, it will also destroy NativeJITCode, but ```result``` will be pointing to an invalid address. > > I think Caio is correct here. This can happen if the VM is destroyed before we create any new VMs. Let's ref the NativeJITCode inside the std::call_once so that it will never be deref'ed to 0. Is it possible to write a test to cover such case? 1506239 7 362006 mark.lam 2019-02-14 10:52:13 -0800 Comment on attachment 362006 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362006&action=review >>> Source/JavaScriptCore/runtime/VM.cpp:698 >>> + return makeRef(*result); >> >> IIUC, NativeJITCode is ThreadSafeRefCounted. How do we guarantee that ```result``` will always pointing to a valid object? Once last ```NativeExecutable``` is destroyed, it will also destroy NativeJITCode, but ```result``` will be pointing to an invalid address. > > I think Caio is correct here. This can happen if the VM is destroyed before we create any new VMs. Let's ref the NativeJITCode inside the std::call_once so that it will never be deref'ed to 0. Yusuke pointed out that this is already handled by makeRef(). Note: he's not using adoptRef(). So all is well. 1506245 8 362006 ysuzuki 2019-02-14 10:55:29 -0800 Comment on attachment 362006 Patch Thank you for your review! 1506257 9 ysuzuki 2019-02-14 11:10:32 -0800 Committed r241557: <https://trac.webkit.org/changeset/241557> 1506260 10 webkit-bug-importer 2019-02-14 11:14:20 -0800 <rdar://problem/48080631> 362006 2019-02-14 02:02:15 -0800 2019-02-14 09:36:47 -0800 Patch bug-194634-20190214020214.patch text/plain 3424 ysuzuki U3VidmVyc2lvbiBSZXZpc2lvbjogMjQxNTAwCmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvQ2hhbmdlTG9nIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwppbmRleCAy MDNiZjhlMzgwNTYyNGE3NTUwN2JjOWY1MGQ2YTFjZmYzZjMxODQ5Li4xMjM5MzczZWM0NGMyYWE2 NDg4MTE3NWZlYzkxNGZlYjY2Y2NjOGZhIDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwyMCBAQAorMjAxOS0wMi0xNCAgWXVzdWtlIFN1enVraSAgPHlzdXp1a2lAYXBwbGUuY29t PgorCisgICAgICAgIFtKU0NdIFNob3VsZCBoYXZlIGRlZmF1bHQgTmF0aXZlSklUQ29kZQorICAg ICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTk0NjM0CisKKyAg ICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgSW4gSlNDX3VzZUpJ VD1mYWxzZSBtb2RlLCB3ZSBhbHdheXMgY3JlYXRlIGlkZW50aWNhbCBOYXRpdmVKSVRDb2RlIGZv ciBjYWxsIGFuZCBjb25zdHJ1Y3Qgd2hlbiB3ZSBjcmVhdGUgTmF0aXZlRXhlY3V0YWJsZS4KKyAg ICAgICAgVGhpcyBpcyBtZWFuaW5nbGVzcyBzaW5jZSB3ZSBkbyBub3QgbW9kaWZ5IE5hdGl2ZUpJ VENvZGUgYWZ0ZXIgdGhlIGNyZWF0aW9uLiBUaGlzIHBhdGNoIGFkZHMgc2luZ2xldG9uIHVzZWQg YXMgYSBkZWZhdWx0IG9uZS4KKyAgICAgICAgU2luY2UgTmF0aXZlSklUQ29kZSAoJiBKSVRDb2Rl KSBpcyBUaHJlYWRTYWZlUmVmQ291bnRlZCwgd2UgY2FuIGp1c3Qgc2hhcmUgaXQgaW4gYSB3aG9s ZSBwcm9jZXNzIGxldmVsLiBUaGlzIHJlbW92ZXMgNDQ2IE5hdGl2ZUpJVENvZGUKKyAgICAgICAg YWxsb2NhdGlvbnMsIHdoaWNoIHRha2VzIDE0S0IuCisKKyAgICAgICAgKiBydW50aW1lL1ZNLmNw cDoKKyAgICAgICAgKEpTQzo6aml0Q29kZUZvckNhbGxUcmFtcG9saW5lKToKKyAgICAgICAgKEpT Qzo6aml0Q29kZUZvckNvbnN0cnVjdFRyYW1wb2xpbmUpOgorICAgICAgICAoSlNDOjpWTTo6Z2V0 SG9zdEZ1bmN0aW9uKToKKwogMjAxOS0wMi0xMyAgS2VpdGggTWlsbGVyICA8a2VpdGhfbWlsbGVy QGFwcGxlLmNvbT4gYW5kIFl1c3VrZSBTdXp1a2kgIDx5c3V6dWtpQGFwcGxlLmNvbT4KIAogICAg ICAgICBXZSBzaG91bGQgb25seSBtYWtlIHJvcGUgc3RyaW5ncyB3aGVuIGNvbmNhdGVuYXRpbmcg c3RyaW5ncyBsb25nIGVub3VnaC4KZGlmZiAtLWdpdCBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9y dW50aW1lL1ZNLmNwcCBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9ydW50aW1lL1ZNLmNwcAppbmRl eCBhMzA5MWQwOTY5M2Y4OWM4MzU1ZTRjZGUwN2Q4N2M4Y2IzZDNiOWZlLi44YjEzM2Y2MDQ1YmNm M2VkYzc2ZWUxYzAyY2M0ZTc4OTA3Y2JkMGUwIDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvcnVudGltZS9WTS5jcHAKKysrIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL3J1bnRpbWUv Vk0uY3BwCkBAIC02ODgsNiArNjg4LDI2IEBAIE5hdGl2ZUV4ZWN1dGFibGUqIFZNOjpnZXRIb3N0 RnVuY3Rpb24oTmF0aXZlRnVuY3Rpb24gZnVuY3Rpb24sIE5hdGl2ZUZ1bmN0aW9uIGNvCiAgICAg cmV0dXJuIGdldEhvc3RGdW5jdGlvbihmdW5jdGlvbiwgTm9JbnRyaW5zaWMsIGNvbnN0cnVjdG9y LCBudWxscHRyLCBuYW1lKTsKIH0KIAorc3RhdGljIFJlZjxOYXRpdmVKSVRDb2RlPiBqaXRDb2Rl Rm9yQ2FsbFRyYW1wb2xpbmUoKQoreworICAgIHN0YXRpYyBOYXRpdmVKSVRDb2RlKiByZXN1bHQ7 CisgICAgc3RhdGljIHN0ZDo6b25jZV9mbGFnIG9uY2VLZXk7CisgICAgc3RkOjpjYWxsX29uY2Uo b25jZUtleSwgWyZdIHsKKyAgICAgICAgcmVzdWx0ID0gbmV3IE5hdGl2ZUpJVENvZGUoTExJbnQ6 OmdldENvZGVSZWY8SlNFbnRyeVB0clRhZz4obGxpbnRfbmF0aXZlX2NhbGxfdHJhbXBvbGluZSks IEpJVENvZGU6Okhvc3RDYWxsVGh1bmssIE5vSW50cmluc2ljKTsKKyAgICB9KTsKKyAgICByZXR1 cm4gbWFrZVJlZigqcmVzdWx0KTsKK30KKworc3RhdGljIFJlZjxOYXRpdmVKSVRDb2RlPiBqaXRD b2RlRm9yQ29uc3RydWN0VHJhbXBvbGluZSgpCit7CisgICAgc3RhdGljIE5hdGl2ZUpJVENvZGUq IHJlc3VsdDsKKyAgICBzdGF0aWMgc3RkOjpvbmNlX2ZsYWcgb25jZUtleTsKKyAgICBzdGQ6OmNh bGxfb25jZShvbmNlS2V5LCBbJl0geworICAgICAgICByZXN1bHQgPSBuZXcgTmF0aXZlSklUQ29k ZShMTEludDo6Z2V0Q29kZVJlZjxKU0VudHJ5UHRyVGFnPihsbGludF9uYXRpdmVfY29uc3RydWN0 X3RyYW1wb2xpbmUpLCBKSVRDb2RlOjpIb3N0Q2FsbFRodW5rLCBOb0ludHJpbnNpYyk7CisgICAg fSk7CisgICAgcmV0dXJuIG1ha2VSZWYoKnJlc3VsdCk7Cit9CisKIE5hdGl2ZUV4ZWN1dGFibGUq IFZNOjpnZXRIb3N0RnVuY3Rpb24oTmF0aXZlRnVuY3Rpb24gZnVuY3Rpb24sIEludHJpbnNpYyBp bnRyaW5zaWMsIE5hdGl2ZUZ1bmN0aW9uIGNvbnN0cnVjdG9yLCBjb25zdCBET01KSVQ6OlNpZ25h dHVyZSogc2lnbmF0dXJlLCBjb25zdCBTdHJpbmcmIG5hbWUpCiB7CiAjaWYgRU5BQkxFKEpJVCkK QEAgLTcwMCwxMSArNzIwLDcgQEAgTmF0aXZlRXhlY3V0YWJsZSogVk06OmdldEhvc3RGdW5jdGlv bihOYXRpdmVGdW5jdGlvbiBmdW5jdGlvbiwgSW50cmluc2ljIGludHJpbnMKICNlbmRpZiAvLyBF TkFCTEUoSklUKQogICAgIFVOVVNFRF9QQVJBTShpbnRyaW5zaWMpOwogICAgIFVOVVNFRF9QQVJB TShzaWduYXR1cmUpOwotCi0gICAgcmV0dXJuIE5hdGl2ZUV4ZWN1dGFibGU6OmNyZWF0ZSgqdGhp cywKLSAgICAgICAgYWRvcHRSZWYoKm5ldyBOYXRpdmVKSVRDb2RlKExMSW50OjpnZXRDb2RlUmVm PEpTRW50cnlQdHJUYWc+KGxsaW50X25hdGl2ZV9jYWxsX3RyYW1wb2xpbmUpLCBKSVRDb2RlOjpI b3N0Q2FsbFRodW5rLCBOb0ludHJpbnNpYykpLCBmdW5jdGlvbiwKLSAgICAgICAgYWRvcHRSZWYo Km5ldyBOYXRpdmVKSVRDb2RlKExMSW50OjpnZXRDb2RlUmVmPEpTRW50cnlQdHJUYWc+KGxsaW50 X25hdGl2ZV9jb25zdHJ1Y3RfdHJhbXBvbGluZSksIEpJVENvZGU6Okhvc3RDYWxsVGh1bmssIE5v SW50cmluc2ljKSksIGNvbnN0cnVjdG9yLAotICAgICAgICBuYW1lKTsKKyAgICByZXR1cm4gTmF0 aXZlRXhlY3V0YWJsZTo6Y3JlYXRlKCp0aGlzLCBqaXRDb2RlRm9yQ2FsbFRyYW1wb2xpbmUoKSwg ZnVuY3Rpb24sIGppdENvZGVGb3JDb25zdHJ1Y3RUcmFtcG9saW5lKCksIGNvbnN0cnVjdG9yLCBu YW1lKTsKIH0KIAogTWFjcm9Bc3NlbWJsZXJDb2RlUHRyPEpTRW50cnlQdHJUYWc+IFZNOjpnZXRD VElJbnRlcm5hbEZ1bmN0aW9uVHJhbXBvbGluZUZvcihDb2RlU3BlY2lhbGl6YXRpb25LaW5kIGtp bmQpCg==