Bug 227843 – [ITP] Improve ResourceLoadStatisticsDatabaseStore's buildList() and use it in more places

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

Improve ResourceLoadStatisticsDatabaseStore's buildList() and use it in more places.

[Chris Dumez]

Created attachment 433224 [details]
Patch

[Chris Dumez]

Created attachment 433225 [details]
Patch

[Sam Weinig]

Comment on attachment 433225 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=433225&action=review

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:330
> +static String buildList(const ContainerType& values)

One day I will add the following to WTF:

template <typename ContainerType, typename SeparatorType, typename TransformFunctor> String join(const ContainerType& values, const SeparatorType& separator, TransformFunctor&& functor)
{
    StringBuilder builder;

    bool first = true;
    for (auto& value : values) {
        auto tuple = std::invoke(functor, value);
        if (first) {
            builder.append(tuple);
            first = false;
        } else
            builder.append(separator, tuple);
    }
    return builder.toString();
}

so this becomes

template <typename ContainerType> static String buildList(const ContainerType& values)
{
    return join(values, ", ", [](auto& value) {
        if constexpr (std::is_arithmetic_v<std::remove_reference_t<decltype(value)>>)
            return std::make_tuple('"', value, '"');
        else
            return value;
    });
}


I'm unclear it it is better :).

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:338
> +        if constexpr (std::is_arithmetic_v<std::remove_reference_t<decltype(value)>>) {
> +            builder.append(builder.isEmpty() ? "" : ", ", value);
> +        } else {
> +            builder.append(builder.isEmpty() ? "" : ", ", '"', value, '"');
> +        }

No need for the braces here. I think it is likely a bit more efficient to use dedicated bool rather than check isEmpty().

[Chris Dumez]

(In reply to Sam Weinig from comment #3)
> Comment on attachment 433225 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=433225&action=review
> 
> > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:330
> > +static String buildList(const ContainerType& values)
> 
> One day I will add the following to WTF:
> 
> template <typename ContainerType, typename SeparatorType, typename
> TransformFunctor> String join(const ContainerType& values, const
> SeparatorType& separator, TransformFunctor&& functor)
> {
>     StringBuilder builder;
> 
>     bool first = true;
>     for (auto& value : values) {
>         auto tuple = std::invoke(functor, value);
>         if (first) {
>             builder.append(tuple);
>             first = false;
>         } else
>             builder.append(separator, tuple);
>     }
>     return builder.toString();
> }
> 
> so this becomes
> 
> template <typename ContainerType> static String buildList(const
> ContainerType& values)
> {
>     return join(values, ", ", [](auto& value) {
>         if constexpr
> (std::is_arithmetic_v<std::remove_reference_t<decltype(value)>>)
>             return std::make_tuple('"', value, '"');
>         else
>             return value;
>     });
> }
> 
> 
> I'm unclear it it is better :).
> 
> > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:338
> > +        if constexpr (std::is_arithmetic_v<std::remove_reference_t<decltype(value)>>) {
> > +            builder.append(builder.isEmpty() ? "" : ", ", value);
> > +        } else {
> > +            builder.append(builder.isEmpty() ? "" : ", ", '"', value, '"');
> > +        }
> 
> No need for the braces here. I think it is likely a bit more efficient to
> use dedicated bool rather than check isEmpty().

You mean I do not need the curly braces in the else case, right? I thought you'd like the symmetry, since I need them for the `if constexpr` but I'll drop them :)

By a dedicated bool, you mean adding a Boolean variable like "isFirst" before the loop then checking it inside the loop and setting it to false?
I am personally not convinced it is worse the extra code given that StringBuilder::isEmpty() is implemented like so:
```
bool isEmpty() const { return !m_length; }
```

[Chris Dumez]

Created attachment 433250 [details]
Patch

[EWS]

Committed r279802 (239566@main): <https://commits.webkit.org/239566@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 433250 [details].

[Radar WebKit Bug Importer]

<rdar://problem/80406056>

[Sam Weinig]

