1843775 - Dyn. memory latency ~3 times slower than other browsers for sizes < to the L1 cache according to a JavaScript test

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P3)

Description

[David]

Attachment: Memory latency browsers comparison results (JS).csv

Steps to reproduce:

Run the test at https://chipsandcheese.com/memory-latency-test/ on latest FF.
Compare the results with chromium or webkit-based browsers.

Actual results:

On a Apple M2, on macOS & Linux, chunks < 128 kB take ~3 times the ones of chrome and webkit.
The results included are on Ventura and were similar on Asahi Linux (~3,96).

Expected results:

They should be close to the ones reported by these other browsers and native code outside of a browser engine (which I have failed to run on my environment for the latter).

The issue is prominent until 128 kB, which is the size of the L1 data cache for the performance cores, which are used for direct user-interacting apps on macOS.
On Linux the performance cores run at their max freq. (3,504 GHz) for FF and chromium, I suspect it is similar on macOS, for the same reason (priorities user-interacting apps).
I could not get / test L1/2 caches latency numbers for the M2, but for the M1 according to a source, it takes 3 cycles and 11 respectively. If they are similar (and correct), it should takes 3.504 GHz / 3 cycles ~ 0.86 ns for the L1 (which is near chrome & safari results) at 1.2 ns and 3.504 / 11 ~ 3.14 ns for the L2, also near the results of Firefox at ~ 3.7.

