Bug 38930 - 3% PLT Regression from moving strings into WTF
Summary: 3% PLT Regression from moving strings into WTF
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows 7
: P2 Normal
Assignee: Brian Weinstein
URL:
Keywords:
Depends on: 38933
Blocks:
  Show dependency treegraph
 
Reported: 2010-05-11 12:55 PDT by Brian Weinstein
Modified: 2010-05-11 20:42 PDT (History)
6 users (show)

See Also:


Attachments
[PATCH] Fix (58.42 KB, patch)
2010-05-11 13:21 PDT, Brian Weinstein
no flags Details | Formatted Diff | Diff
Patch (58.50 KB, patch)
2010-05-11 13:41 PDT, Brian Weinstein
no flags Details | Formatted Diff | Diff
Patch (60.22 KB, patch)
2010-05-11 15:43 PDT, Brian Weinstein
no flags Details | Formatted Diff | Diff
Patch (60.19 KB, patch)
2010-05-11 15:55 PDT, Brian Weinstein
no flags Details | Formatted Diff | Diff
Patch (11.12 KB, patch)
2010-05-11 18:41 PDT, Brian Weinstein
mjs: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.