Bug 238235 – Use ASCIILiteral in a few more places where it is useful

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Chris Dumez]

Use ASCIILiteral in a few more places where it is useful.

[Chris Dumez]

Created attachment 455448 [details]
Patch

[Geoffrey Garen]

Comment on attachment 455448 [details]
Patch

r=me

[EWS]

Committed r291731 (248763@main): <https://commits.webkit.org/248763@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 455448 [details].

[Radar WebKit Bug Importer]

<rdar://problem/90671659>

[Darin Adler]

Comment on attachment 455448 [details]
Patch

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

> Source/JavaScriptCore/runtime/FunctionExecutable.cpp:162
> +        functionHeader = "function "_s;

I don’t understand why we want a String for functionHeader instead of a const char*. Optimizing how efficiently we initialize a String that should not be created in the first place seems like a mistake. Not sure exactly how jsMakeNontrivialString works, but it seems a lot like makeString to me.

> Source/WebCore/html/HTMLTextAreaElement.cpp:398
> +    normalizedValue.replace("\r\n"_s, "\n"_s);

Makes no logical sense to me that _s is useful here. Replacing one substring with another should be efficient when both are const char* since neither is being used as a String after the operation is over, and adding that overload would be better than adding _s. And using _s means that if we do overload for const char* that will not be sufficient. We’ll have to overload for ASCIILiteral as well.

This points to the trouble we can run into with so many different ways to represent string literals. This code isn’t performance critical, but it really offends me that it creates and destroys two StringImpl, and optimizing how quickly it does that seems like we are sort of missing the plot.

[Chris Dumez]

