Last Comment Bug 776022 - IonMonkey: Concatenating String objects is slow
: IonMonkey: Concatenating String objects is slow
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Jan de Mooij [:jandem]
: general
Mentors:
Depends on:
Blocks: IonSpeed 777561
  Show dependency treegraph
 
Reported: 2012-07-20 10:15 PDT by Jan de Mooij [:jandem]
Modified: 2012-07-27 13:14 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (15.66 KB, patch)
2012-07-27 04:18 PDT, Jan de Mooij [:jandem]
dvander: review+
Details | Diff | Review

Description Jan de Mooij [:jandem] 2012-07-20 10:15:49 PDT
We're slower than JM on SS date-format-xparb, because the benchmark uses String objects a lot. If I replace "new String" in String.leftPad with just "String" --no-jm is as fast as --no-ion.

On the micro-benchmark below we have:

JM+TI : 1010 ms
Ion   : 3046 ms

A profile shows a lot of time under InlineFrameIterator, SnapshotReader, etc so it looks like the concatenation somewhere queries the current script/pc.

It seems like there are two solutions:

1) Add a cache to get the current script/pc.
2) Specialize String objects.

I think we should really try (1) first because there are other operations which need the current script/pc, and specializing all of them seems like it will be a lot of work. We can always do (2) too if needed.

function f() {
    var s = new String("f");
    var res = "";
    for (var i=0; i<10000000; i++) {
        res += s;
    }
    return res;
}
var t = new Date;
f();
print(new Date - t);
Comment 1 Luke Wagner [:luke] 2012-07-20 10:18:04 PDT
What is the callpath that asks for the current script/pc?
Comment 2 Jan de Mooij [:jandem] 2012-07-20 10:32:16 PDT
(In reply to Luke Wagner [:luke] from comment #1)
> What is the callpath that asks for the current script/pc?

MonitorString in AddOperation. Looking at AddOperation there seems to be an easy fix for this particular case: we only have to call MonitorString if both operands are not strings. In this case one operand is a string so we don't have to monitor the result.

But this will fail if both operands are String objects, it would be good to fix that as well.
Comment 3 Luke Wagner [:luke] 2012-07-20 11:12:14 PDT
Given that TI penalizes any time in the VM, I would guess the right fix is to somehow stay in jit code (or at least stay in C++ stub calls that don't need the general VM type-monitoring path).
Comment 4 Jan de Mooij [:jandem] 2012-07-27 04:18:09 PDT
Created attachment 646513 [details] [diff] [review]
Patch

I tried to use MMonitorTypes to do the monitoring in JIT code, but it turns out the dynamic monitoring for JSOP_ADD etc (MonitorString, MonitorOverflow, etc) is quite different from the Monitor(..) calls for JSOP_GETPROP etc and the patch got pretty hackish and complicated.

So instead I went with the simpler approach of passing the script/pc to the stub. The micro-benchmark in comment 0 is now about as fast as JM+TI and we can always win more by specializing more cases - that's going to be faster than the generic stubs anyway.

On date-format-xparb Ion is now about as fast as JM+TI, Ion+JM is still a bit slower but that's most likely bug 775818.
Comment 5 David Anderson [:dvander] 2012-07-27 13:14:22 PDT
http://hg.mozilla.org/projects/ionmonkey/rev/ee40f69169e9

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