Bug 238162 – Inline String(ASCIILiteral) constructor so the compiler can optimize out strlen()

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

[Chris Dumez]

String literals with _str suffix should not do strlen() at runtime:
Sample Count, Samples %, Normalized CPU %, Symbol
25, 0.0%, 0.0%, _platform_strlen (in libsystem_platform.dylib)
4, 0.0%, 0.0%,     WTF::StringImpl::createFromLiteral(WTF::ASCIILiteral) (in JavaScriptCore)
4, 0.0%, 0.0%,         WTF::String::String(WTF::ASCIILiteral) (in JavaScriptCore)
2, 0.0%, 0.0%,             JSC::ProxyObject::performGet(JSC::JSGlobalObject*, JSC::PropertyName, JSC::PropertySlot&) (in JavaScriptCore)
1, 0.0%, 0.0%,             JSC::ProxyObject::performDefineOwnProperty(JSC::JSGlobalObject*, JSC::PropertyName, JSC::PropertyDescriptor const&, bool) (in JavaScriptCore)
1, 0.0%, 0.0%,             JSC::ProxyObject::performInternalMethodGetOwnProperty(JSC::JSGlobalObject*, JSC::PropertyName, JSC::PropertySlot&) (in JavaScriptCore)

[Chris Dumez]

Created attachment 455295 [details]
Patch

[Chris Dumez]

Created attachment 455321 [details]
Patch

[Chris Dumez]

Created attachment 455332 [details]
Patch

[Chris Dumez]

Confirmed 0.6-0.8% progression on Speedometer on iMac 20,1 on a/b bots.

[Darin Adler]

Comment on attachment 455332 [details]
Patch

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

So unclear to me when to use _s vs. _str — please write down a guideline for future use somewhere. Maybe we’ll even want to rename one or both so it’s more obvious when one is right vs. the other.

If ""_s was smaller code than ""_str then you’d want to decide based on how likely a code path was perhaps? I personally don’t like having a choice when it’s not obvious which is better, and there are so often choices where you could use "", ""_s, or ""_str.

For example, does a string literal argument to makeString compile into a code sequence that involves call to strlen? Or does that get constant-folded? What about if you use ""_s or ""_str instead of just ""?

> Source/WTF/ChangeLog:9
> +        it could trigger calls to strlen() at run time to figure out the length of the string. This was unforunate

Typo in unfortunate

> Source/WTF/ChangeLog:14
> +        This patch also adopts the ""_str operator in more places so they can leverage the optimization.

Is this a measurable speedup? Is it a measurable code size increase or decrease?

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2538
> +                return $useAtomString ? "AtomString(${defaultValue}, AtomString::ConstructFromLiteral)" : "${defaultValue}_str";

Looks like you need to rebase the bindings test results because of these changes to CodeGeneratorJS.pm.

[Chris Dumez]