(In reply to Darin Adler from comment #5)
> Comment on attachment 455448 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=455448&action=review
> 
> > Source/JavaScriptCore/runtime/FunctionExecutable.cpp:162
> > +        functionHeader = "function "_s;
> 
> I don’t understand why we want a String for functionHeader instead of a
> const char*. Optimizing how efficiently we initialize a String that should
> not be created in the first place seems like a mistake. Not sure exactly how
> jsMakeNontrivialString works, but it seems a lot like makeString to me.
> 
> > Source/WebCore/html/HTMLTextAreaElement.cpp:398
> > +    normalizedValue.replace("\r\n"_s, "\n"_s);
> 
> Makes no logical sense to me that _s is useful here. Replacing one substring
> with another should be efficient when both are const char* since neither is
> being used as a String after the operation is over, and adding that overload
> would be better than adding _s. And using _s means that if we do overload
> for const char* that will not be sufficient. We’ll have to overload for
> ASCIILiteral as well.
> 
> This points to the trouble we can run into with so many different ways to
> represent string literals. This code isn’t performance critical, but it
> really offends me that it creates and destroys two StringImpl, and
> optimizing how quickly it does that seems like we are sort of missing the
> plot.

To be clear, _s is definitely useful for performance here, with the current code base. What you're pointing out is that we should probably refactor the code so that we don't need a String at all.
That sounds fair and I can look into this in a follow-up.

Also, if we added a String::replace(const char*, const char*) and passed in a _s, I suspect the compiler would complain about it being ambiguous since an ASCIILiteral can be implicitly converted to either a const char* or a String.

[Darin Adler]

(In reply to Chris Dumez from comment #6)
> To be clear, _s is definitely useful for performance here, with the current
> code base. What you're pointing out is that we should probably refactor the
> code so that we don't need a String at all.

Yes, and in the first, non-replace, case, it’s a super-local, super-trivial refactor, almost as simple as adding the _s.

> Also, if we added a String::replace(const char*, const char*) and passed in
> a _s, I suspect the compiler would complain about it being ambiguous since
> an ASCIILiteral can be implicitly converted to either a const char* or a
> String.

On reflection what we probably want is String::replace(StringView, StringView). That might have good enough performance that we could get rid of most or all of the other overloads.

[Dawn Morningstar]

(In reply to EWS from comment #3)
> Committed r291731 (248763@main): <https://commits.webkit.org/248763@main>
> 
> All reviewed patches have been landed. Closing bug and clearing flags on
> attachment 455448 [details].

Could this patch be causing a whole suite of constantly crashing tests?
https://results.webkit.org/?platform=ios&suite=layout-tests&suite=layout-tests&suite=layout-tests&suite=layout-tests&suite=layout-tests&suite=layout-tests&suite=layout-tests&test=editing%2Fdeleting%2F5126166.html&test=editing%2Fdeleting%2F5206311-1.html&test=editing%2Fdeleting%2F5206311-2.html&test=editing%2Fdeleting%2F5433862-1.html&test=editing%2Fdeleting%2Fbackspace-at-table-cell-beginning.html&test=editing%2Fdeleting%2Fdelete-line-016.html&test=editing%2Fdeleting%2Fforward-delete-empty-table-cell.html

[Chris Dumez]

(In reply to Matteo Flores from comment #8)
> (In reply to EWS from comment #3)
> > Committed r291731 (248763@main): <https://commits.webkit.org/248763@main>
> > 
> > All reviewed patches have been landed. Closing bug and clearing flags on
> > attachment 455448 [details].
> 
> Could this patch be causing a whole suite of constantly crashing tests?
> https://results.webkit.org/?platform=ios&suite=layout-tests&suite=layout-
> tests&suite=layout-tests&suite=layout-tests&suite=layout-tests&suite=layout-
> tests&suite=layout-tests&test=editing%2Fdeleting%2F5126166.
> html&test=editing%2Fdeleting%2F5206311-1.
> html&test=editing%2Fdeleting%2F5206311-2.
> html&test=editing%2Fdeleting%2F5433862-1.
> html&test=editing%2Fdeleting%2Fbackspace-at-table-cell-beginning.
> html&test=editing%2Fdeleting%2Fdelete-line-016.
> html&test=editing%2Fdeleting%2Fforward-delete-empty-table-cell.html

Hmm, crashes seem to look like:
ASSERTION FAILED: areVisiblePositionsInSameTreeScope(result, vp) => useDownstream ? (result > vp) : (result < vp)
./editing/VisibleUnits.cpp(1707) : WebCore::VisiblePosition WebCore::nextSentenceBoundaryInDirection(const WebCore::VisiblePosition &, WebCore::SelectionDirection)
1   0x1381f1d89 WTFCrash
2   0x163f1d5da WebCore::nextSentenceBoundaryInDirection(WebCore::VisiblePosition const&, WebCore::SelectionDirection)
3   0x163f1cb4a WebCore::positionOfNextBoundaryOfGranularity(WebCore::VisiblePosition const&, WebCore::TextGranularity, WebCore::SelectionDirection)
4   0x124cf43b6 WebKit::WebPage::autocorrectionContext()
5   0x124cf4c78 WebKit::WebPage::preemptivelySendAutocorrectionContext()
6   0x125a264eb WebKit::WebPage::didChangeSelection(WebCore::Frame&)
7   0x12571fb35 WebKit::WebEditorClient::respondToChangedSelection(WebCore::Frame*)
8   0x163e5d360 WebCore::Editor::respondToChangedSelection(WebCore::VisibleSelection const&, WTF::OptionSet<WebCore::FrameSelection::SetSelectionOption>)
9   0x163e68314 WebCore::FrameSelection::setSelectionWithoutUpdatingAppearance(WebCore::VisibleSelection const&, WTF::OptionSet<WebCore::FrameSelection::SetSelectionOption>, WebCore::FrameSelection::CursorAlignOnScroll, WebCore::TextGranularity)
10  0x163e4b190 WebCore::FrameSelection::setSelection(WebCore::VisibleSelection const&, WTF::OptionSet<WebCore::FrameSelection::SetSelectionOption>, WebCore::AXTextStateChangeIntent, WebCore::FrameSelection::CursorAlignOnScroll, WebCore::TextGranularity)
11  0x163e5bef6 WebCore::FrameSelection::moveTo(WebCore::Position const&, WebCore::Affinity, WebCore::EUserTriggered)
12  0x16498d294 WebCore::DOMSelection::collapse(WebCore::Node*, unsigned int)
13  0x160f9b8bd WebCore::jsDOMSelectionPrototypeFunction_collapseBody(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSDOMSelection*)::'lambda'()::operator()() const
14  0x160f9b5e1 JSC::JSValue WebCore::toJS<WebCore::IDLUndefined, WebCore::jsDOMSelectionPrototypeFunction_collapseBody(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSDOMSelection*)::'lambda'()>(JSC::JSGlobalObject&, JSC::ThrowScope&, WebCore::jsDOMSelectionPrototypeFunction_collapseBody(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSDOMSelection*)::'lambda'()&&)
15  0x160f9b51b WebCore::jsDOMSelectionPrototypeFunction_collapseBody(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSDOMSelection*)
16  0x160f9b0b5 long long WebCore::IDLOperation<WebCore::JSDOMSelection>::call<&(WebCore::jsDOMSelectionPrototypeFunction_collapseBody(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSDOMSelection*)), (WebCore::CastedThisErrorBehavior)0>(JSC::JSGlobalObject&, JSC::CallFrame&, char const*)
17  0x160f983a4 WebCore::jsDOMSelectionPrototypeFunction_collapse(JSC::JSGlobalObject*, JSC::CallFrame*)

It does not look related to me.

[Chris Dumez]

(In reply to Chris Dumez from comment #9)
> (In reply to Matteo Flores from comment #8)
> > (In reply to EWS from comment #3)
> > > Committed r291731 (248763@main): <https://commits.webkit.org/248763@main>
> > > 
> > > All reviewed patches have been landed. Closing bug and clearing flags on
> > > attachment 455448 [details].
> > 
> > Could this patch be causing a whole suite of constantly crashing tests?
> > https://results.webkit.org/?platform=ios&suite=layout-tests&suite=layout-
> > tests&suite=layout-tests&suite=layout-tests&suite=layout-tests&suite=layout-
> > tests&suite=layout-tests&test=editing%2Fdeleting%2F5126166.
> > html&test=editing%2Fdeleting%2F5206311-1.
> > html&test=editing%2Fdeleting%2F5206311-2.
> > html&test=editing%2Fdeleting%2F5433862-1.
> > html&test=editing%2Fdeleting%2Fbackspace-at-table-cell-beginning.
> > html&test=editing%2Fdeleting%2Fdelete-line-016.
> > html&test=editing%2Fdeleting%2Fforward-delete-empty-table-cell.html
> 
> Hmm, crashes seem to look like:
> ASSERTION FAILED: areVisiblePositionsInSameTreeScope(result, vp) =>
> useDownstream ? (result > vp) : (result < vp)
> ./editing/VisibleUnits.cpp(1707) : WebCore::VisiblePosition
> WebCore::nextSentenceBoundaryInDirection(const WebCore::VisiblePosition &,
> WebCore::SelectionDirection)
> 1   0x1381f1d89 WTFCrash
> 2   0x163f1d5da
> WebCore::nextSentenceBoundaryInDirection(WebCore::VisiblePosition const&,
> WebCore::SelectionDirection)
> 3   0x163f1cb4a
> WebCore::positionOfNextBoundaryOfGranularity(WebCore::VisiblePosition
> const&, WebCore::TextGranularity, WebCore::SelectionDirection)
> 4   0x124cf43b6 WebKit::WebPage::autocorrectionContext()
> 5   0x124cf4c78 WebKit::WebPage::preemptivelySendAutocorrectionContext()
> 6   0x125a264eb WebKit::WebPage::didChangeSelection(WebCore::Frame&)
> 7   0x12571fb35
> WebKit::WebEditorClient::respondToChangedSelection(WebCore::Frame*)
> 8   0x163e5d360
> WebCore::Editor::respondToChangedSelection(WebCore::VisibleSelection const&,
> WTF::OptionSet<WebCore::FrameSelection::SetSelectionOption>)
> 9   0x163e68314
> WebCore::FrameSelection::setSelectionWithoutUpdatingAppearance(WebCore::
> VisibleSelection const&,
> WTF::OptionSet<WebCore::FrameSelection::SetSelectionOption>,
> WebCore::FrameSelection::CursorAlignOnScroll, WebCore::TextGranularity)
> 10  0x163e4b190
> WebCore::FrameSelection::setSelection(WebCore::VisibleSelection const&,
> WTF::OptionSet<WebCore::FrameSelection::SetSelectionOption>,
> WebCore::AXTextStateChangeIntent,
> WebCore::FrameSelection::CursorAlignOnScroll, WebCore::TextGranularity)
> 11  0x163e5bef6 WebCore::FrameSelection::moveTo(WebCore::Position const&,
> WebCore::Affinity, WebCore::EUserTriggered)
> 12  0x16498d294 WebCore::DOMSelection::collapse(WebCore::Node*, unsigned int)
> 13  0x160f9b8bd
> WebCore::jsDOMSelectionPrototypeFunction_collapseBody(JSC::JSGlobalObject*,
> JSC::CallFrame*, WebCore::JSDOMSelection*)::'lambda'()::operator()() const
> 14  0x160f9b5e1 JSC::JSValue WebCore::toJS<WebCore::IDLUndefined,
> WebCore::jsDOMSelectionPrototypeFunction_collapseBody(JSC::JSGlobalObject*,
> JSC::CallFrame*,
> WebCore::JSDOMSelection*)::'lambda'()>(JSC::JSGlobalObject&,
> JSC::ThrowScope&,
> WebCore::jsDOMSelectionPrototypeFunction_collapseBody(JSC::JSGlobalObject*,
> JSC::CallFrame*, WebCore::JSDOMSelection*)::'lambda'()&&)
> 15  0x160f9b51b
> WebCore::jsDOMSelectionPrototypeFunction_collapseBody(JSC::JSGlobalObject*,
> JSC::CallFrame*, WebCore::JSDOMSelection*)
> 16  0x160f9b0b5 long long
> WebCore::IDLOperation<WebCore::JSDOMSelection>::call<&(WebCore::
> jsDOMSelectionPrototypeFunction_collapseBody(JSC::JSGlobalObject*,
> JSC::CallFrame*, WebCore::JSDOMSelection*)),
> (WebCore::CastedThisErrorBehavior)0>(JSC::JSGlobalObject&, JSC::CallFrame&,
> char const*)
> 17  0x160f983a4
> WebCore::jsDOMSelectionPrototypeFunction_collapse(JSC::JSGlobalObject*,
> JSC::CallFrame*)
> 
> It does not look related to me.

Wenson says this is a regression from https://github.com/WebKit/WebKit/commit/5bd94703d84ac79d2a2d3fefe454709c54145e84, not my change.