Bug 21203 - Optimize appending a number to a string
Summary: Optimize appending a number to a string
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Maciej Stachowiak
Depends on:
Reported: 2008-09-28 19:56 PDT by Maciej Stachowiak
Modified: 2008-09-29 18:31 PDT (History)
0 users

See Also:

patch to do it. Some sad code duplication here. (20.34 KB, patch)
2008-09-28 19:57 PDT, Maciej Stachowiak
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Maciej Stachowiak 2008-09-28 19:56:22 PDT
Optimize appending a number to a string. It's pretty common in real-world code (and on some of the v8 benchmarks) to do this.
Comment 1 Maciej Stachowiak 2008-09-28 19:57:18 PDT
Created attachment 23900 [details]
patch to do it. Some sad code duplication here.
Comment 2 Maciej Stachowiak 2008-09-28 20:01:01 PDT
I sadly duplicated the logic for UString::from(int) and UString::from(double)
though fortunately not the actual append logic. Perhaps I should try harder to
refactor to make it ot duplicat, though it was not 100% clear how to do this
except maybe using a template. UString::from(int) is not much code but the
double case is nontrivial.
Comment 3 Darin Adler 2008-09-28 20:12:20 PDT
Comment on attachment 23900 [details]
patch to do it. Some sad code duplication here.

+    if ((leftIsString = v1->isString()) && v2->isString()) {

Why not just put the leftIsString boolean initialization separate in front of the if statement in a more conventional way?

+        RefPtr<UString::Rep> value;
+        if (JSImmediate::isImmediate(v2))
+            value = concatenate(static_cast<JSString*>(v1)->value().rep(), JSImmediate::getTruncatedInt32(v2));
+        else
+            value = concatenate(static_cast<JSString*>(v1)->value().rep(), right);

I believe would be more efficient if done with the ternary operator instead of an if statement. I believe it would save a null check and a branch over deref code on the assignment to the value local.

+inline size_t expandedSize(size_t size, size_t otherSize)

If this isn't going to be a static member function, then I suggest internal linkage -- add the static keyword.

+inline bool expandCapacity(UString::Rep* rep, int requiredLength)

This should also be marked static to get internal linkage.

Comment 4 Maciej Stachowiak 2008-09-29 18:31:50 PDT
Landed as r37089