185083 2018-04-27 10:57:38 -0700 Remove unneeded exception check from String.fromCharCode 2018-04-30 14:39:43 -0700 1 1 1 Unclassified WebKit New Bugs WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 keith_miller keith_miller commit-queue darin ews-watchlist mark.lam msaboff saam webkit-bug-importer oldest_to_newest 1418548 0 keith_miller 2018-04-27 10:57:38 -0700 Remove unneeded exception check from String.fromCharCode 1418559 1 339007 keith_miller 2018-04-27 11:14:49 -0700 Created attachment 339007 Patch 1418569 2 339008 keith_miller 2018-04-27 11:32:56 -0700 Created attachment 339008 Patch 1419001 3 339008 darin 2018-04-30 09:41:46 -0700 Comment on attachment 339008 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=339008&action=review > Source/JavaScriptCore/runtime/StringConstructor.cpp:80 > scope.release(); Can we move the declarations of "vm" and "scope" after the if statement and remove this line of code too? 1419007 4 339008 mark.lam 2018-04-30 09:45:43 -0700 Comment on attachment 339008 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=339008&action=review >> Source/JavaScriptCore/runtime/StringConstructor.cpp:80 >> scope.release(); > > Can we move the declarations of "vm" and "scope" after the if statement and remove this line of code too? Wouldn't the compiler know that vm and scope is not used until after this anyway? scope.release() is a no-op on release builds. I suspect that this move is unnecessary. 1419017 5 339008 darin 2018-04-30 09:57:42 -0700 Comment on attachment 339008 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=339008&action=review >>> Source/JavaScriptCore/runtime/StringConstructor.cpp:80 >>> scope.release(); >> >> Can we move the declarations of "vm" and "scope" after the if statement and remove this line of code too? > > Wouldn't the compiler know that vm and scope is not used until after this anyway? scope.release() is a no-op on release builds. I suspect that this move is unnecessary. Sure, it’s unnecessary to generate optimal code if it has no effect on the generated code. But it’s also unnecessary to write the code the way it currently is; I think it’s clearer the way I suggest, and that’s how it was until a recent change, too. The fast case is clearly separated from the rest of the code with no exception handling code in it. 1419027 6 339008 mark.lam 2018-04-30 10:08:46 -0700 Comment on attachment 339008 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=339008&action=review >>>> Source/JavaScriptCore/runtime/StringConstructor.cpp:80 >>>> scope.release(); >>> >>> Can we move the declarations of "vm" and "scope" after the if statement and remove this line of code too? >> >> Wouldn't the compiler know that vm and scope is not used until after this anyway? scope.release() is a no-op on release builds. I suspect that this move is unnecessary. > > Sure, it’s unnecessary to generate optimal code if it has no effect on the generated code. > > But it’s also unnecessary to write the code the way it currently is; I think it’s clearer the way I suggest, and that’s how it was until a recent change, too. The fast case is clearly separated from the rest of the code with no exception handling code in it. Actually, there's a need to declare the ThrowScope at the very top, and by convention we always do this where a ThrowScope is needed, unless there is a special reasons to make an exception from this rule (more on this below). The reason for always declaring the ThrowScope at the top is because the purpose of a ThrowScope is to communicate to its caller that this function "may" throw an exception regardless which path is taken during the current invocation (fast, or slow). This allows us to assert in the parent that a potential exception could have been thrown, and add an exception there if needed. By declaring the ThrowScope later, we will cease to notify the caller that this function can potentially throw an exception, thereby defeating its purpose. The only times we don't declare a ThrowScope at the top (and there should be very few of these) is if the function will actually eat / handle all exceptions that it encounters (i.e. it should have a CatchScope at the top), but in an embedded block, it needs a ThrowScope for various reasons. There may be other reasons for not declaring the ThrowScope at the top. but this is the most obvious one I can think of at the moment. In the case of stringFromCharCode(), I see no special reason why we should not declare ThrowScope at the top. 1419109 7 339008 mark.lam 2018-04-30 14:06:42 -0700 Comment on attachment 339008 Patch r=me 1419111 8 339008 keith_miller 2018-04-30 14:11:31 -0700 Comment on attachment 339008 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=339008&action=review Thanks for the review! >>>>> Source/JavaScriptCore/runtime/StringConstructor.cpp:80 >>>>> scope.release(); >>>> >>>> Can we move the declarations of "vm" and "scope" after the if statement and remove this line of code too? >>> >>> Wouldn't the compiler know that vm and scope is not used until after this anyway? scope.release() is a no-op on release builds. I suspect that this move is unnecessary. >> >> Sure, it’s unnecessary to generate optimal code if it has no effect on the generated code. >> >> But it’s also unnecessary to write the code the way it currently is; I think it’s clearer the way I suggest, and that’s how it was until a recent change, too. The fast case is clearly separated from the rest of the code with no exception handling code in it. > > Actually, there's a need to declare the ThrowScope at the very top, and by convention we always do this where a ThrowScope is needed, unless there is a special reasons to make an exception from this rule (more on this below). > > The reason for always declaring the ThrowScope at the top is because the purpose of a ThrowScope is to communicate to its caller that this function "may" throw an exception regardless which path is taken during the current invocation (fast, or slow). This allows us to assert in the parent that a potential exception could have been thrown, and add an exception there if needed. By declaring the ThrowScope later, we will cease to notify the caller that this function can potentially throw an exception, thereby defeating its purpose. > > The only times we don't declare a ThrowScope at the top (and there should be very few of these) is if the function will actually eat / handle all exceptions that it encounters (i.e. it should have a CatchScope at the top), but in an embedded block, it needs a ThrowScope for various reasons. There may be other reasons for not declaring the ThrowScope at the top. but this is the most obvious one I can think of at the moment. In the case of stringFromCharCode(), I see no special reason why we should not declare ThrowScope at the top. I agree with Mark here. Knowing which functions can throw exceptions is very useful. Given that the compiler will not emit any extra code in release there's really no reason to not have the ThrowScope at the top of the function. 1419118 9 339008 commit-queue 2018-04-30 14:38:32 -0700 Comment on attachment 339008 Patch Clearing flags on attachment: 339008 Committed r231171: <https://trac.webkit.org/changeset/231171> 1419119 10 commit-queue 2018-04-30 14:38:33 -0700 All reviewed patches have been landed. Closing bug. 1419120 11 webkit-bug-importer 2018-04-30 14:39:43 -0700 <rdar://problem/39849327> 339007 2018-04-27 11:14:49 -0700 2018-04-27 11:32:54 -0700 Patch bug-185083-20180427111448.patch text/plain 1544 keith_miller U3VidmVyc2lvbiBSZXZpc2lvbjogMjMxMTAxCmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvQ2hhbmdlTG9nIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwppbmRleCBk ZTJhYTQ0OWNhNTc5MmE4MTMyMGYxMWZmZjVjNzJiYmZlOGM0ZGUxLi5iZTJiNDg4Mzk4MDM4OGM0 ZmVhMDE4ZTk2NGY3N2E1YWM4MWFkYTJmIDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwxMyBAQAorMjAxOC0wNC0yNyAgS2VpdGggTWlsbGVyICA8a2VpdGhfbWlsbGVyQGFwcGxl LmNvbT4KKworICAgICAgICBSZW1vdmUgdW5uZWVkZWQgZXhjZXB0aW9uIGNoZWNrIGZyb20gU3Ry aW5nLmZyb21DaGFyQ29kZQorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1 Zy5jZ2k/aWQ9MTg1MDgzCisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisK KyAgICAgICAgKiBydW50aW1lL1N0cmluZ0NvbnN0cnVjdG9yLmNwcDoKKyAgICAgICAgKEpTQzo6 c3RyaW5nRnJvbUNoYXJDb2RlKToKKwogMjAxOC0wNC0yNiAgQ2FpbyBMaW1hICA8dGljYWlvbGlt YUBnbWFpbC5jb20+CiAKICAgICAgICAgW0VTTmV4dF1bQmlnSW50XSBJbXBsZW1lbnQgc3VwcG9y dCBmb3IgIioiIG9wZXJhdGlvbgpkaWZmIC0tZ2l0IGEvU291cmNlL0phdmFTY3JpcHRDb3JlL3J1 bnRpbWUvU3RyaW5nQ29uc3RydWN0b3IuY3BwIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL3J1bnRp bWUvU3RyaW5nQ29uc3RydWN0b3IuY3BwCmluZGV4IDEwYWJjNzYxMDVkMmIwZDBlZWE5ODI1OWY3 MGMyZDQ5M2FkZDkwYWEuLjM1ZWVjNzliZjc2ZmNmMjRmYzg1NGJkMzRmNGZiNTQ0MTQ3ZTliOGQg MTAwNjQ0Ci0tLSBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9ydW50aW1lL1N0cmluZ0NvbnN0cnVj dG9yLmNwcAorKysgYi9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvcnVudGltZS9TdHJpbmdDb25zdHJ1 Y3Rvci5jcHAKQEAgLTc4LDggKzc4LDggQEAgc3RhdGljIEVuY29kZWRKU1ZhbHVlIEpTQ19IT1NU X0NBTEwgc3RyaW5nRnJvbUNoYXJDb2RlKEV4ZWNTdGF0ZSogZXhlYykKICAgICB1bnNpZ25lZCBs ZW5ndGggPSBleGVjLT5hcmd1bWVudENvdW50KCk7CiAgICAgaWYgKExJS0VMWShsZW5ndGggPT0g MSkpIHsKICAgICAgICAgdW5zaWduZWQgY29kZSA9IGV4ZWMtPnVuY2hlY2tlZEFyZ3VtZW50KDAp LnRvVUludDMyKGV4ZWMpOwotICAgICAgICBSRVRVUk5fSUZfRVhDRVBUSU9OKHNjb3BlLCBlbmNv ZGVkSlNWYWx1ZSgpKTsKICAgICAgICAgc2NvcGUucmVsZWFzZSgpOworICAgICAgICAvLyBUaGlz IGlzIG9rIGJlY2F1c2UganNTaW5nbGVDaGFyYWN0ZXJTdHJpbmcgd2lsbCBqdXN0IGZldGNoIGFu IHVudXNlZCBzdHJpbmcgaWYgdGhlcmUncyBhbiBleGNlcHRpb24uCiAgICAgICAgIHJldHVybiBK U1ZhbHVlOjplbmNvZGUoanNTaW5nbGVDaGFyYWN0ZXJTdHJpbmcoZXhlYywgY29kZSkpOwogICAg IH0KIAo= 339008 2018-04-27 11:32:56 -0700 2018-04-30 14:38:32 -0700 Patch bug-185083-20180427113255.patch text/plain 1644 keith_miller U3VidmVyc2lvbiBSZXZpc2lvbjogMjMxMTAxCmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvQ2hhbmdlTG9nIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwppbmRleCBk ZTJhYTQ0OWNhNTc5MmE4MTMyMGYxMWZmZjVjNzJiYmZlOGM0ZGUxLi5iZTJiNDg4Mzk4MDM4OGM0 ZmVhMDE4ZTk2NGY3N2E1YWM4MWFkYTJmIDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwxMyBAQAorMjAxOC0wNC0yNyAgS2VpdGggTWlsbGVyICA8a2VpdGhfbWlsbGVyQGFwcGxl LmNvbT4KKworICAgICAgICBSZW1vdmUgdW5uZWVkZWQgZXhjZXB0aW9uIGNoZWNrIGZyb20gU3Ry aW5nLmZyb21DaGFyQ29kZQorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1 Zy5jZ2k/aWQ9MTg1MDgzCisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisK KyAgICAgICAgKiBydW50aW1lL1N0cmluZ0NvbnN0cnVjdG9yLmNwcDoKKyAgICAgICAgKEpTQzo6 c3RyaW5nRnJvbUNoYXJDb2RlKToKKwogMjAxOC0wNC0yNiAgQ2FpbyBMaW1hICA8dGljYWlvbGlt YUBnbWFpbC5jb20+CiAKICAgICAgICAgW0VTTmV4dF1bQmlnSW50XSBJbXBsZW1lbnQgc3VwcG9y dCBmb3IgIioiIG9wZXJhdGlvbgpkaWZmIC0tZ2l0IGEvU291cmNlL0phdmFTY3JpcHRDb3JlL3J1 bnRpbWUvU3RyaW5nQ29uc3RydWN0b3IuY3BwIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL3J1bnRp bWUvU3RyaW5nQ29uc3RydWN0b3IuY3BwCmluZGV4IDEwYWJjNzYxMDVkMmIwZDBlZWE5ODI1OWY3 MGMyZDQ5M2FkZDkwYWEuLjBiMTBkOGY0MGI4MzYzY2JkMDQzNjg5ODZjMzM5YjU5NTEyYjIwOWUg MTAwNjQ0Ci0tLSBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9ydW50aW1lL1N0cmluZ0NvbnN0cnVj dG9yLmNwcAorKysgYi9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvcnVudGltZS9TdHJpbmdDb25zdHJ1 Y3Rvci5jcHAKQEAgLTc3LDkgKzc3LDkgQEAgc3RhdGljIEVuY29kZWRKU1ZhbHVlIEpTQ19IT1NU X0NBTEwgc3RyaW5nRnJvbUNoYXJDb2RlKEV4ZWNTdGF0ZSogZXhlYykKIAogICAgIHVuc2lnbmVk IGxlbmd0aCA9IGV4ZWMtPmFyZ3VtZW50Q291bnQoKTsKICAgICBpZiAoTElLRUxZKGxlbmd0aCA9 PSAxKSkgewotICAgICAgICB1bnNpZ25lZCBjb2RlID0gZXhlYy0+dW5jaGVja2VkQXJndW1lbnQo MCkudG9VSW50MzIoZXhlYyk7Ci0gICAgICAgIFJFVFVSTl9JRl9FWENFUFRJT04oc2NvcGUsIGVu Y29kZWRKU1ZhbHVlKCkpOwogICAgICAgICBzY29wZS5yZWxlYXNlKCk7CisgICAgICAgIHVuc2ln bmVkIGNvZGUgPSBleGVjLT51bmNoZWNrZWRBcmd1bWVudCgwKS50b1VJbnQzMihleGVjKTsKKyAg ICAgICAgLy8gTm90IGNoZWNraW5nIGZvciBhbiBleGNlcHRpb24gaGVyZSBpcyBvayBiZWNhdXNl IGpzU2luZ2xlQ2hhcmFjdGVyU3RyaW5nIHdpbGwganVzdCBmZXRjaCBhbiB1bnVzZWQgc3RyaW5n IGlmIHRoZXJlJ3MgYW4gZXhjZXB0aW9uLgogICAgICAgICByZXR1cm4gSlNWYWx1ZTo6ZW5jb2Rl KGpzU2luZ2xlQ2hhcmFjdGVyU3RyaW5nKGV4ZWMsIGNvZGUpKTsKICAgICB9CiAK