Bug 239464 – Introduce makeAtomString()

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

[Chris Dumez]

Introduce makeAtomString() which is an optimized version of makeString() when the caller knows it wants an AtomString. It may avoid a StringImpl allocation when the string is already in the AtomStringTable.

[Chris Dumez]

Created attachment 457820 [details]
Patch

[Chris Dumez]

Created attachment 457824 [details]
Patch

[Darin Adler]

Comment on attachment 457824 [details]
Patch

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

> Source/WTF/wtf/text/StringConcatenate.h:502
> +        if (areAllAdapters8Bit) {
> +            LChar buffer[maxLengthToUseStackVariable];
> +            stringTypeAdapterAccumulator(buffer, adapter, adapters...);
> +            return AtomString { buffer, length };
> +        }
> +        UChar buffer[maxLengthToUseStackVariable];
> +        stringTypeAdapterAccumulator(buffer, adapter, adapters...);
> +        return AtomString { buffer, length };

I wonder if the compiler is smart enough to not allocate both buffers.

> Source/WTF/wtf/text/StringConcatenate.h:525
> +    if (areAllAdapters8Bit) {
> +        LChar* buffer;
> +        RefPtr<StringImpl> resultImpl = StringImpl::tryCreateUninitialized(length, buffer);
> +        if (!resultImpl)
> +            return nullAtom();
> +
> +        if (buffer)
> +            stringTypeAdapterAccumulator(buffer, adapter, adapters...);
> +
> +        return resultImpl.get();
> +    }
> +
> +    UChar* buffer;
> +    RefPtr<StringImpl> resultImpl = StringImpl::tryCreateUninitialized(length, buffer);
> +    if (!resultImpl)
> +        return nullAtom();
> +
> +    if (buffer)
> +        stringTypeAdapterAccumulator(buffer, adapter, adapters...);
> +
> +    return resultImpl.get();

Is there a way to share code with tryMakeStringFromAdapters here?

[Chris Dumez]

Created attachment 457836 [details]
Patch

[Chris Dumez]

Created attachment 457837 [details]
Patch

[Chris Dumez]

Created attachment 457838 [details]
Patch

[Darin Adler]

Comment on attachment 457838 [details]
Patch

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

> Source/WebCore/svg/SVGLengthValue.cpp:30
> +#include <wtf/text/StringConcatenate.h>
>  #include <wtf/text/StringConcatenateNumbers.h>

I thought "StringConcatenateNumbers.h" included "StringConcatenate.h"

[Chris Dumez]

(In reply to Darin Adler from comment #7)
> Comment on attachment 457838 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=457838&action=review
> 
> > Source/WebCore/svg/SVGLengthValue.cpp:30
> > +#include <wtf/text/StringConcatenate.h>
> >  #include <wtf/text/StringConcatenateNumbers.h>
> 
> I thought "StringConcatenateNumbers.h" included "StringConcatenate.h"

Oh, I think that was me trying to fix a build error the wrong way. I'll drop.

[Chris Dumez]

Created attachment 457841 [details]
Patch

[Chris Dumez]

(In reply to Darin Adler from comment #3)
> Comment on attachment 457824 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=457824&action=review
> 
> > Source/WTF/wtf/text/StringConcatenate.h:502
> > +        if (areAllAdapters8Bit) {
> > +            LChar buffer[maxLengthToUseStackVariable];
> > +            stringTypeAdapterAccumulator(buffer, adapter, adapters...);
> > +            return AtomString { buffer, length };
> > +        }
> > +        UChar buffer[maxLengthToUseStackVariable];
> > +        stringTypeAdapterAccumulator(buffer, adapter, adapters...);
> > +        return AtomString { buffer, length };
> 
> I wonder if the compiler is smart enough to not allocate both buffers.

Good question. I am not quite sure how to find out though.

[Sam Weinig]

Comment on attachment 457841 [details]
Patch

Very nice.

[EWS]

Committed r293024 (249763@main): <https://commits.webkit.org/249763@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/91972473>