The default bug view has changed. See this FAQ.

IonMonkey: Concatenating String objects is slow

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

(Blocks: 2 bugs)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
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

5 years ago
What is the callpath that asks for the current script/pc?
(Assignee)

Comment 2

5 years ago
(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

5 years ago
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)

Updated

5 years ago
Assignee: general → jdemooij
Status: NEW → ASSIGNED
(Assignee)

Updated

5 years ago
Blocks: 777561
(Assignee)

Comment 4

5 years ago
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.
Attachment #646513 - Flags: review?(dvander)
Attachment #646513 - Flags: review?(dvander) → review+
http://hg.mozilla.org/projects/ionmonkey/rev/ee40f69169e9
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.