Bug 239370 - Replace complex String::insert() with a simplified makeStringByInserting() free function
Summary: Replace complex String::insert() with a simplified makeStringByInserting() fr...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-04-14 20:06 PDT by Chris Dumez
Modified: 2022-04-16 13:33 PDT (History)
17 users (show)

See Also:


Attachments
Patch (9.79 KB, patch)
2022-04-14 20:10 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (9.79 KB, patch)
2022-04-14 21:54 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (12.43 KB, patch)
2022-04-15 15:52 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (12.63 KB, patch)
2022-04-15 21:08 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2022-04-14 20:06:57 PDT
Simplify String::insert() implementation and reduce usage.
Comment 1 Chris Dumez 2022-04-14 20:10:40 PDT
Created attachment 457668 [details]
Patch
Comment 2 Chris Dumez 2022-04-14 21:54:41 PDT
Created attachment 457673 [details]
Patch
Comment 3 Darin Adler 2022-04-15 14:06:50 PDT
Comment on attachment 457673 [details]
Patch

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

> Source/WTF/ChangeLog:8
> +        Simplify String::insert() implementation and reduce usage.

What uses remain?

> Source/WebCore/dom/CharacterData.cpp:170
> +    StringView oldDataView { m_data };
> +    auto newData = makeString(oldDataView.left(offset), data, oldDataView.substring(offset + count));

