Bug 236286 - Reduce allocations and increase thread safety of constructedPath
Summary: Reduce allocations and increase thread safety of constructedPath
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-02-07 21:46 PST by Alex Christensen
Modified: 2022-02-08 09:47 PST (History)
4 users (show)

See Also:


Attachments
Patch (2.54 KB, patch)
2022-02-07 21:47 PST, Alex Christensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2022-02-07 21:46:57 PST
Reduce allocations and increase thread safety of constructedPath
Comment 1 Alex Christensen 2022-02-07 21:47:42 PST
Created attachment 451208 [details]
Patch
Comment 2 Alex Christensen 2022-02-07 21:47:46 PST
<rdar://problem/86904276>
Comment 3 Chris Dumez 2022-02-08 07:27:31 PST
Comment on attachment 451208 [details]
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.
Comment 4 Alex Christensen 2022-02-08 08:59:38 PST
Comment on attachment 451208 [details]
Patch

The optimizer is able to see that they are the same.
Comment 5 EWS 2022-02-08 09:04:53 PST
Committed r289380 (246968@main): <https://commits.webkit.org/246968@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 451208 [details].
Comment 6 Chris Dumez 2022-02-08 09:21:43 PST
Comment on attachment 451208 [details]
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-");`
Comment 7 Chris Dumez 2022-02-08 09:39:12 PST
Comment on attachment 451208 [details]
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.
Comment 8 Darin Adler 2022-02-08 09:39:28 PST
Comment on attachment 451208 [details]
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.
Comment 9 Darin Adler 2022-02-08 09:42:33 PST
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.
Comment 10 Chris Dumez 2022-02-08 09:43:58 PST
Comment on attachment 451208 [details]
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).
Comment 11 Darin Adler 2022-02-08 09:47:06 PST
Comment on attachment 451208 [details]
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".