Bug 38930 – 3% PLT Regression from moving strings into WTF

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Brian Weinstein]

On Windows, moving strings from WebCore into WTF caused a 3% regression in the PLT test because this made all string operations from WebCore go across the DLL boundary.

[Brian Weinstein]

Created attachment 55743 [details]
[PATCH] Fix

[WebKit Review Bot]

Attachment 55743 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
JavaScriptCore/wtf/text/StringStatics.cpp:32:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
JavaScriptCore/wtf/text/StringStatics.cpp:52:  DEFINE_GLOBAL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/text/StringStatics.cpp:53:  DEFINE_GLOBAL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/text/StringStatics.cpp:54:  DEFINE_GLOBAL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/text/StringStatics.cpp:55:  DEFINE_GLOBAL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/text/StringStatics.cpp:56:  DEFINE_GLOBAL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/text/StringStatics.cpp:57:  DEFINE_GLOBAL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/text/StringStatics.cpp:58:  DEFINE_GLOBAL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 8 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brian Weinstein]

Created attachment 55748 [details]
Patch

[WebKit Review Bot]

Attachment 55748 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
JavaScriptCore/wtf/text/StringStatics.cpp:32:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
JavaScriptCore/wtf/text/StringStatics.cpp:52:  DEFINE_GLOBAL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/text/StringStatics.cpp:53:  DEFINE_GLOBAL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/text/StringStatics.cpp:54:  DEFINE_GLOBAL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/text/StringStatics.cpp:55:  DEFINE_GLOBAL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/text/StringStatics.cpp:56:  DEFINE_GLOBAL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/text/StringStatics.cpp:57:  DEFINE_GLOBAL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/text/StringStatics.cpp:58:  DEFINE_GLOBAL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 8 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Seidel (no email)]

As I suspect you're already aware, this broke a bunch of builders.
http://build.webkit.org/console

I'm sorry the EWS bots aren't faster so as to help us catch these sooner.  I'll work on making them poll more often.

[WebKit Review Bot]

http://trac.webkit.org/changeset/59171 might have broken Qt Linux Release minimal

[Brian Weinstein]

Created attachment 55766 [details]
Patch

[Eric Seidel (no email)]

Attachment 55766 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/2174143

[WebKit Review Bot]

Attachment 55766 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
JavaScriptCore/wtf/text/StringStatics.cpp:1:  One or more unexpected \r (^M) found; better to use only a \n  [whitespace/carriage_return] [1]
Suppressing further [whitespace/carriage_return] reports for this file.
JavaScriptCore/wtf/text/StringStatics.cpp:32:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
JavaScriptCore/wtf/text/StringStatics.cpp:52:  DEFINE_GLOBAL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/text/StringStatics.cpp:53:  DEFINE_GLOBAL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/text/StringStatics.cpp:54:  DEFINE_GLOBAL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/text/StringStatics.cpp:55:  DEFINE_GLOBAL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/text/StringStatics.cpp:56:  DEFINE_GLOBAL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/text/StringStatics.cpp:57:  DEFINE_GLOBAL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/text/StringStatics.cpp:58:  DEFINE_GLOBAL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/text/StringStatics.cpp:80:  Could not find a newline character at the end of the file.  [whitespace/ending_newline] [5]
Total errors found: 88 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Seidel (no email)]

Looks like a bug in check-webkit-style.

[Brian Weinstein]

(In reply to comment #10)
> Looks like a bug in check-webkit-style.

Filed: https://bugs.webkit.org/show_bug.cgi?id=38944

[Brian Weinstein]

Created attachment 55770 [details]
Patch

[WebKit Review Bot]

Attachment 55770 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
JavaScriptCore/wtf/text/StringStatics.cpp:1:  One or more unexpected \r (^M) found; better to use only a \n  [whitespace/carriage_return] [1]
Suppressing further [whitespace/carriage_return] reports for this file.
JavaScriptCore/wtf/text/StringStatics.cpp:32:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
JavaScriptCore/wtf/text/StringStatics.cpp:52:  DEFINE_GLOBAL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/text/StringStatics.cpp:53:  DEFINE_GLOBAL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/text/StringStatics.cpp:54:  DEFINE_GLOBAL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/text/StringStatics.cpp:55:  DEFINE_GLOBAL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/text/StringStatics.cpp:56:  DEFINE_GLOBAL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/text/StringStatics.cpp:57:  DEFINE_GLOBAL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/text/StringStatics.cpp:58:  DEFINE_GLOBAL is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 88 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jon Honeycutt]

Comment on attachment 55770 [details]
Patch

r=me

[Brian Weinstein]

Created attachment 55793 [details]
Patch

[Maciej Stachowiak]

Comment on attachment 55793 [details]
Patch

r=me

[Brian Weinstein]

Landed in r59187 and r59196.

[Eric Seidel (no email)]

I think this broke linking on Gtk:

./.libs/libwebkit-1.0.so: undefined reference to `WebCore::String::length() const'
./.libs/libwebkit-1.0.so: undefined reference to `WebCore::String::isEmpty() const'
./.libs/libwebkit-1.0.so: undefined reference to `WebCore::AtomicString::add(WebCore::StringImpl*)'
collect2: ld returned 1 exit status

But it could have been http://trac.webkit.org/changeset/59197 instead.  Unsure.