(In reply to Darin Adler from comment #5)
> Comment on attachment 455332 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=455332&action=review
> 
> So unclear to me when to use _s vs. _str — please write down a guideline for
> future use somewhere. Maybe we’ll even want to rename one or both so it’s
> more obvious when one is right vs. the other.

Basically the idea is to use _str whenever you actually want a String and _s when you want an ASCIILiteral. Right now, it seems that most people were using _s unconditionally since we can silently construct a String from an ASCIILiteral.

> 
> If ""_s was smaller code than ""_str then you’d want to decide based on how
> likely a code path was perhaps? I personally don’t like having a choice when
> it’s not obvious which is better, and there are so often choices where you
> could use "", ""_s, or ""_str.

I agree. It seems that right now, most people use _s and it won't be the most efficient anymore for constructing a String.
Maybe if we made String constructor that takes in an ASCIILiteral explicit, then people would be a lot more likely to adopt _str since they'd have to use String { "foo"_s } otherwise, which is just longer than "foo"_str.

> For example, does a string literal argument to makeString compile into a
> code sequence that involves call to strlen? Or does that get
> constant-folded? What about if you use ""_s or ""_str instead of just ""?

That part I don't have an answer to at the moment and it will need more investigation.

> 
> > Source/WTF/ChangeLog:9
> > +        it could trigger calls to strlen() at run time to figure out the length of the string. This was unforunate
> 
> Typo in unfortunate
> 
> > Source/WTF/ChangeLog:14
> > +        This patch also adopts the ""_str operator in more places so they can leverage the optimization.
> 
> Is this a measurable speedup? Is it a measurable code size increase or
> decrease?

I haven't measured code size since I didn't expect any changes (the size parameter was already there, I don't do any templating). I can check though.
With regards to perf, yes. See https://bugs.webkit.org/show_bug.cgi?id=238162#c4.

> 
> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2538
> > +                return $useAtomString ? "AtomString(${defaultValue}, AtomString::ConstructFromLiteral)" : "${defaultValue}_str";
> 
> Looks like you need to rebase the bindings test results because of these
> changes to CodeGeneratorJS.pm.

Yes, I purposefully omitted those changes from the patch to facilitate review as the patch was already large.

[Chris Dumez]

I have just checked and there is no impact on the binary size for either the JavaScriptCore or the WebCore frameworks.

[Geoffrey Garen]

> I agree. It seems that right now, most people use _s and it won't be the
> most efficient anymore for constructing a String.
> Maybe if we made String constructor that takes in an ASCIILiteral explicit,
> then people would be a lot more likely to adopt _str since they'd have to
> use String { "foo"_s } otherwise, which is just longer than "foo"_str.

I think making String(ASCIILiteral) explicit makes sense. Based on this patch, I wonder how many, if any, uses of String(ASCIILiteral) are on purpose.

I also think we should consider making the "good" (fast) literal the easy thing to type out, and making the "bad" (slow) literal more verbose. Maybe:

    ""_s => String

    ""_cstring => ASCIILiteral

But maybe I'm underestimating the non-String uses of ASCIILiteral?

Alternatively, would it be crazy for ASCIILiteral to store its length as a data member? If it did, then this distinction might not matter.

[Chris Dumez]

(In reply to Geoffrey Garen from comment #8)
> > I agree. It seems that right now, most people use _s and it won't be the
> > most efficient anymore for constructing a String.
> > Maybe if we made String constructor that takes in an ASCIILiteral explicit,
> > then people would be a lot more likely to adopt _str since they'd have to
> > use String { "foo"_s } otherwise, which is just longer than "foo"_str.
> 
> I think making String(ASCIILiteral) explicit makes sense. Based on this
> patch, I wonder how many, if any, uses of String(ASCIILiteral) are on
> purpose.
> 
> I also think we should consider making the "good" (fast) literal the easy
> thing to type out, and making the "bad" (slow) literal more verbose. Maybe:
> 
>     ""_s => String
> 
>     ""_cstring => ASCIILiteral
> 
> But maybe I'm underestimating the non-String uses of ASCIILiteral?

I have a feeling this would probably work out but I'd have to try it out to see how much ASCIILiteral usage remains. Like you, I believe most of the time, we want a String.

> 
> Alternatively, would it be crazy for ASCIILiteral to store its length as a
> data member? If it did, then this distinction might not matter.

Adding the length as data member to ASCIILiteral was actually my first attempt. However, this was a perf regression on Speedometer according to A/B bots... Weird.

[Darin Adler]

ASCIILiteral was created with a single mission: Giving a way to tell the String constructor that the backing characters are immortal because they are a compile-time ASCII literal. Once we had it we realized that it can also be used anywhere else in code we would otherwise write "const char*" but wish to convey the information that the characters are immortal as part of the type. There should be few non-String uses, so some of the discussion above doesn’t make a lot of sense to me.

If we see a performance regression putting lengths into all ASCIILiteral objects, my guess is it has something to do with greater code size. But just a wild guess.

[Chris Dumez]

(In reply to Darin Adler from comment #10)
> ASCIILiteral was created with a single mission: Giving a way to tell the
> String constructor that the backing characters are immortal because they are
> a compile-time ASCII literal. Once we had it we realized that it can also be
> used anywhere else in code we would otherwise write "const char*" but wish
> to convey the information that the characters are immortal as part of the
> type. There should be few non-String uses, so some of the discussion above
> doesn’t make a lot of sense to me.

What do you think of the proposal to use ""_s to construct a String since it is the common case and use something longer like ""_cstr or ""_cstring for constructing an ASCIILiteral since it should become a lot less common?

[Darin Adler]

Sounds fine to me. I just want to make sure I understand where ""_s is good and where is it not; ideally there are compile-time errors when you make the most obvious mistakes. It would be a true tragedy if people created and destroyed a String as one of the arguments to makeString, to give just one example. Or when passing a string to a function that takes a StringView.

[Chris Dumez]

(In reply to Darin Adler from comment #12)
> Sounds fine to me. I just want to make sure I understand where ""_s is good
> and where is it not; ideally there are compile-time errors when you make the
> most obvious mistakes. It would be a true tragedy if people created and
> destroyed a String as one of the arguments to makeString, to give just one
> example. Or when passing a string to a function that takes a StringView.

I agree it would be unfortunate to construct String implicitly in cases where it isn't needed. I'll be careful about that.

With this patch, if you call something that needs a String, passing "foo"_str is the most efficient. Passing "foo"_s will compile but be slightly less efficient (as it will avoid the character copy but not the strlen). I think this second part could be addressed by making the String(ASCIILiteral) constructor explicit but this is likely a large change until I have adopted _str in more places.

For now, I'll keep the patch as is (except for the typo fix and bindings rebaselining). I'll follow-up with more _str adoption (and maybe changing suffixes if we come up with a good proposal).

[Chris Dumez]

Created attachment 455400 [details]
Patch

[Chris Dumez]

As far as I can tell, the patch as-is is not controversial since it makes the pre-existing ""_str faster. ""_s still exist and has the same behavior / performance as before.

The only risk is that people will keep using ""_s in cases where they really needed a String, as is currently pretty common. That said, this is the current status quo anyway. We can also make this less likely eventually by:
- Port existing code to ""_str when suitable (like I started doing in this patch)
- Making String(ASCIILiteral) explicit
- Maybe send an email out to webkit-dev?

[Darin Adler]

There’s also the risk that confusion just makes it harder to code on WebKit.

[Geoffrey Garen]

FWIW, I'm happy with this patch landing as is, since it's a speedup, and the subtle distinction between _s and _str is not new to this patch.

I'd also like to continue discussing options to reduce subtlety / confusion.

Here's another option:

Create a new type, ASCIILiteralWithLength
_s creates an ASCIILiteralWithLength
ASCIILiteral(ASCIILiteralWithLength) is implicit
String(ASCIILiteralWithLength) is implicit
String(ASCIILiteral) is explicit

Reasons to consider:

(1) Makes the fastest String constructor easy / default and the variant that calls strlen() harder / non-default, reducing (or even eliminating?) the need for "I guess we should have used _str here to avoid strlen()".

(2) Might avoid the regression that always storing length in ASCIILiteral caused (if that regression was caused by bloating the size of globals or data members typed ASCIILiteral).

[Darin Adler]

I find that plan appealing. Might also want to shift at some point so that long term the two names could be ASCIILiteralWithoutLength and ASCIILiteral.

But also please consider my request that we find a way to make the most common mistakes result in compilation errors rather than inefficient idioms scattered randomly across the code.

[Chris Dumez]

Created attachment 455460 [details]
Geoff's alternative proposal

I need to A/B test this.

[EWS Watchlist]

This patch modifies the inspector protocol generator. Please ensure that you have rebaselined any generator test results (i.e., by running `Tools/Scripts/run-inspector-generator-tests --reset-results`)

[Chris Dumez]

Created attachment 455462 [details]
Geoff's alternative proposal

[Chris Dumez]

Created attachment 455464 [details]
Geoff's alternative proposal

[Chris Dumez]

Created attachment 455465 [details]
Geoff's alternative proposal

[Chris Dumez]

Created attachment 455468 [details]
Geoff's alternative proposal

[Chris Dumez]

Created attachment 455497 [details]
Geoff's alternative proposal

[Chris Dumez]

I got A/B results back from Geoff's proposal. It is performance neutral on Speedometer on Both Intel and Apple Silicon.

My other patch with ""_str looked like a 0.6-0.8% progression on a/b bots on Intel (neutral on Apple Silicon).

[Geoffrey Garen]

> I got A/B results back from Geoff's proposal. It is performance neutral on
> Speedometer on Both Intel and Apple Silicon.

Frustrating!

OK, I hate to keep going 'round the merry-go-round, but I have another idea.

Premise 1: Our problem is that we're calling strlen(), even though we totally know the length.

Premise 2: strlen() will reliably constant fold at both -O2 and -Os. Test case:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

struct ASCIILiteral {
    size_t length() { return strlen(ptr); }
    const char* ptr;
};

inline ASCIILiteral test(ASCIILiteral a)
{
    if (a.length() != 2)
        abort();
    return a;
}

__attribute__((noinline)) void f() 
{
    auto a = ASCIILiteral { "hi" };
    auto b = test(a);
    if (b.length() != 2)
        abort();
}

int main(int arc, char** argv)
{
    f();
    return 0;
}

Test case output:

~> clang++ -std=c++17 -Os -o scratch scratch.cc && otool -tvV scratch
scratch:
(__TEXT,__text) section
__Z1fv:
0000000100003f94	ret
_main:
0000000100003f98	stp	x29, x30, [sp, #-0x10]!
0000000100003f9c	mov	x29, sp
0000000100003fa0	bl	__Z1fv
0000000100003fa4	mov	w0, #0x0
0000000100003fa8	ldp	x29, x30, [sp], #0x10
0000000100003fac	ret

~> clang++ -std=c++17 -O2 -o scratch scratch.cc && otool -tvV scratch
scratch:
(__TEXT,__text) section
__Z1fv:
0000000100003f94	ret
_main:
0000000100003f98	stp	x29, x30, [sp, #-0x10]!
0000000100003f9c	mov	x29, sp
0000000100003fa0	bl	__Z1fv
0000000100003fa4	mov	w0, #0x0
0000000100003fa8	ldp	x29, x30, [sp], #0x10
0000000100003fac	ret

Proposal:

(1) Inline a few functions to ensure that String(""_s) inlines construction of ASCIILiteral and access to ASCIILiteral::length().

I think this means inlining String(ASCIILiteral), StringImpl::createFromLiteral(const char*), and StringImpl::createFromLiteral(ASCIILiteral) (but not their callees).

(2) Verify that strlen() disappeared from the profile -- or if it didn't, wow, where are we constructing String(ASCIILiteral) where ASCIILiteral is not constant, and yet changing _s to _str is a speedup? - seems impossible.

Reason to prefer:

If this works, the difference between _str and _s becomes negligible, and maybe _str can even go away?

[Chris Dumez]

(In reply to Geoffrey Garen from comment #27)
> > I got A/B results back from Geoff's proposal. It is performance neutral on
> > Speedometer on Both Intel and Apple Silicon.
> 
> Frustrating!
> 
> OK, I hate to keep going 'round the merry-go-round, but I have another idea.
> 
> Premise 1: Our problem is that we're calling strlen(), even though we
> totally know the length.
> 
> Premise 2: strlen() will reliably constant fold at both -O2 and -Os. Test
> case:
> 
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
> 
> struct ASCIILiteral {
>     size_t length() { return strlen(ptr); }
>     const char* ptr;
> };
> 
> inline ASCIILiteral test(ASCIILiteral a)
> {
>     if (a.length() != 2)
>         abort();
>     return a;
> }
> 
> __attribute__((noinline)) void f() 
> {
>     auto a = ASCIILiteral { "hi" };
>     auto b = test(a);
>     if (b.length() != 2)
>         abort();
> }
> 
> int main(int arc, char** argv)
> {
>     f();
>     return 0;
> }
> 
> Test case output:
> 
> ~> clang++ -std=c++17 -Os -o scratch scratch.cc && otool -tvV scratch
> scratch:
> (__TEXT,__text) section
> __Z1fv:
> 0000000100003f94	ret
> _main:
> 0000000100003f98	stp	x29, x30, [sp, #-0x10]!
> 0000000100003f9c	mov	x29, sp
> 0000000100003fa0	bl	__Z1fv
> 0000000100003fa4	mov	w0, #0x0
> 0000000100003fa8	ldp	x29, x30, [sp], #0x10
> 0000000100003fac	ret
> 
> ~> clang++ -std=c++17 -O2 -o scratch scratch.cc && otool -tvV scratch
> scratch:
> (__TEXT,__text) section
> __Z1fv:
> 0000000100003f94	ret
> _main:
> 0000000100003f98	stp	x29, x30, [sp, #-0x10]!
> 0000000100003f9c	mov	x29, sp
> 0000000100003fa0	bl	__Z1fv
> 0000000100003fa4	mov	w0, #0x0
> 0000000100003fa8	ldp	x29, x30, [sp], #0x10
> 0000000100003fac	ret
> 
> Proposal:
> 
> (1) Inline a few functions to ensure that String(""_s) inlines construction
> of ASCIILiteral and access to ASCIILiteral::length().
> 
> I think this means inlining String(ASCIILiteral),
> StringImpl::createFromLiteral(const char*), and
> StringImpl::createFromLiteral(ASCIILiteral) (but not their callees).
> 
> (2) Verify that strlen() disappeared from the profile -- or if it didn't,
> wow, where are we constructing String(ASCIILiteral) where ASCIILiteral is
> not constant, and yet changing _s to _str is a speedup? - seems impossible.
> 
> Reason to prefer:
> 
> If this works, the difference between _str and _s becomes negligible, and
> maybe _str can even go away?

Definitely worth a try. I'll do so today.

In parallel, I am also working on making the String(const char*) constructor explicit as it helps catching many case where the ""_s suffix is missing for String literals.

[Chris Dumez]

Created attachment 455546 [details]
Patch

[Chris Dumez]

Created attachment 455547 [details]
Patch

[Chris Dumez]

(In reply to Geoffrey Garen from comment #27)
> > I got A/B results back from Geoff's proposal. It is performance neutral on
> > Speedometer on Both Intel and Apple Silicon.
> 
> Frustrating!
> 
> OK, I hate to keep going 'round the merry-go-round, but I have another idea.
> 
> Premise 1: Our problem is that we're calling strlen(), even though we
> totally know the length.
> 
> Premise 2: strlen() will reliably constant fold at both -O2 and -Os. Test
> case:
> 
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
> 
> struct ASCIILiteral {
>     size_t length() { return strlen(ptr); }
>     const char* ptr;
> };
> 
> inline ASCIILiteral test(ASCIILiteral a)
> {
>     if (a.length() != 2)
>         abort();
>     return a;
> }
> 
> __attribute__((noinline)) void f() 
> {
>     auto a = ASCIILiteral { "hi" };
>     auto b = test(a);
>     if (b.length() != 2)
>         abort();
> }
> 
> int main(int arc, char** argv)
> {
>     f();
>     return 0;
> }
> 
> Test case output:
> 
> ~> clang++ -std=c++17 -Os -o scratch scratch.cc && otool -tvV scratch
> scratch:
> (__TEXT,__text) section
> __Z1fv:
> 0000000100003f94	ret
> _main:
> 0000000100003f98	stp	x29, x30, [sp, #-0x10]!
> 0000000100003f9c	mov	x29, sp
> 0000000100003fa0	bl	__Z1fv
> 0000000100003fa4	mov	w0, #0x0
> 0000000100003fa8	ldp	x29, x30, [sp], #0x10
> 0000000100003fac	ret
> 
> ~> clang++ -std=c++17 -O2 -o scratch scratch.cc && otool -tvV scratch
> scratch:
> (__TEXT,__text) section
> __Z1fv:
> 0000000100003f94	ret
> _main:
> 0000000100003f98	stp	x29, x30, [sp, #-0x10]!
> 0000000100003f9c	mov	x29, sp
> 0000000100003fa0	bl	__Z1fv
> 0000000100003fa4	mov	w0, #0x0
> 0000000100003fa8	ldp	x29, x30, [sp], #0x10
> 0000000100003fac	ret
> 
> Proposal:
> 
> (1) Inline a few functions to ensure that String(""_s) inlines construction
> of ASCIILiteral and access to ASCIILiteral::length().
> 
> I think this means inlining String(ASCIILiteral),
> StringImpl::createFromLiteral(const char*), and
> StringImpl::createFromLiteral(ASCIILiteral) (but not their callees).
> 
> (2) Verify that strlen() disappeared from the profile -- or if it didn't,
> wow, where are we constructing String(ASCIILiteral) where ASCIILiteral is
> not constant, and yet changing _s to _str is a speedup? - seems impossible.
> 
> Reason to prefer:
> 
> If this works, the difference between _str and _s becomes negligible, and
> maybe _str can even go away?

Yes, that works! According to the profiler, no more strlen() under String(ASCIILiteral) / StringImpl::createFromLiteral(ASCIILiteral).

[Geoffrey Garen]

> Yes, that works! According to the profiler, no more strlen() under
> String(ASCIILiteral) / StringImpl::createFromLiteral(ASCIILiteral).

Cool!

Knowing that, if the inlining patch is still not as fast as the _str patch on the bots, one thing to consider is that some functions might be overloaded for both String and ASCIILiteral, with the String overload being faster for some incidental reason. (Maybe the String overload is inlined and the ASCIILiteral overload is not, or some other silly thing.)

But hopefully the inlining patch will tie or beat the _str patch on the bots. Then things get a lot simpler.

[Geoffrey Garen]

Comment on attachment 455547 [details]
Patch

r=me

[Chris Dumez]

Hmmm:
ERROR: JavaScriptCore has a weak external symbol in it (/Volumes/Data/worker/macOS-AppleSilicon-Big-Sur-Debug-Build-EWS/build/WebKitBuild/Debug/JavaScriptCore.framework/Versions/A/JavaScriptCore)
ERROR: A weak external symbol is generated when a symbol is defined in multiple compilation units and is also marked as being exported from the library.
ERROR: A common cause of weak external symbols is when an inline function is listed in the linker export file.
ERROR: symbol __ZN3WTF10StringImpl17createFromLiteralENS_12ASCIILiteralE

[Chris Dumez]

Created attachment 455577 [details]
Patch

[Geoffrey Garen]

> ERROR: JavaScriptCore has a weak external symbol in it
> (/Volumes/Data/worker/macOS-AppleSilicon-Big-Sur-Debug-Build-EWS/build/
> WebKitBuild/Debug/JavaScriptCore.framework/Versions/A/JavaScriptCore)

I think you just need to remove WTF_EXPORT_PRIVATE? (New patch seems to do more than that.)

[Chris Dumez]

(In reply to Geoffrey Garen from comment #36)
> > ERROR: JavaScriptCore has a weak external symbol in it
> > (/Volumes/Data/worker/macOS-AppleSilicon-Big-Sur-Debug-Build-EWS/build/
> > WebKitBuild/Debug/JavaScriptCore.framework/Versions/A/JavaScriptCore)
> 
> I think you just need to remove WTF_EXPORT_PRIVATE? (New patch seems to do
> more than that.)

Yes, I am hoping removing WTF_EXPORT_PRIVATE will do the trick.

I noticed that the other (templated) createFromLiteral() is unused outside tests so I think we should drop it.

[Chris Dumez]

(In reply to Chris Dumez from comment #37)
> (In reply to Geoffrey Garen from comment #36)
> > > ERROR: JavaScriptCore has a weak external symbol in it
> > > (/Volumes/Data/worker/macOS-AppleSilicon-Big-Sur-Debug-Build-EWS/build/
> > > WebKitBuild/Debug/JavaScriptCore.framework/Versions/A/JavaScriptCore)
> > 
> > I think you just need to remove WTF_EXPORT_PRIVATE? (New patch seems to do
> > more than that.)
> 
> Yes, I am hoping removing WTF_EXPORT_PRIVATE will do the trick.
> 
> I noticed that the other (templated) createFromLiteral() is unused outside
> tests so I think we should drop it.

Would you prefer I move the removal of the (unused) templated createFromLiteral() out of this patch?

[Geoffrey Garen]

> Would you prefer I move the removal of the (unused) templated
> createFromLiteral() out of this patch?

No preference.

[Chris Dumez]

Created attachment 455594 [details]
Patch

[EWS]

Committed r291787 (248815@main): <https://commits.webkit.org/248815@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/90752707>

[Darin Adler]

Comment on attachment 455594 [details]
Patch

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

> Tools/TestWebKitAPI/Tests/WTF/StringImpl.cpp:38
>      // Constructor using the template to determine the size.

This comment is no longer accurate. We should probably delete or reword it.

[Chris Dumez]

I may actually need to revert completely. A/B came back as a 2-3% regression. I suspect too much is getting inlined?

[Chris Dumez]

(In reply to Chris Dumez from comment #44)
> I may actually need to revert completely. A/B came back as a 2-3%
> regression. I suspect too much is getting inlined?

Although I don't see that regression on the regular bots after this landed. I'll look into it more.