.
Pull request: https://github.com/WebKit/WebKit/pull/8141
<rdar://problem/104063262>
Created attachment 464477 [details] WIP patch
Created attachment 464478 [details] WIP patch
Committed 258946@main (46f1a9702080): <https://commits.webkit.org/258946@main> Reviewed commits have been landed. Closing PR #8141 and removing active labels.
Reverted by https://github.com/WebKit/WebKit/pull/8698
Committed 258958@main (d087dce13aca): <https://commits.webkit.org/258958@main> Reviewed commits have been landed. Closing PR #8698 and removing active labels.
Reopening for the next iteration.
Pull request: https://github.com/WebKit/WebKit/pull/8712
Created attachment 464666 [details] Patch
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
Created attachment 464770 [details] Patch
Created attachment 464771 [details] Patch
Created attachment 464787 [details] Patch
Are you just splitting the patch, or is there something else that you propose changing?
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?
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.
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.
Created attachment 464806 [details] Patch
Created attachment 464808 [details] Patch
Pull request: https://github.com/WebKit/WebKit/pull/9740
(In reply to Zan Dobersek from comment #15) > Are you just splitting the patch, or is there something else that you > propose changing? ?
(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.
(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
(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.
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.
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.