It looks like FF does not uses the L1 cache at least in this JS test*.
If this due to a bug in moz-jemalloc (i don't see why SpiderMonkey would manage low level memory allocations), this is very worthy of investigating as it may be slowing other things down.
Similar discrepancies have been found by the author(s) of the test and by users of a forum.

*which I find bizarre for a widely distributed software

Comment 1

[BugBot [:suhaib / :marco]]

The Bugbug bot thinks this bug should belong to the 'Core::JavaScript Engine' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 2

[David]

"Memory allocator" component is more specific (even though moz-jemalloc might not be the culprit).

Comment 3

[David]

Attachment: Latency results

fix + legend

Comment 4

[André Bargull [:anba]]

The test case is measuring boxing overhead after sum exceeds INT32_MAX and is converted to a Double.

Generated assembly for the loop body:

[Codegen] --------------------------------
[Codegen] # block3 /tmp/g.js:13:20 (loop header):
[Codegen] .set .Llabel150, .
[Codegen]                                 # LIR=InterruptCheck
[Codegen] movabsq    $0x7f05c4c2419c, %r11
[Codegen] cmpl       $0x0, 0x0(%r11)
[Codegen] jne        .Lfrom170
[Codegen] .set .Llabel170, .
[Codegen]                                 # LIR=OsiPoint
[Codegen] .set .Llabel170, .
[Codegen] .set .Llabel170, .
[Codegen]                                 # LIR=Unbox:Int32
[Codegen] movq       %r8, %r11
[Codegen] shrq       $47, %r11
[Codegen] cmpl       $0x1fff1, %r11d
[Codegen] jne        .Lfrom190
[Codegen] movl       %r8d, %r10d
[Codegen]                                 # LIR=CompareAndBranch:Lt
[Codegen] cmpl       %edx, %r10d
[Codegen] jge        .Lfrom202
[Codegen] --------------------------------
[Codegen] # block4 /tmp/g.js:13:20:
[Codegen] .set .Llabel202, .
[Codegen]                                 # LIR=Unbox:Int32
[Codegen] movq       %rbx, %r11
[Codegen] shrq       $47, %r11
[Codegen] cmpl       $0x1fff1, %r11d
[Codegen] jne        .Lfrom222
[Codegen] movl       %ebx, %r12d
[Codegen]                                 # LIR=Int32ToIntPtr
[Codegen] movslq     %r12d, %r12
[Codegen]                                 # LIR=BoundsCheck
[Codegen] cmpq       %r12, %rdi
[Codegen] jbe        .Lfrom237
[Codegen]                                 # LIR=SpectreMaskIndex
[Codegen] xorl       %r13d, %r13d
[Codegen] cmpq       %rdi, %r12
[Codegen] cmovb      %r12, %r13
[Codegen]                                 # LIR=LoadUnboxedScalar
[Codegen] movl       0x0(%r9,%r13,4), %r12d
[Codegen] testl      %r12d, %r12d
[Codegen] js         .Lfrom260
[Codegen]                                 # LIR=KeepAliveObject
[Codegen]                                 # LIR=UnboxFloatingPoint:Double
[Codegen] movq       %rcx, %r11
[Codegen] shrq       $47, %r11
[Codegen] cmpl       $0x1fff0, %r11d
[Codegen] ja         .Lfrom280
[Codegen] vmovq      %rcx, %xmm0
[Codegen] .set .Llabel285, .
[Codegen]                                 # LIR=Int32ToDouble
[Codegen] xorpd      %xmm1, %xmm1
[Codegen] cvtsi2sd   %r12d, %xmm1
[Codegen]                                 # LIR=MathD:Add
[Codegen] addsd      %xmm1, %xmm0
[Codegen]                                 # LIR=AddI
[Codegen] addl       $1, %r10d
[Codegen]                                 # LIR=Box:Int32
[Codegen] movabsq    $0xfff8800000000000, %rbx
[Codegen] orq        %r12, %rbx
[Codegen]                                 # LIR=Box:Double
[Codegen] vmovq      %xmm0, %rcx
[Codegen] movabsq    $0xfff80000ffffffff, %r11
[Codegen] cmpq       %rcx, %r11
[Codegen] cmovb      %r11, %rcx
[Codegen]                                 # LIR=Box:Int32
[Codegen] movabsq    $0xfff8800000000000, %r8
[Codegen] orq        %r10, %r8
[Codegen]                                 # LIR=Goto
[Codegen] jmp        .Llabel150
[Codegen] --------------------------------

Changing sum += current; to sum = (sum + current)|0; to force Int32 variables removes the boxing overhead:

[Codegen] --------------------------------
[Codegen] # block3 /tmp/g.js:13:20 (loop header):
[Codegen] .set .Llabel216, .
[Codegen]                                 # LIR=InterruptCheck
[Codegen] movabsq    $0x7f7435220a9c, %r11
[Codegen] cmpl       $0x0, 0x0(%r11)
[Codegen] jne        .Lfrom236
[Codegen] .set .Llabel236, .
[Codegen]                                 # LIR=OsiPoint
[Codegen] .set .Llabel236, .
[Codegen] .set .Llabel236, .
[Codegen]                                 # LIR=CompareAndBranch:Lt
[Codegen] cmpl       %esi, %edx
[Codegen] jge        .Lfrom244
[Codegen] --------------------------------
[Codegen] # block4 /tmp/g.js:13:20:
[Codegen] .set .Llabel244, .
[Codegen]                                 # LIR=Int32ToIntPtr
[Codegen] movslq     %ebx, %r10
[Codegen]                                 # LIR=BoundsCheck
[Codegen] cmpq       %r10, %r8
[Codegen] jbe        .Lfrom256
[Codegen]                                 # LIR=SpectreMaskIndex
[Codegen] xorl       %r12d, %r12d
[Codegen] cmpq       %r8, %r10
[Codegen] cmovb      %r10, %r12
[Codegen]                                 # LIR=LoadUnboxedScalar
[Codegen] movl       0x0(%r9,%r12,4), %r10d
[Codegen] testl      %r10d, %r10d
[Codegen] js         .Lfrom279
[Codegen]                                 # LIR=KeepAliveObject
[Codegen]                                 # LIR=AddI
[Codegen] addl       %r10d, %r13d
[Codegen]                                 # LIR=AddI
[Codegen] addl       $1, %edx
[Codegen]                                 # LIR=MoveGroup
[Codegen] movl       %r10d, %ebx
[Codegen]                                 # LIR=Goto
[Codegen] jmp        .Llabel216
[Codegen] --------------------------------

The remaining overhead are now Spectre mitigations, disabling them (--spectre-mitigations=off on the shell) results in:

[Codegen] --------------------------------
[Codegen] # block3 /tmp/g.js:13:20 (loop header):
[Codegen] .set .Llabel210, .
[Codegen]                                 # LIR=InterruptCheck
[Codegen] movabsq    $0x7f2a7ac2419c, %r11
[Codegen] cmpl       $0x0, 0x0(%r11)
[Codegen] jne        .Lfrom230
[Codegen] .set .Llabel230, .
[Codegen]                                 # LIR=OsiPoint
[Codegen] .set .Llabel230, .
[Codegen] .set .Llabel230, .
[Codegen]                                 # LIR=CompareAndBranch:Lt
[Codegen] cmpl       %esi, %edx
[Codegen] jge        .Lfrom238
[Codegen] --------------------------------
[Codegen] # block4 /tmp/g.js:13:20:
[Codegen] .set .Llabel238, .
[Codegen]                                 # LIR=Int32ToIntPtr
[Codegen] movslq     %ebx, %r12
[Codegen]                                 # LIR=BoundsCheck
[Codegen] cmpq       %r12, %r8
[Codegen] jbe        .Lfrom250
[Codegen]                                 # LIR=LoadUnboxedScalar
[Codegen] movl       0x0(%r9,%r12,4), %r10d
[Codegen] testl      %r10d, %r10d
[Codegen] js         .Lfrom263
[Codegen]                                 # LIR=KeepAliveObject
[Codegen]                                 # LIR=AddI
[Codegen] addl       %r10d, %r13d
[Codegen]                                 # LIR=AddI
[Codegen] addl       $1, %edx
[Codegen]                                 # LIR=MoveGroup
[Codegen] movl       %r10d, %ebx
[Codegen]                                 # LIR=Goto
[Codegen] jmp        .Llabel210
[Codegen] --------------------------------

Comment 5

[Nicolas B. Pierron [:nbp] {away}]

The script is located in the script tag id="worker" of (https://jsmemtest.chipsandcheese.com/) the index page, and runs in a Worker:

        function latencyTest(sizeKb, iterations) {
            var listSize = sizeKb * 1024 / 4;  // converting to uint32 = 4 bytes per element
            var testArr = [];
            if (sizeKb <= 512) iteraitons = iterations * 30;
            if (sizeKb > 4096) iterations = iterations / 10;
            if (sizeKb > 65536) iterations = iterations / 10;
            if (sizeKb > 131072) iterations = iterations / 30;
            for (var i = 0; i < listSize; i++) {
                testArr[i] = i;
            }

            var iter = listSize;
            while (iter > 1) {
                iter -= 1;
                var j = (iter - 1) == 0 ? 0 : Math.floor(Math.random() * (iter));
                var tmp = testArr[iter];
                testArr[iter] = testArr[j];
                testArr[j] = tmp;
            }

            var uintTestArr = new Uint32Array(testArr);

            var d = new Date();
            var start = d.getTime();
            var current = uintTestArr[0];
            var sum = 0;
            for (var i = 0; i < iterations; i++) {
                current = uintTestArr[current];
                sum += current;
            }

            var d1 = new Date();
            var end = d1.getTime();
            var elapsedTimeMs = end - start;
            var latency = 1000000.0 * elapsedTimeMs / iterations;

            return [latency, sum];
        }

The way the script is initialized, and the random nature without clipping to int32, implies that we would bailout and recompile to double after some limit.

Using 128 kB, with the roughly n**2 sum result, 128kB^2 is 4 times larger than what can be represented in a 32 bits number.
So the bailout and recompilation is unavoidable here.

Thus why would this be faster in other browsers, unless they were to compile the loop twice, once for integers and a second time for doubles, maybe?

Comment 6

[André Bargull [:anba]]

Thus why would this be faster in other browsers, unless they were to compile the loop twice, once for integers and a second time for doubles, maybe?

It's slow for us because of the extra boxing overhead. Initially we're compiling using Int32 numbers, but at some point sum += current exceeds the Int32 range and we bailout (with bailout kind TranspiledCacheIR). After the bailout to Baseline we're checking at the OSR entry if there's an IonScript. The IonScript is still present, so we're reentering Ion. We're back in the old IonScript, which is still compiled for Int32 numbers, and the MUnbox instructions for the OSR values try to unbox sum, which is now a Double, as an Int32. This fails and we now bailout with bailout kind SpeculativePhi. The bailout kind SpeculativePhi disables speculative Phi unboxing, so when the IonScript gets recompiled for Double values, we end up with additional MBox and MUnbox instructions everywhere.

This can be easily checked when running the script with IONFLAGS=bl-bails,bailouts. When changing the Jit options to directly recompile the IonScript after the first TranspiledCacheIR bailout (setJitCompilerOption("ion.frequent-bailout-threshold", 1)), there won't be OSRValue unbox failures and the script runs faster.

Comment 7

[Nicolas B. Pierron [:nbp] {away}]

(In reply to André Bargull [:anba] from comment #6)

[…], so when the IonScript gets recompiled for Double values, we end up with additional MBox and MUnbox instructions everywhere.

I do not understand, if we are doing Double math, we should not have any MBox / MUnbox at all, as double are already boxed by definition.

Iain, maybe there is something more hiding here.

Comment 8

[Iain Ireland [:iain]]

Here's a shorter testcase that exhibits the same behaviour:

var arr = [];
for (var i = 0; i < 32000; i++) {
    arr.push(i+1);
}
arr.push(0);

with ({}) {}
function foo() {
    var current = 0;
    var sum = 0;
    for (var i = 0; i < 10000; i++) {
	current = arr[current];
	sum += current * 1000;
    }
    return sum;
}
foo();

Sequence of events:

1. We OSR into the loop.
2. Eventually sum overflows int32. The addition bails out.
3. We perform the addition in baseline.
4. On the next iteration, we OSR back into the loop.
5. In the OSR entry block, we optimistically unbox sum to an Int32. Because it's now a Double, this fails and we bail out again.
6. We recompile with speculative phi unboxing disabled.
7. Because of this, the iteration counter (i) is also boxed. Incrementing that counter is now much slower.

We added SpeculativePhi bailouts to fix a performance cliff involving catch blocks (described in bug 1671635). Maybe there's a way we can be more precise here to avoid both cliffs. One option is to not disable speculative phi unboxing if we have already had fixable bailouts (eg the overflow in this case), under the assumption that recompiling after fixing those bailouts may avoid the speculative phi failure. It works in this case without regressing the original catch-block microbenchmark. I'll see how it performs on try.

Comment 9

[Nicolas B. Pierron [:nbp] {away}]

(In reply to Iain Ireland [:iain] from comment #8)

5. In the OSR entry block, we optimistically unbox sum to an Int32. Because it's now a Double, this fails and we bail out again.

Shouldn't we have captured the double type as part of the addition in the loop execution?

Comment 10

[Iain Ireland [:iain]]

(In reply to Nicolas B. Pierron [:nbp] from comment #9)

(In reply to Iain Ireland [:iain] from comment #8)

5. In the OSR entry block, we optimistically unbox sum to an Int32. Because it's now a Double, this fails and we bail out again.

Shouldn't we have captured the double type as part of the addition in the loop execution?

This is still the original version of the Ion code that was specialized for Int32, before we overflowed. We don't invalidate immediately after the first CacheIR bailout. Instead, we give it ten bailouts before we invalidate, hoping that it's a one-time thing. We've already attached a new baseline IC, but that won't be reflected in the Ion code until we recompile.

Comment 11

[Iain Ireland [:iain]]

Attachment: Bug 1843775: Disable speculative phis less aggressively r=jandem

If we have already bailed out for some fixable reason (eg TranspiledCacheIR), then there's a good chance that fixing that reason will also fix the speculative phi failure. Disabling speculative phis can be a big deal in cases where it causes us to box and unbox iteration variables in an OSR loop.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/18a8d6e37793
Disable speculative phis less aggressively r=jandem

Comment 13

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/18a8d6e37793

Comment 14

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/18a8d6e37793