String's startsWith() / endsWith() should take in a StringView instead of a String, to avoid unnecessary String creation in many instances.
Created attachment 455655 [details] Patch
Created attachment 455661 [details] Patch
Created attachment 455664 [details] Patch
Created attachment 455666 [details] Patch
Comment on attachment 455666 [details] Patch r=me
Comment on attachment 455666 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=455666&action=review > Source/WTF/wtf/text/StringImpl.h:459 > - WTF_EXPORT_PRIVATE Ref<StringImpl> replace(unsigned start, unsigned length, StringImpl*); > + WTF_EXPORT_PRIVATE Ref<StringImpl> replace(unsigned start, unsigned length, StringView); This improved interface precludes the possible optimization for cases where the start is 0 and length is >= the existing length of the string, where we can just return the passed-in StringImpl*. Is that an important fast case anywhere? > Source/WebCore/platform/graphics/angle/GraphicsContextGLANGLE.cpp:1914 > + static const char angleWarning[] = "WARNING: 0:1: extension 'GL_ARB_gpu_shader5' is not supported\n"; When touching code like this I suggest considering constexpr. > Source/WebCore/platform/graphics/angle/GraphicsContextGLANGLE.cpp:1915 > + int startFrom = log.startsWith(angleWarning) ? sizeof(angleWarning) - 1 : 0; I would probably use std::size instead of sizeof, but I’ve got to admit I am not in love with doing size - 1 for length. Is there some more elegant way to do it? We know that strlen would get const-folded, might even be guaranteed in modern versions of C++. But that’s not completely elegant I suppose. > Source/WebCore/platform/graphics/opengl/GraphicsContextGLOpenGL.cpp:1977 > + static const char angleWarning[] = "WARNING: 0:1: extension 'GL_ARB_gpu_shader5' is not supported\n"; Ditto. > Source/WebCore/platform/network/ParsedRequestRange.cpp:36 > + if (!input.startsWith(StringView("bytes="))) I think we don’t need the explicit StringView here any more, right? > Source/WebKit/NetworkProcess/storage/LocalStorageManager.cpp:45 > + if (!fileName.endsWith(StringView { fileSuffix })) Do we really need an explicit cast to StringView here? If so, why? What’s ambiguous here? > Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:4451 > static bool matchesExtensionOrEquivalent(const String& filename, const String& extension) Seems like these arguments could both be StringView.
(In reply to Darin Adler from comment #6) > Comment on attachment 455666 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=455666&action=review > > > Source/WTF/wtf/text/StringImpl.h:459 > > - WTF_EXPORT_PRIVATE Ref<StringImpl> replace(unsigned start, unsigned length, StringImpl*); > > + WTF_EXPORT_PRIVATE Ref<StringImpl> replace(unsigned start, unsigned length, StringView); > > This improved interface precludes the possible optimization for cases where > the start is 0 and length is >= the existing length of the string, where we > can just return the passed-in StringImpl*. Is that an important fast case > anywhere? That's a very good point. I should have paid more attention. I may revert that part after some investigation. > > > Source/WebCore/platform/graphics/angle/GraphicsContextGLANGLE.cpp:1914 > > + static const char angleWarning[] = "WARNING: 0:1: extension 'GL_ARB_gpu_shader5' is not supported\n"; > > When touching code like this I suggest considering constexpr. OK. > > > Source/WebCore/platform/graphics/angle/GraphicsContextGLANGLE.cpp:1915 > > + int startFrom = log.startsWith(angleWarning) ? sizeof(angleWarning) - 1 : 0; > > I would probably use std::size instead of sizeof, but I’ve got to admit I am > not in love with doing size - 1 for length. Is there some more elegant way > to do it? We know that strlen would get const-folded, might even be > guaranteed in modern versions of C++. But that’s not completely elegant I > suppose. I think I'll just use strlen(). It should get optimized out by the compiler anyway and it does look much nicer. > > > Source/WebCore/platform/graphics/opengl/GraphicsContextGLOpenGL.cpp:1977 > > + static const char angleWarning[] = "WARNING: 0:1: extension 'GL_ARB_gpu_shader5' is not supported\n"; > > Ditto. > > > Source/WebCore/platform/network/ParsedRequestRange.cpp:36 > > + if (!input.startsWith(StringView("bytes="))) > > I think we don’t need the explicit StringView here any more, right? Indeed. > > > Source/WebKit/NetworkProcess/storage/LocalStorageManager.cpp:45 > > + if (!fileName.endsWith(StringView { fileSuffix })) > > Do we really need an explicit cast to StringView here? If so, why? What’s > ambiguous here? The StringView(ASCIILiteral) constructor is explicit for some reason. It is not directly obvious to me why we want it to be explicit. > > > Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:4451 > > static bool matchesExtensionOrEquivalent(const String& filename, const String& extension) > > Seems like these arguments could both be StringView. Will look into it.
(In reply to Chris Dumez from comment #7) > (In reply to Darin Adler from comment #6) > > Comment on attachment 455666 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=455666&action=review > > > > > Source/WTF/wtf/text/StringImpl.h:459 > > > - WTF_EXPORT_PRIVATE Ref<StringImpl> replace(unsigned start, unsigned length, StringImpl*); > > > + WTF_EXPORT_PRIVATE Ref<StringImpl> replace(unsigned start, unsigned length, StringView); > > > > This improved interface precludes the possible optimization for cases where > > the start is 0 and length is >= the existing length of the string, where we > > can just return the passed-in StringImpl*. Is that an important fast case > > anywhere? > > That's a very good point. I should have paid more attention. I may revert > that part after some investigation. > > > > > > Source/WebCore/platform/graphics/angle/GraphicsContextGLANGLE.cpp:1914 > > > + static const char angleWarning[] = "WARNING: 0:1: extension 'GL_ARB_gpu_shader5' is not supported\n"; > > > > When touching code like this I suggest considering constexpr. > > OK. > > > > > > Source/WebCore/platform/graphics/angle/GraphicsContextGLANGLE.cpp:1915 > > > + int startFrom = log.startsWith(angleWarning) ? sizeof(angleWarning) - 1 : 0; > > > > I would probably use std::size instead of sizeof, but I’ve got to admit I am > > not in love with doing size - 1 for length. Is there some more elegant way > > to do it? We know that strlen would get const-folded, might even be > > guaranteed in modern versions of C++. But that’s not completely elegant I > > suppose. > > I think I'll just use strlen(). It should get optimized out by the compiler > anyway and it does look much nicer. > > > > > > Source/WebCore/platform/graphics/opengl/GraphicsContextGLOpenGL.cpp:1977 > > > + static const char angleWarning[] = "WARNING: 0:1: extension 'GL_ARB_gpu_shader5' is not supported\n"; > > > > Ditto. > > > > > Source/WebCore/platform/network/ParsedRequestRange.cpp:36 > > > + if (!input.startsWith(StringView("bytes="))) > > > > I think we don’t need the explicit StringView here any more, right? > > Indeed. > > > > > > Source/WebKit/NetworkProcess/storage/LocalStorageManager.cpp:45 > > > + if (!fileName.endsWith(StringView { fileSuffix })) > > > > Do we really need an explicit cast to StringView here? If so, why? What’s > > ambiguous here? > > The StringView(ASCIILiteral) constructor is explicit for some reason. It is > not directly obvious to me why we want it to be explicit. > > > > > > Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:4451 > > > static bool matchesExtensionOrEquivalent(const String& filename, const String& extension) > > > > Seems like these arguments could both be StringView. > > Will look into it. There is a single call site and both parameters are of type `NSString *`. As a result, I would need to explicitly construct a String at the call site instead, which would not be obviously better?
(In reply to Darin Adler from comment #6) > Comment on attachment 455666 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=455666&action=review > > > Source/WTF/wtf/text/StringImpl.h:459 > > - WTF_EXPORT_PRIVATE Ref<StringImpl> replace(unsigned start, unsigned length, StringImpl*); > > + WTF_EXPORT_PRIVATE Ref<StringImpl> replace(unsigned start, unsigned length, StringView); > > This improved interface precludes the possible optimization for cases where > the start is 0 and length is >= the existing length of the string, where we > can just return the passed-in StringImpl*. Is that an important fast case > anywhere? I looked at our implementation and there is no such optimization currently. Therefore, my change is not disabling an existing optimization at the very least.
Created attachment 455682 [details] Patch
Comment on attachment 455682 [details] Patch Clearing flags on attachment: 455682 Committed r291837 (248854@trunk): <https://commits.webkit.org/248854@trunk>
All reviewed patches have been landed. Closing bug.
<rdar://problem/90806465>