Bug 239370 – Replace complex String::insert() with a simplified makeStringByInserting() free function

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Chris Dumez]

Simplify String::insert() implementation and reduce usage.

[Chris Dumez]

Created attachment 457668 [details]
Patch

[Chris Dumez]

Created attachment 457673 [details]
Patch

[Darin Adler]

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?

[Chris Dumez]

(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.

[Darin Adler]

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.

[Chris Dumez]

(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?

[Chris Dumez]

(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).

[Darin Adler]

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.

[Chris Dumez]

(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.

[Darin Adler]

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.

[Chris Dumez]

(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.

[Darin Adler]

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.

[Chris Dumez]

Created attachment 457728 [details]
Patch

[Darin Adler]

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.

[Chris Dumez]

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.

[Darin Adler]

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

[Chris Dumez]

Created attachment 457741 [details]
Patch

[EWS]

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].

[Radar WebKit Bug Importer]

<rdar://problem/91853248>