REOPENED 250017
Improve StringTypeAdapter templates and handling
https://bugs.webkit.org/show_bug.cgi?id=250017
Summary Improve StringTypeAdapter templates and handling
Zan Dobersek
Reported 2023-01-02 23:31:26 PST
.
Attachments
WIP patch (14.74 KB, patch)
2023-01-12 21:38 PST, Fujii Hironori
no flags
WIP patch (18.03 KB, patch)
2023-01-12 21:39 PST, Fujii Hironori
ews-feeder: commit-queue-
Patch (23.51 KB, patch)
2023-01-26 01:25 PST, Fujii Hironori
no flags
Patch (7.17 KB, patch)
2023-01-30 16:23 PST, Fujii Hironori
ews-feeder: commit-queue-
Patch (12.18 KB, patch)
2023-01-30 18:07 PST, Fujii Hironori
no flags
Patch (14.75 KB, patch)
2023-01-31 19:53 PST, Fujii Hironori
no flags
Patch (14.20 KB, patch)
2023-02-01 16:12 PST, Fujii Hironori
ews-feeder: commit-queue-
Patch (14.11 KB, patch)
2023-02-01 18:21 PST, Fujii Hironori
no flags
Zan Dobersek
Comment 1 2023-01-02 23:32:26 PST
Radar WebKit Bug Importer
Comment 2 2023-01-09 23:32:21 PST
Fujii Hironori
Comment 3 2023-01-12 21:38:08 PST
Created attachment 464477 [details] WIP patch
Fujii Hironori
Comment 4 2023-01-12 21:39:39 PST
Created attachment 464478 [details] WIP patch
EWS
Comment 5 2023-01-16 03:31:03 PST
Committed 258946@main (46f1a9702080): <https://commits.webkit.org/258946@main> Reviewed commits have been landed. Closing PR #8141 and removing active labels.
Zan Dobersek
Comment 6 2023-01-16 09:18:44 PST
EWS
Comment 7 2023-01-16 09:48:03 PST
Committed 258958@main (d087dce13aca): <https://commits.webkit.org/258958@main> Reviewed commits have been landed. Closing PR #8698 and removing active labels.
Zan Dobersek
Comment 8 2023-01-16 22:21:44 PST
Reopening for the next iteration.
Zan Dobersek
Comment 9 2023-01-16 22:59:57 PST
Fujii Hironori
Comment 10 2023-01-26 01:25:29 PST
Fujii Hironori
Comment 11 2023-01-30 16:13:38 PST
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
Comment 12 2023-01-30 16:23:49 PST
Fujii Hironori
Comment 13 2023-01-30 18:07:22 PST
Fujii Hironori
Comment 14 2023-01-31 19:53:42 PST
Zan Dobersek
Comment 15 2023-02-01 00:47:50 PST
Are you just splitting the patch, or is there something else that you propose changing?
Darin Adler
Comment 16 2023-02-01 08:19:28 PST
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 17 2023-02-01 09:31:48 PST
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 18 2023-02-01 13:52:48 PST
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
Comment 19 2023-02-01 16:12:13 PST
Fujii Hironori
Comment 20 2023-02-01 18:21:17 PST
Fujii Hironori
Comment 21 2023-02-06 23:39:27 PST
Zan Dobersek
Comment 22 2023-02-07 01:12:28 PST
(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
Comment 23 2023-02-07 01:58:18 PST
(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
Comment 24 2023-02-07 19:29:56 PST
(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
Comment 25 2023-02-08 23:59:22 PST
(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
Comment 26 2023-02-09 00:57:38 PST
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
Comment 27 2023-02-09 02:55:44 PST
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.
Note You need to log in before you can comment on or make changes to this bug.