Bug 112599 - DFG should inline binary string concatenations (i.e. ValueAdd with string children)
Summary: DFG should inline binary string concatenations (i.e. ValueAdd with string chi...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords:
Depends on: 112663 112665 112697
Blocks: 112376
  Show dependency treegraph
 
Reported: 2013-03-18 11:21 PDT by Filip Pizlo
Modified: 2013-03-19 10:05 PDT (History)
11 users (show)

See Also:


Attachments
the patch (16.51 KB, patch)
2013-03-18 11:24 PDT, Filip Pizlo
oliver: review+
webkit-ews: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2013-03-18 11:21:05 PDT
And it should do so even if the children are StringObjects, or possibly StringObjects.  This should also lay the groundwork for making this fast:

"foo" + x

where x is an int.
Comment 1 Filip Pizlo 2013-03-18 11:24:16 PDT
Created attachment 193612 [details]
the patch
Comment 2 Early Warning System Bot 2013-03-18 11:36:25 PDT
Comment on attachment 193612 [details]
the patch

Attachment 193612 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17187464
Comment 3 Early Warning System Bot 2013-03-18 11:42:49 PDT
Comment on attachment 193612 [details]
the patch

Attachment 193612 [details] did not pass qt-wk2-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17160560
Comment 4 Oliver Hunt 2013-03-18 12:54:49 PDT
Comment on attachment 193612 [details]
the patch

r=me.

I've found myself pondering making us plant calls to the valueOf functions in the DFG, that way the function calls are cacheable and we could add intrinsics for the more common implementations, and custom calls would "just work"

That said i'm not sure how much of a win that would be for concat, etc now that the common case of String objects is handled.  But I do recall toString being sufficiently common on generic objects to be something we might consider investigating in future.

Other than that, make the qt bots non-red (!PLATFORM(QT) ?) and r=me
Comment 5 EFL EWS Bot 2013-03-18 13:28:04 PDT
Comment on attachment 193612 [details]
the patch

Attachment 193612 [details] did not pass efl-ews (efl):
Output: http://webkit-commit-queue.appspot.com/results/17235080
Comment 6 Build Bot 2013-03-18 14:12:13 PDT
Comment on attachment 193612 [details]
the patch

Attachment 193612 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17080771
Comment 7 Filip Pizlo 2013-03-18 17:46:35 PDT
(In reply to comment #4)
> (From update of attachment 193612 [details])
> r=me.
> 
> I've found myself pondering making us plant calls to the valueOf functions in the DFG, that way the function calls are cacheable and we could add intrinsics for the more common implementations, and custom calls would "just work"

Can't do that.  Because you have to do the weirdo thing where you either look up valueOf, or toString if the former finds nothing.  And then convert the result to a string.  Or not a string.  And then either do a numeric addition or a string concatenation.

So, I mean, you could plant all of that.  But you'll be planting a national forest.

> 
> That said i'm not sure how much of a win that would be for concat, etc now that the common case of String objects is handled.  But I do recall toString being sufficiently common on generic objects to be something we might consider investigating in future.
> 
> Other than that, make the qt bots non-red (!PLATFORM(QT) ?) and r=me
Comment 8 Filip Pizlo 2013-03-18 17:47:30 PDT
Landed with 32-bit fixes in http://trac.webkit.org/changeset/146164
Comment 9 Csaba Osztrogonác 2013-03-19 10:05:27 PDT
(In reply to comment #8)
> Landed with 32-bit fixes in http://trac.webkit.org/changeset/146164

It made all inspector tests timeout on ARM traditional platform:
https://bugs.webkit.org/show_bug.cgi?id=112697