Bug 238333 – String's startsWith() / endsWith() / replace() should take in a StringView instead of a String

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

[Chris Dumez]

String's startsWith() / endsWith() should take in a StringView instead of a String, to avoid unnecessary String creation in many instances.

[Chris Dumez]

Created attachment 455655 [details]
Patch

[Chris Dumez]

Created attachment 455661 [details]
Patch

[Chris Dumez]

Created attachment 455664 [details]
Patch

[Chris Dumez]

Created attachment 455666 [details]
Patch

[Geoffrey Garen]

Comment on attachment 455666 [details]
Patch

r=me

[Darin Adler]

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.

[Chris Dumez]

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

[Chris Dumez]

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

[Chris Dumez]

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

[Chris Dumez]

Created attachment 455682 [details]
Patch

[Chris Dumez]

Comment on attachment 455682 [details]
Patch

Clearing flags on attachment: 455682

Committed r291837 (248854@trunk): <https://commits.webkit.org/248854@trunk>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/90806465>