Bug 250017 – Improve StringTypeAdapter templates and handling

• Component: New Bugs
• Status: REOPENED

[Zan Dobersek]

.

[Zan Dobersek]

Pull request: https://github.com/WebKit/WebKit/pull/8141

[Radar WebKit Bug Importer]

<rdar://problem/104063262>

[Fujii Hironori]

Created attachment 464477 [details]
WIP patch

[Fujii Hironori]

Created attachment 464478 [details]
WIP patch

[EWS]

Committed 258946@main (46f1a9702080): <https://commits.webkit.org/258946@main>

Reviewed commits have been landed. Closing PR #8141 and removing active labels.

[Zan Dobersek]

Reverted by https://github.com/WebKit/WebKit/pull/8698

[EWS]

Committed 258958@main (d087dce13aca): <https://commits.webkit.org/258958@main>

Reviewed commits have been landed. Closing PR #8698 and removing active labels.

[Zan Dobersek]

Reopening for the next iteration.

[Zan Dobersek]

Pull request: https://github.com/WebKit/WebKit/pull/8712

[Fujii Hironori]

Created attachment 464666 [details]
Patch

[Fujii Hironori]

I came up with a good idea. I'm going to split Zan's patch into three.
* Making StringTypeAdapter ctor take const reference
* Using parameter packs and fold expressions
* Using more StringView internally to simplify

[Fujii Hironori]

Created attachment 464770 [details]
Patch

[Fujii Hironori]

Created attachment 464771 [details]
Patch

[Fujii Hironori]

Created attachment 464787 [details]
Patch

[Zan Dobersek]

Are you just splitting the patch, or is there something else that you propose changing?

[Darin Adler]

Comment on attachment 464787 [details]
Patch

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

> COMMIT_MESSAGE:7
> +makeString family unnecessary copied objects passed as arguments. They
> +should take references and pass them to StringTypeAdapter.

Why? Will this be more efficient? Can we make a test showing the benefit?

[Darin Adler]

Comment on attachment 464787 [details]
Patch

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

>> COMMIT_MESSAGE:7
>> +should take references and pass them to StringTypeAdapter.
> 
> Why? Will this be more efficient? Can we make a test showing the benefit?

One reason I am asking this is that many types are more efficient to pass by value than by reference, unless things are inlined. For example, a StringView is almost certainly more efficient to pass by value, same for scalars and various types of pointers. An exception might be reference counted objects like String where passing by reference might avoid reference count churn.

I am concerned that this change could make performance worse rather than making it better. I would like some evidence one way or the other.

[Fujii Hironori]

Comment on attachment 464787 [details]
Patch

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

>>> COMMIT_MESSAGE:7
>>> +should take references and pass them to StringTypeAdapter.
>> 
>> Why? Will this be more efficient? Can we make a test showing the benefit?
> 
> One reason I am asking this is that many types are more efficient to pass by value than by reference, unless things are inlined. For example, a StringView is almost certainly more efficient to pass by value, same for scalars and various types of pointers. An exception might be reference counted objects like String where passing by reference might avoid reference count churn.
> 
> I am concerned that this change could make performance worse rather than making it better. I would like some evidence one way or the other.

We will be able to use makeString for non-copyable types.
I will revise the patch to use pass-by-value for simple values and add a testcase of non-copyable.

[Fujii Hironori]

Created attachment 464806 [details]
Patch

[Fujii Hironori]

Created attachment 464808 [details]
Patch

[Fujii Hironori]

Pull request: https://github.com/WebKit/WebKit/pull/9740

[Zan Dobersek]

(In reply to Zan Dobersek from comment #15)
> Are you just splitting the patch, or is there something else that you
> propose changing?

?

[Zan Dobersek]

(In reply to Fujii Hironori from comment #18)
> Comment on attachment 464787 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=464787&action=review
> 
> >>> COMMIT_MESSAGE:7
> >>> +should take references and pass them to StringTypeAdapter.
> >> 
> >> Why? Will this be more efficient? Can we make a test showing the benefit?
> > 
> > One reason I am asking this is that many types are more efficient to pass by value than by reference, unless things are inlined. For example, a StringView is almost certainly more efficient to pass by value, same for scalars and various types of pointers. An exception might be reference counted objects like String where passing by reference might avoid reference count churn.
> > 
> > I am concerned that this change could make performance worse rather than making it better. I would like some evidence one way or the other.
> 
> We will be able to use makeString for non-copyable types.

Enabling use of non-copyable types isn't an end-goal here, and it's not a problem today. Idea is to avoid unnecessary copies of objects that are somewhat expensive to copy, e.g. Strings, Vectors, tuples.

I don't see passing by reference in general as having an impact on performance. Passing by reference specifically for some trivial types can be hindering under certain conditions (i.e. lack of inlining), but I don't think this will outweigh the benefits. I'll argue all this separately, later.

> I will revise the patch to use pass-by-value for simple values and add a
> testcase of non-copyable.

You ended up using forwarding references, which is still passing by reference and not by value.

[Fujii Hironori]

(In reply to Zan Dobersek from comment #15)
> Are you just splitting the patch, or is there something else that you
> propose changing?

Fixes the problem. https://github.com/WebKit/WebKit/pull/8712#discussion_r1090303757

[Zan Dobersek]

(In reply to Fujii Hironori from comment #24)
> (In reply to Zan Dobersek from comment #15)
> > Are you just splitting the patch, or is there something else that you
> > propose changing?
> 
> Fixes the problem.
> https://github.com/WebKit/WebKit/pull/8712#discussion_r1090303757

Forwarding references do fix that, but that problem is trivial and avoidable if a Span-taking named constructor for wchar_t is adopted, as being discussed for other types in https://github.com/WebKit/WebKit/pull/9339.

I can handle these changes on my own, along with other ideas beyond these, but I'll probably be splitting them up considerably, evaluating and quantifying the changes (especially the more delicate ones). Just please review them when you have a chance.

[Fujii Hironori]

I like your three ideas in comment 11, but your patch looks unaccepable to me. I did review, but you dont fix the problem. That's the reason i fixes the problem. If you think your patch is acceptable, lets start a discussion in webkit-dev.

[Zan Dobersek]

I don't think my patch is acceptable. I split off the relevant problems into separate bugs or pull requests, of which bug #251385 (PR 9339) still remains, and is mostly stuck on agreeing the range and names of Span-based named constructors.

Your patch takes a part of my work, without any attribution, and extends it to use rvalue references, which is one way of avoiding the problem of unintentionally constructing Strings and StringViews from native string literals and fixed-length C arrays, but it also doesn't address the pass-by-value concerns.

If you want to work with me on this, then please work with me, but let's coordinate around problems, ideas and possible solutions on it, and avoid stepping on each other's toes.