1343857 - IonMonkey: Try to unbox objects that are created once by the interpreter in Ion

Status: RESOLVED INVALID

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

Description

[Sander Mathijs van Veen]

Attachment: try-unbox-object-in-ion.patch

Objects that are created once by the interpreter are currently not converted to an unboxed layout. A heuristic prevents instances of an object that are only created once from using the unboxed layout. The idea is that while running in the interpreter it is not a good idea to create an unboxed layout for every object after the first creation. It's not a good idea because creating a boxed version from an unboxed object generates worse code in the end.

Small test case:

$ cat testcase.js
"use strict";

function calculate(Mobj, Lobjs, LN, Mout, Lout) {
    while (1) {
        var tmp1 = Mobj[Lobjs | 0];
        var tmp2 = +tmp1.d0;
        var tmp3 = -0;

        var Lmul = tmp2 * tmp3;

        var tmp5 = Mout[Lout | 0];
        tmp5.d0 = +Lmul;

        LN = LN - 1 | 0;
        if ((LN | 0) === 0)
            break;
    }
}

function main() {
    var LN = 500000000 | 0;

    var objs = [{
        d0: -0,
    }];

    var out = [{
        d0: -0,
    }];

    while (1) {
        calculate(objs, 0 | 0, LN, out, 0 | 0);
        break;
    }
}

main();

Running the test case with LICM disabled (otherwise most of the inner loop is optimized out) on an optimized JS Shell gives the following assembly for the inner loop:

[LoadElementT]                    
movabsq    $0x7fffffffffff, %rdi  
andq       0x0(%rcx,%rdx,8), %rdi 
[LoadFixedSlotT]                  
cmpl       $0xfff88000, 0x24(%rdi)
jne        .Lfrom614              
xorpd      %xmm0, %xmm0           
cvtsi2sd   0x20(%rdi), %xmm0      
jmp        .Lfrom628              
.set .Llabel628, .                
.set .Lfrom614, .Llabel628        
movsd      0x20(%rdi), %xmm0      
.set .Llabel633, .                
.set .Lfrom628, .Llabel633        
[Double]                          
movsd      .Lfrom641(%rip), %xmm1 
[MoveGroup]                       
movapd     %xmm0, %xmm2           
[MathD:mul]                       
mulsd      %xmm1, %xmm0           
[Elements]                        
movq       0x18(%rbp), %rcx       
[InitializedLength]               
movl       -0xc(%rcx), %edi       
[BoundsCheck]                     
cmpl       %esi, %edi             
jbe        .Lfrom664              
[LoadElementT]                    
movabsq    $0x7fffffffffff, %rdi  
andq       0x0(%rcx,%rsi,8), %rdi 
[StoreFixedSlotT]                 
movsd      %xmm0, 0x20(%rdi)      

When the heuristic "objectCount" is disabled by the given patch, the assembly code becomes:

[LoadElementT]
movabsq    $0x7fffffffffff, %rdi
andq       0x0(%rcx,%rdx,8), %rdi
[LoadUnboxedScalar]
movsd      0x10(%rdi), %xmm0
[Double]
movsd      .Lfrom614(%rip), %xmm1
[MoveGroup]
movapd     %xmm0, %xmm2
[MathD:mul]
mulsd      %xmm1, %xmm0
[Elements]
movq       0x18(%rbp), %rcx
[InitializedLength]
movl       -0xc(%rcx), %edi
[BoundsCheck]
cmpl       %esi, %edi
jbe        .Lfrom637
[LoadElementT]
movabsq    $0x7fffffffffff, %rdi
andq       0x0(%rcx,%rsi,8), %rdi
[StoreUnboxedScalar]
movsd      %xmm0, 0x10(%rdi)

The patch creates an unboxed layout for the object and can therefore loads/stores the scalar using LoadUnboxedScalar/StoreUnboxedScalar.

PreliminaryObjectArrayWithTemplate::maybeAnalyze(JSContext* cx, ObjectGroup* group, bool force) calls TryConvertToUnboxedLayout but does not propagate its force variable. Note that force is true when maybeAnalyse is called by:

1. js::MaybeAnalyzeBeforeCreatingLargeArray
2. js::jit::EnsureArrayGroupAnalyzed
3. js::jit::IonCompile
4. GiveObjectGroup

The callers 1, 2, and 4 will only set "force" to true for array objects, and the heuristic does not apply for arrays. Caller 3 (IonCompile) will now convert objects that are boxed to unboxed, since it passes "force" through with this patch. Objects created in the interpreter or baseline will not be converted to an unboxed layout, while Ion will try to unbox the objects (if there is only one instance) to generate better code.

Real-world benchmark (Emscripten's "skinning" compiled by Cheerp):

$ grep name /proc/cpuinfo | head -n1
model name      : Intel(R) Core(TM) i7-6700 CPU @ 3.40GHz

# Unpatched SpiderMonkey (Feb 28th, 2017).
$ time ~/work/sm-js /tmp/emscripten_temp/sm-cheerp_skinning.cpp.js
11.06user 0.00system 0:11.05elapsed 100%CPU (0avgtext+0avgdata 25536maxresident)k

# Applied patch that disables objectCount for IonCompile
$ time ~/work/sm-js /tmp/emscripten_temp/sm-cheerp_skinning.cpp.js
8.71user 0.01system 0:08.71elapsed 100%CPU (0avgtext+0avgdata 25648maxresident)k

Comment 1

[Jan de Mooij [:jandem]]

Do you see any difference on Sunspider, Octane, Kraken?

I'm unsure about this patch. Unboxed objects can introduce extra overhead or perf issues, so when we're creating a single object (instead of a lot of 'similar' ones) I'd prefer to be conservative and not unbox. Overall unboxing too eagerly can cause worse performance issues than not unboxing.

Comment 2

[Sander Mathijs van Veen]

Attachment: try-unbox-object-in-ion.patch

I did not see performance regressions on SunSpider or Kraken-1.1.

I did see some differences on Octane: the total score improves from 35923 to 36342 (median of 20 runs).

The following Octane benchmarks got faster (median on 20 runs, before patch -> after patch):
- delta blue: 72389 -> 72941 (1%).
- ray tracer: 129138 -> 129868 (1%).
- mandreel latency: 38055 -> 42811 (12%).
- zlib: 93237 -> 95450 (2%).

I saw one insignificant regressions in Octane:
- gameboy: 67707 -> 67640 (0%).

Comment 3

[Jan de Mooij [:jandem]]

Comment on attachment 8843959 [details] [diff] [review]
try-unbox-object-in-ion.patch

Review of attachment 8843959 [details] [diff] [review]:
-----------------------------------------------------------------

I'm sorry but I still think we shouldn't do this. Certain operations on unboxed objects are slower (for instance adding a new property requires allocating a separate expando object), so we have to be more careful when/where we use unboxed objects. These benchmarks are probably not adding new properties to the unboxed objects, but I'm sure other code on the web is doing that and we would have a new performance cliff to deal with.

Comment 4

[André Bargull [:anba]]

No longer valid because unboxed objects have been removed.