(In reply to Chris Dumez from comment #4)
> (In reply to Sam Weinig from comment #3)
> > Comment on attachment 433225 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=433225&action=review
> > 
> > > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:330
> > > +static String buildList(const ContainerType& values)
> > 
> > One day I will add the following to WTF:
> > 
> > template <typename ContainerType, typename SeparatorType, typename
> > TransformFunctor> String join(const ContainerType& values, const
> > SeparatorType& separator, TransformFunctor&& functor)
> > {
> >     StringBuilder builder;
> > 
> >     bool first = true;
> >     for (auto& value : values) {
> >         auto tuple = std::invoke(functor, value);
> >         if (first) {
> >             builder.append(tuple);
> >             first = false;
> >         } else
> >             builder.append(separator, tuple);
> >     }
> >     return builder.toString();
> > }
> > 
> > so this becomes
> > 
> > template <typename ContainerType> static String buildList(const
> > ContainerType& values)
> > {
> >     return join(values, ", ", [](auto& value) {
> >         if constexpr
> > (std::is_arithmetic_v<std::remove_reference_t<decltype(value)>>)
> >             return std::make_tuple('"', value, '"');
> >         else
> >             return value;
> >     });
> > }
> > 
> > 
> > I'm unclear it it is better :).
> > 
> > > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:338
> > > +        if constexpr (std::is_arithmetic_v<std::remove_reference_t<decltype(value)>>) {
> > > +            builder.append(builder.isEmpty() ? "" : ", ", value);
> > > +        } else {
> > > +            builder.append(builder.isEmpty() ? "" : ", ", '"', value, '"');
> > > +        }
> > 
> > No need for the braces here. I think it is likely a bit more efficient to
> > use dedicated bool rather than check isEmpty().
> 
> You mean I do not need the curly braces in the else case, right? I thought
> you'd like the symmetry, since I need them for the `if constexpr` but I'll
> drop them :)

I did mean curly braces. I don't think you need curly braces here though. What kind of error do you get without them?

> 
> By a dedicated bool, you mean adding a Boolean variable like "isFirst"
> before the loop then checking it inside the loop and setting it to false?
> I am personally not convinced it is worse the extra code given that
> StringBuilder::isEmpty() is implemented like so:
> ```
> bool isEmpty() const { return !m_length; }
> ```

Yeah, you are probably right. Both are probably on the stack so unlikely to be any different.

[Chris Dumez]

(In reply to Sam Weinig from comment #8)
> (In reply to Chris Dumez from comment #4)
> > (In reply to Sam Weinig from comment #3)
> > > Comment on attachment 433225 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=433225&action=review
> > > 
> > > > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:330
> > > > +static String buildList(const ContainerType& values)
> > > 
> > > One day I will add the following to WTF:
> > > 
> > > template <typename ContainerType, typename SeparatorType, typename
> > > TransformFunctor> String join(const ContainerType& values, const
> > > SeparatorType& separator, TransformFunctor&& functor)
> > > {
> > >     StringBuilder builder;
> > > 
> > >     bool first = true;
> > >     for (auto& value : values) {
> > >         auto tuple = std::invoke(functor, value);
> > >         if (first) {
> > >             builder.append(tuple);
> > >             first = false;
> > >         } else
> > >             builder.append(separator, tuple);
> > >     }
> > >     return builder.toString();
> > > }
> > > 
> > > so this becomes
> > > 
> > > template <typename ContainerType> static String buildList(const
> > > ContainerType& values)
> > > {
> > >     return join(values, ", ", [](auto& value) {
> > >         if constexpr
> > > (std::is_arithmetic_v<std::remove_reference_t<decltype(value)>>)
> > >             return std::make_tuple('"', value, '"');
> > >         else
> > >             return value;
> > >     });
> > > }
> > > 
> > > 
> > > I'm unclear it it is better :).
> > > 
> > > > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:338
> > > > +        if constexpr (std::is_arithmetic_v<std::remove_reference_t<decltype(value)>>) {
> > > > +            builder.append(builder.isEmpty() ? "" : ", ", value);
> > > > +        } else {
> > > > +            builder.append(builder.isEmpty() ? "" : ", ", '"', value, '"');
> > > > +        }
> > > 
> > > No need for the braces here. I think it is likely a bit more efficient to
> > > use dedicated bool rather than check isEmpty().
> > 
> > You mean I do not need the curly braces in the else case, right? I thought
> > you'd like the symmetry, since I need them for the `if constexpr` but I'll
> > drop them :)
> 
> I did mean curly braces. I don't think you need curly braces here though.
> What kind of error do you get without them?

Oh! you're totally right, it builds without the curly brackets. I don't know why I thought `if constexpr` required curly brackets.

Anyway, I fixed it in <https://commits.webkit.org/239569@main>, thanks!