238263 2022-03-23 08:56:12 -0700 Avoid unnecessary String constructor under FunctionExecutable::toStringSlow() 2022-03-23 11:39:46 -0700 1 1 1 Unclassified WebKit JavaScriptCore WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED https://bugs.webkit.org/show_bug.cgi?id=238235 InRadar P2 Normal --- 1 cdumez cdumez darin ews-watchlist keith_miller mark.lam msaboff saam tzagallo webkit-bug-importer oldest_to_newest 1854190 0 cdumez 2022-03-23 08:56:12 -0700 Avoid unnecessary String constructor under FunctionExecutable::toStringSlow(). 1854191 1 455505 cdumez 2022-03-23 08:57:12 -0700 Created attachment 455505 Patch 1854194 2 455505 darin 2022-03-23 09:02:40 -0700 Comment on attachment 455505 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=455505&action=review > Source/JavaScriptCore/runtime/FunctionExecutable.cpp:144 > - String functionHeader; > + ASCIILiteral functionHeader = ASCIILiteral::null(); A few other thoughts: Why ASCIILiteral::null() here, but ""_s below? I’m not familiar with the variadic jsMakeNontrivialString. Does jsMakeNontrivialString benefit from this being an ASCIILiteral rather than a const char* or a StringView? Since src is a StringView I get the impression that we don’t need to make a String so not sure why ASCIILiteral is better than const char*. I’m sort of surprised that this idiom is jsMakeNontrivialString instead of just jsNontrivialString(makeString()). I think I would have done: auto functionHeader = ""; 1854200 3 cdumez 2022-03-23 09:10:17 -0700 (In reply to Darin Adler from comment #2) > Comment on attachment 455505 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=455505&action=review > > > Source/JavaScriptCore/runtime/FunctionExecutable.cpp:144 > > - String functionHeader; > > + ASCIILiteral functionHeader = ASCIILiteral::null(); > > A few other thoughts: Why ASCIILiteral::null() here, but ""_s below? I’m not > familiar with the variadic jsMakeNontrivialString. Does > jsMakeNontrivialString benefit from this being an ASCIILiteral rather than a > const char* or a StringView? Since src is a StringView I get the impression > that we don’t need to make a String so not sure why ASCIILiteral is better > than const char*. I’m sort of surprised that this idiom is > jsMakeNontrivialString instead of just jsNontrivialString(makeString()). > > I think I would have done: > > auto functionHeader = ""; Technically, the initialization value doesn't matter much since the switch below handles all enum values. Previously, the initialization value was a null string, not an empty string. ASCIILiteral::null() thus seemed closer to pre-existing code than ""_s. That said, I don't mind initializing to _""s. jsMakeNontrivialString() doesn't benefit from using an ASCIILiteral here. Using an ASCIILiteral also doesn't hurt as far as I can tell. Using ASCIILiteral also could be helpful for perf if we ever decide to store the string length as a data member in ASCIILiteral. I think it is best practice to use _s for all String literals in WebKit. It should always be as good or faster than "". If we have API that is slower with an ASCIILiteral, I think that API would need to be fixed. 1854219 4 455509 cdumez 2022-03-23 09:53:15 -0700 Created attachment 455509 Patch 1854303 5 ews-feeder 2022-03-23 11:38:32 -0700 Committed r291755 (248786@main): <https://commits.webkit.org/248786@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 455509. 1854304 6 webkit-bug-importer 2022-03-23 11:39:46 -0700 <rdar://problem/90713418> 455505 2022-03-23 08:57:12 -0700 2022-03-23 09:53:12 -0700 Patch bug-238263-20220323085711.patch text/plain 1792 cdumez U3VidmVyc2lvbiBSZXZpc2lvbjogMjkxNzQ1CmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvQ2hhbmdlTG9nIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwppbmRleCAw OGVlYjIwODc1NjkzZGM2ZmNjODdjZWUwMTUzNzAyMzM1MjVmNDY5Li45OTY4Njc0MTJkNDZiOGIw MmZmYzZhMzk4NGJlZDE5NWMwMzMyNjUzIDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwxMyBAQAorMjAyMi0wMy0yMyAgQ2hyaXMgRHVtZXogIDxjZHVtZXpAYXBwbGUuY29tPgor CisgICAgICAgIEF2b2lkIHVubmVjZXNzYXJ5IFN0cmluZyBjb25zdHJ1Y3RvciB1bmRlciBGdW5j dGlvbkV4ZWN1dGFibGU6OnRvU3RyaW5nU2xvdygpCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJr aXQub3JnL3Nob3dfYnVnLmNnaT9pZD0yMzgyNjMKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JP RFkgKE9PUFMhKS4KKworICAgICAgICAqIHJ1bnRpbWUvRnVuY3Rpb25FeGVjdXRhYmxlLmNwcDoK KyAgICAgICAgKEpTQzo6RnVuY3Rpb25FeGVjdXRhYmxlOjp0b1N0cmluZ1Nsb3cpOgorCiAyMDIy LTAzLTIzICBYYW4gTG9wZXogIDx4YW5AaWdhbGlhLmNvbT4KIAogICAgICAgICBbSlNDXSBBZGQg RG9Ob3RIYXZlVGFnUmVnaXN0ZXJzIG1vZGUgdG8gdW5ib3hEb3VibGUKZGlmZiAtLWdpdCBhL1Nv dXJjZS9KYXZhU2NyaXB0Q29yZS9ydW50aW1lL0Z1bmN0aW9uRXhlY3V0YWJsZS5jcHAgYi9Tb3Vy Y2UvSmF2YVNjcmlwdENvcmUvcnVudGltZS9GdW5jdGlvbkV4ZWN1dGFibGUuY3BwCmluZGV4IGYx NDlkNGU1OWE3MjRhODQyNTdmYjY4Nzg0OTJjMmFhM2IxNjJlOGIuLmJlMDIxMDMwN2FhYWY0YWFi YjBiMTg0ZDhjZGYyOTQwZDQ2OWJhMTUgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9KYXZhU2NyaXB0Q29y ZS9ydW50aW1lL0Z1bmN0aW9uRXhlY3V0YWJsZS5jcHAKKysrIGIvU291cmNlL0phdmFTY3JpcHRD b3JlL3J1bnRpbWUvRnVuY3Rpb25FeGVjdXRhYmxlLmNwcApAQCAtMTQxLDcgKzE0MSw3IEBAIEpT U3RyaW5nKiBGdW5jdGlvbkV4ZWN1dGFibGU6OnRvU3RyaW5nU2xvdyhKU0dsb2JhbE9iamVjdCog Z2xvYmFsT2JqZWN0KQogICAgIGlmIChpc0NsYXNzKCkpCiAgICAgICAgIHJldHVybiBjYWNoZShq c1N0cmluZyh2bSwgY2xhc3NTb3VyY2UoKS52aWV3KCkudG9TdHJpbmcoKSkpOwogCi0gICAgU3Ry aW5nIGZ1bmN0aW9uSGVhZGVyOworICAgIEFTQ0lJTGl0ZXJhbCBmdW5jdGlvbkhlYWRlciA9IEFT Q0lJTGl0ZXJhbDo6bnVsbCgpOwogICAgIHN3aXRjaCAocGFyc2VNb2RlKCkpIHsKICAgICBjYXNl IFNvdXJjZVBhcnNlTW9kZTo6R2VuZXJhdG9yV3JhcHBlckZ1bmN0aW9uTW9kZToKICAgICBjYXNl IFNvdXJjZVBhcnNlTW9kZTo6R2VuZXJhdG9yV3JhcHBlck1ldGhvZE1vZGU6CkBAIC0xNjQsNyAr MTY0LDcgQEAgSlNTdHJpbmcqIEZ1bmN0aW9uRXhlY3V0YWJsZTo6dG9TdHJpbmdTbG93KEpTR2xv YmFsT2JqZWN0KiBnbG9iYWxPYmplY3QpCiAKICAgICBjYXNlIFNvdXJjZVBhcnNlTW9kZTo6QXJy b3dGdW5jdGlvbk1vZGU6CiAgICAgY2FzZSBTb3VyY2VQYXJzZU1vZGU6OkNsYXNzRmllbGRJbml0 aWFsaXplck1vZGU6Ci0gICAgICAgIGZ1bmN0aW9uSGVhZGVyID0gIiI7CisgICAgICAgIGZ1bmN0 aW9uSGVhZGVyID0gIiJfczsKICAgICAgICAgYnJlYWs7CiAKICAgICBjYXNlIFNvdXJjZVBhcnNl TW9kZTo6QXN5bmNGdW5jdGlvbk1vZGU6Cg== 455509 2022-03-23 09:53:15 -0700 2022-03-23 11:38:35 -0700 Patch bug-238263-20220323095314.patch text/plain 1741 cdumez U3VidmVyc2lvbiBSZXZpc2lvbjogMjkxNzQ1CmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvQ2hhbmdlTG9nIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwppbmRleCAw OGVlYjIwODc1NjkzZGM2ZmNjODdjZWUwMTUzNzAyMzM1MjVmNDY5Li41NWI5YmM3NjA3MTljMjQ0 OGExNjcwMzYxOWM3Y2JmMzViZjViMjBlIDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwxMyBAQAorMjAyMi0wMy0yMyAgQ2hyaXMgRHVtZXogIDxjZHVtZXpAYXBwbGUuY29tPgor CisgICAgICAgIEF2b2lkIHVubmVjZXNzYXJ5IFN0cmluZyBjb25zdHJ1Y3RvciB1bmRlciBGdW5j dGlvbkV4ZWN1dGFibGU6OnRvU3RyaW5nU2xvdygpCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJr aXQub3JnL3Nob3dfYnVnLmNnaT9pZD0yMzgyNjMKKworICAgICAgICBSZXZpZXdlZCBieSBEYXJp biBBZGxlci4KKworICAgICAgICAqIHJ1bnRpbWUvRnVuY3Rpb25FeGVjdXRhYmxlLmNwcDoKKyAg ICAgICAgKEpTQzo6RnVuY3Rpb25FeGVjdXRhYmxlOjp0b1N0cmluZ1Nsb3cpOgorCiAyMDIyLTAz LTIzICBYYW4gTG9wZXogIDx4YW5AaWdhbGlhLmNvbT4KIAogICAgICAgICBbSlNDXSBBZGQgRG9O b3RIYXZlVGFnUmVnaXN0ZXJzIG1vZGUgdG8gdW5ib3hEb3VibGUKZGlmZiAtLWdpdCBhL1NvdXJj ZS9KYXZhU2NyaXB0Q29yZS9ydW50aW1lL0Z1bmN0aW9uRXhlY3V0YWJsZS5jcHAgYi9Tb3VyY2Uv SmF2YVNjcmlwdENvcmUvcnVudGltZS9GdW5jdGlvbkV4ZWN1dGFibGUuY3BwCmluZGV4IGYxNDlk NGU1OWE3MjRhODQyNTdmYjY4Nzg0OTJjMmFhM2IxNjJlOGIuLjIzMDdhY2VjYzUwNGRlYTc1OTQx MDk5MjQ3NGJiYmNhNTllYjEwZjEgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9y dW50aW1lL0Z1bmN0aW9uRXhlY3V0YWJsZS5jcHAKKysrIGIvU291cmNlL0phdmFTY3JpcHRDb3Jl L3J1bnRpbWUvRnVuY3Rpb25FeGVjdXRhYmxlLmNwcApAQCAtMTQxLDcgKzE0MSw3IEBAIEpTU3Ry aW5nKiBGdW5jdGlvbkV4ZWN1dGFibGU6OnRvU3RyaW5nU2xvdyhKU0dsb2JhbE9iamVjdCogZ2xv YmFsT2JqZWN0KQogICAgIGlmIChpc0NsYXNzKCkpCiAgICAgICAgIHJldHVybiBjYWNoZShqc1N0 cmluZyh2bSwgY2xhc3NTb3VyY2UoKS52aWV3KCkudG9TdHJpbmcoKSkpOwogCi0gICAgU3RyaW5n IGZ1bmN0aW9uSGVhZGVyOworICAgIEFTQ0lJTGl0ZXJhbCBmdW5jdGlvbkhlYWRlciA9ICIiX3M7 CiAgICAgc3dpdGNoIChwYXJzZU1vZGUoKSkgewogICAgIGNhc2UgU291cmNlUGFyc2VNb2RlOjpH ZW5lcmF0b3JXcmFwcGVyRnVuY3Rpb25Nb2RlOgogICAgIGNhc2UgU291cmNlUGFyc2VNb2RlOjpH ZW5lcmF0b3JXcmFwcGVyTWV0aG9kTW9kZToKQEAgLTE2NCw3ICsxNjQsNiBAQCBKU1N0cmluZyog RnVuY3Rpb25FeGVjdXRhYmxlOjp0b1N0cmluZ1Nsb3coSlNHbG9iYWxPYmplY3QqIGdsb2JhbE9i amVjdCkKIAogICAgIGNhc2UgU291cmNlUGFyc2VNb2RlOjpBcnJvd0Z1bmN0aW9uTW9kZToKICAg ICBjYXNlIFNvdXJjZVBhcnNlTW9kZTo6Q2xhc3NGaWVsZEluaXRpYWxpemVyTW9kZToKLSAgICAg ICAgZnVuY3Rpb25IZWFkZXIgPSAiIjsKICAgICAgICAgYnJlYWs7CiAKICAgICBjYXNlIFNvdXJj ZVBhcnNlTW9kZTo6QXN5bmNGdW5jdGlvbk1vZGU6Cg==