Bug 236286 – Reduce allocations and increase thread safety of constructedPath

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

Reduce allocations and increase thread safety of constructedPath

[Alex Christensen]

Created attachment 451208 [details]
Patch

[Alex Christensen]

<rdar://problem/86904276>

[Chris Dumez]

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.

[Alex Christensen]

Comment on attachment 451208 [details]
Patch

The optimizer is able to see that they are the same.

[EWS]

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].

[Chris Dumez]

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-");`

[Chris Dumez]

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.

[Darin Adler]

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.

[Darin Adler]

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.

[Chris Dumez]

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).

[Darin Adler]

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".