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+

Brian Weinstein

Reported 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.

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
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Brian Weinstein

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

Created attachment 55743 [details] [PATCH] Fix

WebKit Review Bot

Comment 2 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.

Brian Weinstein

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

Created attachment 55748 [details] Patch

WebKit Review Bot

Comment 4 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.

Eric Seidel (no email)

Comment 5 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.

WebKit Review Bot

Comment 6 2010-05-11 14:06:56 PDT

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

Brian Weinstein

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

Created attachment 55766 [details] Patch

Eric Seidel (no email)

Comment 8 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

WebKit Review Bot

Comment 9 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.

Eric Seidel (no email)

Comment 10 2010-05-11 15:53:45 PDT

Looks like a bug in check-webkit-style.

Brian Weinstein

Comment 11 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

Brian Weinstein

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

Created attachment 55770 [details] Patch

WebKit Review Bot

Comment 13 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.

Jon Honeycutt

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

Comment on attachment 55770 [details] Patch r=me

Brian Weinstein

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

Created attachment 55793 [details] Patch

Maciej Stachowiak

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

Comment on attachment 55793 [details] Patch r=me

Brian Weinstein

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

Landed in r59187 and r59196.

Eric Seidel (no email)

Comment 18 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.

Note You need to log in before you can comment on or make changes to this bug.