236286 2022-02-07 21:46:57 -0800 Reduce allocations and increase thread safety of constructedPath 2022-02-08 09:47:06 -0800 1 1 1 Unclassified WebKit New Bugs WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 achristensen achristensen cdumez darin ggaren webkit-bug-importer oldest_to_newest 1838529 0 achristensen 2022-02-07 21:46:57 -0800 Reduce allocations and increase thread safety of constructedPath 1838531 1 451208 achristensen 2022-02-07 21:47:42 -0800 Created attachment 451208 Patch 1838532 2 achristensen 2022-02-07 21:47:46 -0800 <rdar://problem/86904276> 1838651 3 451208 cdumez 2022-02-08 07:27:31 -0800 Comment on attachment 451208 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=451208&action=review r=me with improvement suggestion. > Source/WebKit/UIProcess/API/APIContentRuleListStore.cpp:85 > + static auto* prefix("ContentRuleList-"); I think you could use: static const prefix[] = "ContentRuleList-"; > Source/WebKit/UIProcess/API/APIContentRuleListStore.cpp:524 > + auto prefixLength = strlen(prefix); `sizeof(prefix) - 1` which would be a bit more efficient. 1838702 4 451208 achristensen 2022-02-08 08:59:38 -0800 Comment on attachment 451208 Patch The optimizer is able to see that they are the same. 1838703 5 ews-feeder 2022-02-08 09:04:53 -0800 Committed r289380 (246968@main): <https://commits.webkit.org/246968@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 451208. 1838715 6 451208 cdumez 2022-02-08 09:21:43 -0800 Comment on attachment 451208 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=451208&action=review >> Source/WebKit/UIProcess/API/APIContentRuleListStore.cpp:85 >> + static auto* prefix("ContentRuleList-"); > > I think you could use: > static const prefix[] = "ContentRuleList-"; Whether you want to keep calling strlen() or not fine. But I believe that: `static const prefix[] = "ContentRuleList-";` is strictly superior to: `static auto* prefix("ContentRuleList-");` 1838718 7 451208 cdumez 2022-02-08 09:39:12 -0800 Comment on attachment 451208 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=451208&action=review >>> Source/WebKit/UIProcess/API/APIContentRuleListStore.cpp:85 >>> + static auto* prefix("ContentRuleList-"); >> >> I think you could use: >> static const prefix[] = "ContentRuleList-"; > > Whether you want to keep calling strlen() or not fine. But I believe that: > `static const prefix[] = "ContentRuleList-";` > is strictly superior to: > `static auto* prefix("ContentRuleList-");` In particular, I believe the using the const array will allow the compiler to put it in the .rodata section of the binary, which may have an impact of perf. This is unrelated to the fact that strlen() may not may not be optimized in this case. 1838719 8 451208 darin 2022-02-08 09:39:28 -0800 Comment on attachment 451208 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=451208&action=review >>> Source/WebKit/UIProcess/API/APIContentRuleListStore.cpp:85 >>> + static auto* prefix("ContentRuleList-"); >> >> I think you could use: >> static const prefix[] = "ContentRuleList-"; > > Whether you want to keep calling strlen() or not fine. But I believe that: > `static const prefix[] = "ContentRuleList-";` > is strictly superior to: > `static auto* prefix("ContentRuleList-");` I like one of these: constexpr auto prefix = "ContentRuleList-"; constexpr auto prefix[] = "ContentRuleList-"; >> Source/WebKit/UIProcess/API/APIContentRuleListStore.cpp:524 >> + auto prefixLength = strlen(prefix); > > `sizeof(prefix) - 1` > > which would be a bit more efficient. In clang strlen is optimized and constant folded, so there is no performance difference. 1838720 9 darin 2022-02-08 09:42:33 -0800 I did a little testing and the code is identical between these two: constexpr auto prefix = "ContentRuleList-"; constexpr char prefix[] = "ContentRuleList-"; Both put the string into the .rodata section. 1838722 10 451208 cdumez 2022-02-08 09:43:58 -0800 Comment on attachment 451208 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=451208&action=review >>>>> Source/WebKit/UIProcess/API/APIContentRuleListStore.cpp:85 >>>>> + static auto* prefix("ContentRuleList-"); >>>> >>>> I think you could use: >>>> static const prefix[] = "ContentRuleList-"; >>> >>> Whether you want to keep calling strlen() or not fine. But I believe that: >>> `static const prefix[] = "ContentRuleList-";` >>> is strictly superior to: >>> `static auto* prefix("ContentRuleList-");` >> >> In particular, I believe the using the const array will allow the compiler to put it in the .rodata section of the binary, which may have an impact of perf. >> >> This is unrelated to the fact that strlen() may not may not be optimized in this case. > > I like one of these: > > constexpr auto prefix = "ContentRuleList-"; > constexpr auto prefix[] = "ContentRuleList-"; Right but the code that landed used `static auto* prefix("ContentRuleList-");` and I am less convinced this goes into .rodata (though I haven't verified). 1838728 11 451208 darin 2022-02-08 09:47:06 -0800 Comment on attachment 451208 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=451208&action=review >>>>>> Source/WebKit/UIProcess/API/APIContentRuleListStore.cpp:85 >>>>>> + static auto* prefix("ContentRuleList-"); >>>>> >>>>> I think you could use: >>>>> static const prefix[] = "ContentRuleList-"; >>>> >>>> Whether you want to keep calling strlen() or not fine. But I believe that: >>>> `static const prefix[] = "ContentRuleList-";` >>>> is strictly superior to: >>>> `static auto* prefix("ContentRuleList-");` >>> >>> In particular, I believe the using the const array will allow the compiler to put it in the .rodata section of the binary, which may have an impact of perf. >>> >>> This is unrelated to the fact that strlen() may not may not be optimized in this case. >> >> I like one of these: >> >> constexpr auto prefix = "ContentRuleList-"; >> constexpr auto prefix[] = "ContentRuleList-"; > > Right but the code that landed used `static auto* prefix("ContentRuleList-");` and I am less convinced this goes into .rodata (though I haven't verified). Yes, I think it has too little const. It’s a non-const global variable. More constexpr would fix it. Whether you left "static" in or not, and whether you used "auto*" or "auto". 451208 2022-02-07 21:47:42 -0800 2022-02-08 09:04:55 -0800 Patch bug-236286-20220207214741.patch text/plain 2601 achristensen U3VidmVyc2lvbiBSZXZpc2lvbjogMjg5MjM3CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L0No YW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCmluZGV4IDllMTZmOTBhMGZhZDY5MmUx MDA5ZTkyZDNlNDQzNDg4ZTJiZGJlMDkuLmY1NjZlZDUxNWQ2NTRiY2MxNDM5ZWQ5MTk0MmE2ZmRh ODY0NDNiZGIgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCisrKyBiL1NvdXJj ZS9XZWJLaXQvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTUgQEAKKzIwMjItMDItMDcgIEFsZXggQ2hy aXN0ZW5zZW4gIDxhY2hyaXN0ZW5zZW5Ad2Via2l0Lm9yZz4KKworICAgICAgICBSZWR1Y2UgYWxs b2NhdGlvbnMgYW5kIGluY3JlYXNlIHRocmVhZCBzYWZldHkgb2YgY29uc3RydWN0ZWRQYXRoCisg ICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0yMzYyODYKKyAg ICAgICAgPHJkYXI6Ly84NjkwNDI3Nj4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9P UFMhKS4KKworICAgICAgICAqIFVJUHJvY2Vzcy9BUEkvQVBJQ29udGVudFJ1bGVMaXN0U3RvcmUu Y3BwOgorICAgICAgICAoQVBJOjpjb25zdHJ1Y3RlZFBhdGhQcmVmaXgpOgorICAgICAgICAoQVBJ OjpDb250ZW50UnVsZUxpc3RTdG9yZTo6Z2V0QXZhaWxhYmxlQ29udGVudFJ1bGVMaXN0SWRlbnRp ZmllcnMpOgorCiAyMDIyLTAyLTA3ICBXZW5zb24gSHNpZWggIDx3ZW5zb25faHNpZWhAYXBwbGUu Y29tPgogCiAgICAgICAgIEFkZCBQYWdlQ2xpZW50IHBsdW1iaW5nIGZvciB2aWRlbyBmcmFtZSBl eHRyYWN0aW9uCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L1VJUHJvY2Vzcy9BUEkvQVBJQ29u dGVudFJ1bGVMaXN0U3RvcmUuY3BwIGIvU291cmNlL1dlYktpdC9VSVByb2Nlc3MvQVBJL0FQSUNv bnRlbnRSdWxlTGlzdFN0b3JlLmNwcAppbmRleCAzOTY1YWFlZTVhNzEwNjFmNDljYTU4ODQxNTY1 MjUxZjNkMDk5ZDdiLi5iMmQ4NjEyMGQwNWNjYzY0YzJkNDc4MDBlY2QxYTYyNDE1NmUyZDgxIDEw MDY0NAotLS0gYS9Tb3VyY2UvV2ViS2l0L1VJUHJvY2Vzcy9BUEkvQVBJQ29udGVudFJ1bGVMaXN0 U3RvcmUuY3BwCisrKyBiL1NvdXJjZS9XZWJLaXQvVUlQcm9jZXNzL0FQSS9BUElDb250ZW50UnVs ZUxpc3RTdG9yZS5jcHAKQEAgLTgwLDEwICs4MCwxMCBAQCBDb250ZW50UnVsZUxpc3RTdG9yZTo6 Q29udGVudFJ1bGVMaXN0U3RvcmUoY29uc3QgV1RGOjpTdHJpbmcmIHN0b3JlUGF0aCkKIENvbnRl bnRSdWxlTGlzdFN0b3JlOjp+Q29udGVudFJ1bGVMaXN0U3RvcmUoKSA9IGRlZmF1bHQ7CiAKIC8v IEZJWE1FOiBSZW1vdmUgbGVnYWN5RmlsZW5hbWUgaW4gMjAyMiBvciAyMDIzIGFmdGVyIHVzZXJz IGhhdmUgaGFkIGEgY2hhbmNlIHRvIHJ1biB0aGUgdXBkYXRpbmcgbG9naWMuCi1zdGF0aWMgY29u c3QgV1RGOjpTdHJpbmcmIGNvbnN0cnVjdGVkUGF0aFByZWZpeChib29sIGxlZ2FjeUZpbGVuYW1l KQorc3RhdGljIGNvbnN0IGNoYXIqIGNvbnN0cnVjdGVkUGF0aFByZWZpeChib29sIGxlZ2FjeUZp bGVuYW1lKQogewotICAgIHN0YXRpYyBOZXZlckRlc3Ryb3llZDxXVEY6OlN0cmluZz4gcHJlZml4 KCJDb250ZW50UnVsZUxpc3QtIik7Ci0gICAgc3RhdGljIE5ldmVyRGVzdHJveWVkPFdURjo6U3Ry aW5nPiBsZWdhY3lQcmVmaXgoIkNvbnRlbnRFeHRlbnNpb24tIik7CisgICAgc3RhdGljIGF1dG8q IHByZWZpeCgiQ29udGVudFJ1bGVMaXN0LSIpOworICAgIHN0YXRpYyBhdXRvKiBsZWdhY3lQcmVm aXgoIkNvbnRlbnRFeHRlbnNpb24tIik7CiAgICAgaWYgKGxlZ2FjeUZpbGVuYW1lKQogICAgICAg ICByZXR1cm4gbGVnYWN5UHJlZml4OwogICAgIHJldHVybiBwcmVmaXg7CkBAIC01MjEsOSArNTIx LDkgQEAgdm9pZCBDb250ZW50UnVsZUxpc3RTdG9yZTo6Z2V0QXZhaWxhYmxlQ29udGVudFJ1bGVM aXN0SWRlbnRpZmllcnMoQ29tcGxldGlvbkhhbmQKICAgICBBU1NFUlQoUnVuTG9vcDo6aXNNYWlu KCkpOwogICAgIG1fcmVhZFF1ZXVlLT5kaXNwYXRjaChbcHJvdGVjdGVkVGhpcyA9IFJlZiB7ICp0 aGlzIH0sIHN0b3JlUGF0aCA9IG1fc3RvcmVQYXRoLmlzb2xhdGVkQ29weSgpLCBjb21wbGV0aW9u SGFuZGxlciA9IFdURk1vdmUoY29tcGxldGlvbkhhbmRsZXIpXSgpIG11dGFibGUgewogICAgICAg ICBhdXRvIHByZWZpeCA9IGNvbnN0cnVjdGVkUGF0aFByZWZpeChmYWxzZSAvKmxlZ2FjeSovKTsK LSAgICAgICAgYXV0byBwcmVmaXhMZW5ndGggPSBwcmVmaXgubGVuZ3RoKCk7CisgICAgICAgIGF1 dG8gcHJlZml4TGVuZ3RoID0gc3RybGVuKHByZWZpeCk7CiAgICAgICAgIGF1dG8gbGVnYWN5UHJl Zml4ID0gY29uc3RydWN0ZWRQYXRoUHJlZml4KHRydWUgLypsZWdhY3kqLyk7Ci0gICAgICAgIGF1 dG8gbGVnYWN5UHJlZml4TGVuZ3RoID0gbGVnYWN5UHJlZml4Lmxlbmd0aCgpOworICAgICAgICBh dXRvIGxlZ2FjeVByZWZpeExlbmd0aCA9IHN0cmxlbihsZWdhY3lQcmVmaXgpOwogCiAgICAgICAg IFZlY3RvcjxXVEY6OlN0cmluZz4gaWRlbnRpZmllcnM7CiAgICAgICAgIGZvciAoYXV0byYgZmls ZU5hbWUgOiBsaXN0RGlyZWN0b3J5KHN0b3JlUGF0aCkpIHsK