Bug 38930

Summary:

3% PLT Regression from moving strings into WTF

Product:

WebKit

Reporter:

Brian Weinstein <bweinstein>

Component:

Page Loading

Assignee:

Brian Weinstein <bweinstein>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, barraclough, cjerdonek, eric, hamaji, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

OS:

Windows 7

Bug Depends on:

38933

Bug Blocks:

Attachments:

Description	Flags
[PATCH] Fix	none
Patch	none
Patch	none
Patch	none
Patch	mjs: review+

Description Brian Weinstein 2010-05-11 12:55:59 PDT

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.

Comment 1 Brian Weinstein 2010-05-11 13:21:17 PDT

Created attachment 55743 [details]
[PATCH] Fix

Comment 2 WebKit Review Bot 2010-05-11 13:28:56 PDT

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.

Comment 3 Brian Weinstein 2010-05-11 13:41:47 PDT

Created attachment 55748 [details]
Patch

Comment 4 WebKit Review Bot 2010-05-11 13:44:03 PDT

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.

Comment 5 Eric Seidel (no email) 2010-05-11 13:57:51 PDT

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.

Comment 6 WebKit Review Bot 2010-05-11 14:06:56 PDT

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

Comment 7 Brian Weinstein 2010-05-11 15:43:27 PDT

Created attachment 55766 [details]
Patch

Comment 8 Eric Seidel (no email) 2010-05-11 15:49:42 PDT

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

Comment 9 WebKit Review Bot 2010-05-11 15:50:04 PDT

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.

Comment 10 Eric Seidel (no email) 2010-05-11 15:53:45 PDT

Looks like a bug in check-webkit-style.

Comment 11 Brian Weinstein 2010-05-11 15:54:56 PDT

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

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

Comment 12 Brian Weinstein 2010-05-11 15:55:38 PDT

Created attachment 55770 [details]
Patch

Comment 13 WebKit Review Bot 2010-05-11 15:58:17 PDT

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.

Comment 14 Jon Honeycutt 2010-05-11 16:37:02 PDT

Comment on attachment 55770 [details]
Patch

r=me

Comment 15 Brian Weinstein 2010-05-11 18:41:16 PDT

Created attachment 55793 [details]
Patch

Comment 16 Maciej Stachowiak 2010-05-11 19:17:25 PDT

Comment on attachment 55793 [details]
Patch

r=me

Comment 17 Brian Weinstein 2010-05-11 19:28:45 PDT

Landed in r59187 and r59196.

Comment 18 Eric Seidel (no email) 2010-05-11 20:42:04 PDT

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.