Should we add a special case for when count is 0 and data is empty?
Comment 4 Chris Dumez 2022-04-15 14:56:18 PDT
(In reply to Darin Adler from comment #3)
> Comment on attachment 457673 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=457673&action=review
> 
> > Source/WTF/ChangeLog:8
> > +        Simplify String::insert() implementation and reduce usage.
> 
> What uses remain?

Not many:
Source/WebCore/Modules/mediasource/MediaSource.cpp:        rawType.insert1(".00"_s, position + codec.length());
Source/WebCore/dom/CharacterData.cpp:    newData.insert1(data, offset);
Source/WebCore/html/HTMLTextFormControlElement.cpp:        text.insert1(replacement, start);

> > Source/WebCore/dom/CharacterData.cpp:170
> > +    StringView oldDataView { m_data };
> > +    auto newData = makeString(oldDataView.left(offset), data, oldDataView.substring(offset + count));
> 
> Should we add a special case for when count is 0 and data is empty?

Need to think more about this.
Comment 5 Darin Adler 2022-04-15 14:59:56 PDT
Comment on attachment 457673 [details]
Patch

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

>>> Source/WTF/ChangeLog:8
>>> +        Simplify String::insert() implementation and reduce usage.
>> 
>> What uses remain?
> 
> Not many:
> Source/WebCore/Modules/mediasource/MediaSource.cpp:        rawType.insert1(".00"_s, position + codec.length());
> Source/WebCore/dom/CharacterData.cpp:    newData.insert1(data, offset);
> Source/WebCore/html/HTMLTextFormControlElement.cpp:        text.insert1(replacement, start);

I am surprised you didn’t go all the way and do those 3 too.
Comment 6 Chris Dumez 2022-04-15 15:01:33 PDT
(In reply to Darin Adler from comment #5)
> Comment on attachment 457673 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=457673&action=review
> 
> >>> Source/WTF/ChangeLog:8
> >>> +        Simplify String::insert() implementation and reduce usage.
> >> 
> >> What uses remain?
> > 
> > Not many:
> > Source/WebCore/Modules/mediasource/MediaSource.cpp:        rawType.insert1(".00"_s, position + codec.length());
> > Source/WebCore/dom/CharacterData.cpp:    newData.insert1(data, offset);
> > Source/WebCore/html/HTMLTextFormControlElement.cpp:        text.insert1(replacement, start);
> 
> I am surprised you didn’t go all the way and do those 3 too.

I did that at first. Then it didn't seem worth it to me because it made the call sites look a little more complicated and not more efficient. For this reason, I felt it was good to hide the complexity inside String::insert() and use that for those 3 call sites. Do you feel differently?
Comment 7 Chris Dumez 2022-04-15 15:03:58 PDT
(In reply to Chris Dumez from comment #6)
> (In reply to Darin Adler from comment #5)
> > Comment on attachment 457673 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=457673&action=review
> > 
> > >>> Source/WTF/ChangeLog:8
> > >>> +        Simplify String::insert() implementation and reduce usage.
> > >> 
> > >> What uses remain?
> > > 
> > > Not many:
> > > Source/WebCore/Modules/mediasource/MediaSource.cpp:        rawType.insert1(".00"_s, position + codec.length());
> > > Source/WebCore/dom/CharacterData.cpp:    newData.insert1(data, offset);
> > > Source/WebCore/html/HTMLTextFormControlElement.cpp:        text.insert1(replacement, start);
> > 
> > I am surprised you didn’t go all the way and do those 3 too.
> 
> I did that at first. Then it didn't seem worth it to me because it made the
> call sites look a little more complicated and not more efficient. For this
> reason, I felt it was good to hide the complexity inside String::insert()
> and use that for those 3 call sites. Do you feel differently?

Especially, with my simpler / lightweight String::insert() implementation, the cost to keeping it is not high (compared to previously, as it was a LOT of code).
Comment 8 Darin Adler 2022-04-15 15:06:31 PDT
Comment on attachment 457673 [details]
Patch

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

>>>>> Source/WTF/ChangeLog:8
>>>>> +        Simplify String::insert() implementation and reduce usage.
>>>> 
>>>> What uses remain?
>>> 
>>> Not many:
>>> Source/WebCore/Modules/mediasource/MediaSource.cpp:        rawType.insert1(".00"_s, position + codec.length());
>>> Source/WebCore/dom/CharacterData.cpp:    newData.insert1(data, offset);
>>> Source/WebCore/html/HTMLTextFormControlElement.cpp:        text.insert1(replacement, start);
>> 
>> I am surprised you didn’t go all the way and do those 3 too.
> 
> I did that at first. Then it didn't seem worth it to me because it made the call sites look a little more complicated and not more efficient. For this reason, I felt it was good to hide the complexity inside String::insert() and use that for those 3 call sites. Do you feel differently?

I do.

I think we should stop making String feel "mutable" and get rid of functions that modify a String in place by making a new StringImpl, since they seem more efficient than they are and it’s confusing to have them alongside other functions that return values and don’t modify the String. Instead we should have functions that make new strings. So String::insert is one of the ones I want to get rid of. Even if we just changed it to be a non-member function that returns a String rather than changing a String in place (basically the exact same code) I think it would help us make String better.

I know this is a radical thing to say, but some day we might even rename StringImpl to String and use RefPtr<String> where we use String today.
Comment 9 Chris Dumez 2022-04-15 15:18:51 PDT
(In reply to Darin Adler from comment #8)
> Comment on attachment 457673 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=457673&action=review
> 
> >>>>> Source/WTF/ChangeLog:8
> >>>>> +        Simplify String::insert() implementation and reduce usage.
> >>>> 
> >>>> What uses remain?
> >>> 
> >>> Not many:
> >>> Source/WebCore/Modules/mediasource/MediaSource.cpp:        rawType.insert1(".00"_s, position + codec.length());
> >>> Source/WebCore/dom/CharacterData.cpp:    newData.insert1(data, offset);
> >>> Source/WebCore/html/HTMLTextFormControlElement.cpp:        text.insert1(replacement, start);
> >> 
> >> I am surprised you didn’t go all the way and do those 3 too.
> > 
> > I did that at first. Then it didn't seem worth it to me because it made the call sites look a little more complicated and not more efficient. For this reason, I felt it was good to hide the complexity inside String::insert() and use that for those 3 call sites. Do you feel differently?
> 
> I do.
> 
> I think we should stop making String feel "mutable" and get rid of functions
> that modify a String in place by making a new StringImpl, since they seem
> more efficient than they are and it’s confusing to have them alongside other
> functions that return values and don’t modify the String. Instead we should
> have functions that make new strings. So String::insert is one of the ones I
> want to get rid of. Even if we just changed it to be a non-member function
> that returns a String rather than changing a String in place (basically the
> exact same code) I think it would help us make String better.
> 
> I know this is a radical thing to say, but some day we might even rename
> StringImpl to String and use RefPtr<String> where we use String today.

How would you feel if that insert() function was const and returned a String with WARN_UNUSED_RETURN ?
We have quite a few functions like this on String and I find that less confusing personally.
Comment 10 Darin Adler 2022-04-15 15:26:32 PDT
Comment on attachment 457673 [details]
Patch

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

>>>>>>>> Source/WTF/ChangeLog:8
>>>>>>>> +        Simplify String::insert() implementation and reduce usage.
>>>>>>> 
>>>>>>> What uses remain?
>>>>>> 
>>>>>> Not many:
>>>>>> Source/WebCore/Modules/mediasource/MediaSource.cpp:        rawType.insert1(".00"_s, position + codec.length());
>>>>>> Source/WebCore/dom/CharacterData.cpp:    newData.insert1(data, offset);
>>>>>> Source/WebCore/html/HTMLTextFormControlElement.cpp:        text.insert1(replacement, start);
>>>>> 
>>>>> I am surprised you didn’t go all the way and do those 3 too.
>>>> 
>>>> I did that at first. Then it didn't seem worth it to me because it made the call sites look a little more complicated and not more efficient. For this reason, I felt it was good to hide the complexity inside String::insert() and use that for those 3 call sites. Do you feel differently?
>>> 
>>> Especially, with my simpler / lightweight String::insert() implementation, the cost to keeping it is not high (compared to previously, as it was a LOT of code).
>> 
>> I do.
>> 
>> I think we should stop making String feel "mutable" and get rid of functions that modify a String in place by making a new StringImpl, since they seem more efficient than they are and it’s confusing to have them alongside other functions that return values and don’t modify the String. Instead we should have functions that make new strings. So String::insert is one of the ones I want to get rid of. Even if we just changed it to be a non-member function that returns a String rather than changing a String in place (basically the exact same code) I think it would help us make String better.
>> 
>> I know this is a radical thing to say, but some day we might even rename StringImpl to String and use RefPtr<String> where we use String today.
> 
> How would you feel if that insert() function was const and returned a String with WARN_UNUSED_RETURN ?
> We have quite a few functions like this on String and I find that less confusing personally.

Maybe. Not sure it has to be a member function at all.
Comment 11 Chris Dumez 2022-04-15 15:27:12 PDT
(In reply to Chris Dumez from comment #4)
> (In reply to Darin Adler from comment #3)
> > Comment on attachment 457673 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=457673&action=review
> > 
> > > Source/WTF/ChangeLog:8
> > > +        Simplify String::insert() implementation and reduce usage.
> > 
> > What uses remain?
> 
> Not many:
> Source/WebCore/Modules/mediasource/MediaSource.cpp:       
> rawType.insert1(".00"_s, position + codec.length());
> Source/WebCore/dom/CharacterData.cpp:    newData.insert1(data, offset);
> Source/WebCore/html/HTMLTextFormControlElement.cpp:       
> text.insert1(replacement, start);
> 
> > > Source/WebCore/dom/CharacterData.cpp:170
> > > +    StringView oldDataView { m_data };
> > > +    auto newData = makeString(oldDataView.left(offset), data, oldDataView.substring(offset + count));
> > 
> > Should we add a special case for when count is 0 and data is empty?
> 
> Need to think more about this.

I added logging for this case and it didn't happen on Speedometer or on the several top sites I browsed and interacted with for the past 5 minutes. I am not convinced such optimization is worth it.
Comment 12 Darin Adler 2022-04-15 15:36:29 PDT
Comment on attachment 457673 [details]
Patch

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

>>>>>>>>>> Source/WTF/ChangeLog:8
>>>>>>>>>> +        Simplify String::insert() implementation and reduce usage.
>>>>>>>>> 
>>>>>>>>> What uses remain?
>>>>>>>> 
>>>>>>>> Not many:
>>>>>>>> Source/WebCore/Modules/mediasource/MediaSource.cpp:        rawType.insert1(".00"_s, position + codec.length());
>>>>>>>> Source/WebCore/dom/CharacterData.cpp:    newData.insert1(data, offset);
>>>>>>>> Source/WebCore/html/HTMLTextFormControlElement.cpp:        text.insert1(replacement, start);
>>>>>>> 
>>>>>>> I am surprised you didn’t go all the way and do those 3 too.
>>>>>> 
>>>>>> I did that at first. Then it didn't seem worth it to me because it made the call sites look a little more complicated and not more efficient. For this reason, I felt it was good to hide the complexity inside String::insert() and use that for those 3 call sites. Do you feel differently?
>>>>> 
>>>>> Especially, with my simpler / lightweight String::insert() implementation, the cost to keeping it is not high (compared to previously, as it was a LOT of code).
>>>> 
>>>> I do.
>>>> 
>>>> I think we should stop making String feel "mutable" and get rid of functions that modify a String in place by making a new StringImpl, since they seem more efficient than they are and it’s confusing to have them alongside other functions that return values and don’t modify the String. Instead we should have functions that make new strings. So String::insert is one of the ones I want to get rid of. Even if we just changed it to be a non-member function that returns a String rather than changing a String in place (basically the exact same code) I think it would help us make String better.
>>>> 
>>>> I know this is a radical thing to say, but some day we might even rename StringImpl to String and use RefPtr<String> where we use String today.
>>> 
>>> How would you feel if that insert() function was const and returned a String with WARN_UNUSED_RETURN ?
>>> We have quite a few functions like this on String and I find that less confusing personally.
>> 
>> Maybe. Not sure it has to be a member function at all.
> 
> I added logging for this case and it didn't happen on Speedometer or on the several top sites I browsed and interacted with for the past 5 minutes. I am not convinced such optimization is worth it.

Glad we don’t need it.
Comment 13 Chris Dumez 2022-04-15 15:52:00 PDT
Created attachment 457728 [details]
Patch
Comment 14 Darin Adler 2022-04-15 15:59:52 PDT
Comment on attachment 457728 [details]
Patch

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

> Source/WTF/wtf/text/WTFString.h:606
> +WTF_EXPORT_PRIVATE String makeStringByInserting(const String& originalString, StringView stringToInsert, unsigned position);

I like it. A neat thing about this is that we could also make an overload that takes a StringView for originalString if we find that’s needed.
Comment 15 Chris Dumez 2022-04-15 16:07:02 PDT
Comment on attachment 457728 [details]
Patch

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

>> Source/WTF/wtf/text/WTFString.h:606
>> +WTF_EXPORT_PRIVATE String makeStringByInserting(const String& originalString, StringView stringToInsert, unsigned position);
> 
> I like it. A neat thing about this is that we could also make an overload that takes a StringView for originalString if we find that’s needed.

Good, I like it better as well.
Comment 16 Darin Adler 2022-04-15 20:13:55 PDT
Comment on attachment 457728 [details]
Patch

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

> Source/WTF/wtf/text/WTFString.cpp:84
> +    StringView originalStringView { originalString };
> +    return makeString(originalStringView.left(position), stringToInsert, originalStringView.substring(position));

Wait, there’s nothing about this function taking advantage of the fact that originalString is a String, so we should change it to take a StringView!

> Source/WebCore/dom/CharacterData.cpp:141
> +    String newData = makeStringByInserting(m_data, data, offset);

auto, like below
Comment 17 Chris Dumez 2022-04-15 21:08:00 PDT
Created attachment 457741 [details]
Patch
Comment 18 EWS 2022-04-16 13:32:44 PDT
Committed r292942 (249707@main): <https://commits.webkit.org/249707@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 457741 [details].
Comment 19 Radar WebKit Bug Importer 2022-04-16 13:33:15 PDT
<rdar://problem/91853248>