Bug 150034 - "A + B" with strings shouldn't copy if A or B is empty.
Summary: "A + B" with strings shouldn't copy if A or B is empty.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andreas Kling
URL:
Keywords: Performance
Depends on:
Blocks:
 
Reported: 2015-10-12 10:45 PDT by Andreas Kling
Modified: 2015-10-14 09:23 PDT (History)
5 users (show)

See Also:

Attachments
Patch (26.64 KB, patch)
2015-10-12 10:50 PDT, Andreas Kling 		no flags Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Kling 2015-10-12 10:45:59 PDT 
Doing A + B where A and B are string types will currently always allocate a new StringImpl C with length A.length+B.length, and then blit the contents of A and B into C.
We should have a fast path for A + B where A.length == 0 || B.length == 0.
Comment 1 Andreas Kling 2015-10-12 10:50:45 PDT 
Created attachment 262899 [details]
Patch
Comment 2 WebKit Commit Bot 2015-10-12 12:45:27 PDT 
Comment on attachment 262899 [details]
Patch

Clearing flags on attachment: 262899

Committed r190882: <http://trac.webkit.org/changeset/190882>
Comment 3 WebKit Commit Bot 2015-10-12 12:45:30 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 4 Keith Miller 2015-10-14 07:02:57 PDT 
It looks like this patch is breaking the jsc command line REPL. If you try:

DYLD_FRAMEWORK_PATH=Safari-Gala-Production-SafariTen-190882-76008.app/Contents/Frameworks ./Safari-Gala-Production-SafariTen-190882-76008.app/Contents/Resources/jsc
>>> foo = 1
Exception: ReferenceError: Can't find variable: f
Comment 5 Andreas Kling 2015-10-14 09:23:50 PDT 
(In reply to comment #4)
> It looks like this patch is breaking the jsc command line REPL. If you try:
> 
> DYLD_FRAMEWORK_PATH=Safari-Gala-Production-SafariTen-190882-76008.app/
> Contents/Frameworks
> ./Safari-Gala-Production-SafariTen-190882-76008.app/Contents/Resources/jsc
> >>> foo = 1
> Exception: ReferenceError: Can't find variable: f

Oh dang! I see what the bug is: StringTypeAdapter<const UChar*>::toString() always uses length=1 instead of the provided character count.

Crazy that no test caught this. Will fix shortly.