WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
60700
Replace direct StringConcatenate usage, by using operator+ (again)
https://bugs.webkit.org/show_bug.cgi?id=60700
Summary
Replace direct StringConcatenate usage, by using operator+ (again)
Nikolas Zimmermann
Reported
2011-05-12 08:00:35 PDT
Now that operator+ is fast for multiple string concats, deploy its usage everywhere. Previously I switched several places to using makeString() directly, revert that.
Attachments
Patch
(72.80 KB, patch)
2011-05-12 08:41 PDT
,
Nikolas Zimmermann
darin
: review-
webkit.review.bot
: commit-queue-
Details
Formatted Diff
Diff
Patch v2
(66.97 KB, patch)
2011-05-13 00:13 PDT
,
Nikolas Zimmermann
webkit.review.bot
: commit-queue-
Details
Formatted Diff
Diff
Archive of layout-test-results from ec2-cr-linux-01
(1.24 MB, application/zip)
2011-05-13 00:38 PDT
,
WebKit Review Bot
no flags
Details
Patch v3
(67.01 KB, patch)
2011-05-14 03:54 PDT
,
Nikolas Zimmermann
krit
: review+
krit
: commit-queue-
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Nikolas Zimmermann
Comment 1
2011-05-12 08:41:44 PDT
Created
attachment 93290
[details]
Patch With that patch, there's only one direct user of StringConcatenate.h remaining in WebCore: history/PageCache.cpp:#define PCLOG(...) pageCacheLog(pageCacheLogPrefix(indentLevel), makeString(__VA_ARGS__)) Everything else uses the new String append operator.
WebKit Review Bot
Comment 2
2011-05-12 08:50:30 PDT
Comment on
attachment 93290
[details]
Patch
Attachment 93290
[details]
did not pass chromium-ews (chromium-xvfb): Output:
http://queues.webkit.org/results/8690659
Early Warning System Bot
Comment 3
2011-05-12 08:51:44 PDT
Comment on
attachment 93290
[details]
Patch
Attachment 93290
[details]
did not pass qt-ews (qt): Output:
http://queues.webkit.org/results/8662011
Darin Adler
Comment 4
2011-05-12 09:39:44 PDT
Comment on
attachment 93290
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=93290&action=review
Generally looks good. I suggest landing a patch that covers the simpler obviously-correct cases first, and then we can slowly consider the ones that require a little more thought.
> Source/WebCore/bindings/js/JSCSSStyleDeclarationCustom.cpp:120 > - builder.append(makeString('-', toASCIILower(c))); > + builder.append("-" + toASCIILower(c));
Two problems here. First, this code won’t work. It will take the address of the "-" string, and then move forward by a number of characters equal the the lowercase character code. Pointer arithmetic. Second of all, if it did work, we’d be trading an optimized code path to append two character to a general purpose code path that has to call strlen just to get the length of a string literal.
> Source/WebCore/bindings/v8/custom/V8CSSStyleDeclarationCustom.cpp:139 > - builder.append(makeString('-', toASCIILower(c))); > + builder.append("-" + toASCIILower(c));
Same problem as above.
> Source/WebCore/inspector/InspectorProfilerAgent.cpp:105 > - String message = makeString("Profile \"webkit-profile://", CPUProfileType, '/', encodeWithURLEscapeSequences(title), '#', String::number(profile->uid()), "\" finished."); > + String message = String("Profile \"webkit-profile://") + CPUProfileType + "/" + encodeWithURLEscapeSequences(title) + '#' + String::number(profile->uid()) + "\" finished.";
The new code wastefully converts the first literal into a String just so we can concatenate. This is less efficient than the old code. Changing the '/' into a "/", adds an unneeded call to strlen into the mix.
> Source/WebCore/inspector/InspectorProfilerAgent.cpp:113 > - String message = makeString("Profile \"webkit-profile://", CPUProfileType, '/', encodeWithURLEscapeSequences(title), "#0\" started."); > + String message = String("Profile \"webkit-profile://") + CPUProfileType + "/" + encodeWithURLEscapeSequences(title) + "#0\" started.";
Same problem of allocating an extra string. We don’t want to convert things to String just so we can concatenate them and make the final result into a string. We make sure we preserve a way of doing it that does not require extra memory allocation.
> Source/WebCore/loader/cache/CachedResourceLoader.cpp:496 > + String message; > + if (m_document->url().isNull()) > + message = "Unsafe attempt to load URL " + url.string() + '.'; > + else > + message = "Unsafe attempt to load URL " + url.string() + " from frame with URL " + m_document->url().string() + ". Domains, protocols and ports must match.\n";
The ? : avoids creating an empty object and then using the assignment operator, which is less efficient than just initializing. Was it necessary to change from ? : to if?
Gyuyoung Kim
Comment 5
2011-05-12 18:00:05 PDT
Comment on
attachment 93290
[details]
Patch
Attachment 93290
[details]
did not pass efl-ews (efl): Output:
http://queues.webkit.org/results/8685928
Nikolas Zimmermann
Comment 6
2011-05-13 00:07:36 PDT
(In reply to
comment #4
)
> (From update of
attachment 93290
[details]
) > View in context:
https://bugs.webkit.org/attachment.cgi?id=93290&action=review
> > Generally looks good. I suggest landing a patch that covers the simpler obviously-correct cases first, and then we can slowly consider the ones that require a little more thought.
You are right.
> > > Source/WebCore/bindings/js/JSCSSStyleDeclarationCustom.cpp:120 > First, this code won’t work. It will take the address of the "-" string, and then move forward by a number of characters equal the the lowercase character code. Pointer arithmetic.
Ay ay ay, great catch.
> > Source/WebCore/bindings/v8/custom/V8CSSStyleDeclarationCustom.cpp:139
I'll leave out both files from the next patch.
> > Source/WebCore/inspector/InspectorProfilerAgent.cpp:105 > The new code wastefully converts the first literal into a String just so we can concatenate. This is less efficient than the old code.
Right, I left it out for now.
> > > Source/WebCore/inspector/InspectorProfilerAgent.cpp:113
Ditto.
> > Source/WebCore/loader/cache/CachedResourceLoader.cpp:496 > > + String message; > > + if (m_document->url().isNull()) > > + message = "Unsafe attempt to load URL " + url.string() + '.'; > > + else > > + message = "Unsafe attempt to load URL " + url.string() + " from frame with URL " + m_document->url().string() + ". Domains, protocols and ports must match.\n"; > > The ? : avoids creating an empty object and then using the assignment operator, which is less efficient than just initializing. Was it necessary to change from ? : to if?
If I want to use the ? operator, I have to assure both sides are Strings, so I have to use String("Unsafe...") + url.string()... Compared to that, I think the new solution is better, no?
Nikolas Zimmermann
Comment 7
2011-05-13 00:13:15 PDT
Created
attachment 93411
[details]
Patch v2
WebKit Review Bot
Comment 8
2011-05-13 00:38:41 PDT
Comment on
attachment 93411
[details]
Patch v2
Attachment 93411
[details]
did not pass chromium-ews (chromium-xvfb): Output:
http://queues.webkit.org/results/8690936
New failing tests: svg/dom/SVGTransformList-basics.xhtml
WebKit Review Bot
Comment 9
2011-05-13 00:38:46 PDT
Created
attachment 93414
[details]
Archive of layout-test-results from ec2-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-01 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Nikolas Zimmermann
Comment 10
2011-05-14 03:54:23 PDT
Created
attachment 93552
[details]
Patch v3 Fix typo in patch v2, leading to cr-linux tests problem: + case SVG_TRANSFORM_SCALE: { - DEFINE_STATIC_LOCAL(String, scaleString, ("matrix(")); + DEFINE_STATIC_LOCAL(String, scaleString, ("scale("));
Dirk Schulze
Comment 11
2011-05-14 11:07:30 PDT
Comment on
attachment 93552
[details]
Patch v3 View in context:
https://bugs.webkit.org/attachment.cgi?id=93552&action=review
r=me with some notes.
> Source/WebCore/bindings/js/ScriptDebugServer.cpp:83 > + return sourceID + ":" + String::number(scriptBreakpoint.lineNumber);
not ':'?
> Source/WebCore/page/DOMWindow.cpp:1702 > + " from frame with URL " + activeWindowURL.string() + ". Domains, protocols and ports must match.\n";
don't we use indention with 4 spaces here? I like your style more, but we should be consistent.
> Source/WebCore/platform/efl/PlatformKeyboardEventEfl.cpp:56 > + String key = "F" + String::number(i);
'F' instead of "F"?
> Source/WebCore/platform/efl/PlatformKeyboardEventEfl.cpp:164 > + String key = "F" + String::number(i);
Ditto.
Nikolas Zimmermann
Comment 12
2011-05-15 14:23:16 PDT
(In reply to
comment #11
)
> (From update of
attachment 93552
[details]
) > View in context:
https://bugs.webkit.org/attachment.cgi?id=93552&action=review
> > r=me with some notes. > > > Source/WebCore/bindings/js/ScriptDebugServer.cpp:83 > > + return sourceID + ":" + String::number(scriptBreakpoint.lineNumber); > > not ':'? > > > Source/WebCore/page/DOMWindow.cpp:1702 > > + " from frame with URL " + activeWindowURL.string() + ". Domains, protocols and ports must match.\n"; > > don't we use indention with 4 spaces here? I like your style more, but we should be consistent.
Fixed.
> > > Source/WebCore/platform/efl/PlatformKeyboardEventEfl.cpp:56 > > + String key = "F" + String::number(i); > > 'F' instead of "F"? > > > Source/WebCore/platform/efl/PlatformKeyboardEventEfl.cpp:164 > > + String key = "F" + String::number(i); > > Ditto.
That doesn't build see efl build output from patch v1.
Nikolas Zimmermann
Comment 13
2011-05-16 01:00:37 PDT
Landed in
r86542
and
r86543
(forgot one file).
Joseph Pecoraro
Comment 14
2011-09-28 11:43:19 PDT
This introduced a subtle issue, filed at: <
http://webkit.org/b/69011
> FTPDirectoryDocument Shows Garbled String for Last Modified Date
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug