Closed Bug 776022 Opened 12 years ago Closed 12 years ago

IonMonkey: Concatenating String objects is slow

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: jandem, Assigned: jandem)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file)

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);
What is the callpath that asks for the current script/pc?
(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.
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).
Assignee: general → jdemooij
Status: NEW → ASSIGNED
Blocks: 777561
Attached patch PatchSplinter Review
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.
Attachment #646513 - Flags: review?(dvander)
Attachment #646513 - Flags: review?(dvander) → review+
http://hg.mozilla.org/projects/ionmonkey/rev/ee40f69169e9
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.