RFR: 8305959: x86: Improve itable_stub [v2]

[Aleksey Shipilev]

On Fri, 26 May 2023 08:10:09 GMT, Boris Ulasevich <bulasevich at openjdk.org> wrote:

>> Async profiler shows that applications spend up to 10% in itable_stubs.
>> 
>> The current inefficiency of itable stubs is as follows. The generated itable_stub scans itable twice: first it checks if the object class is a subtype of the resolved_class, and then it finds the holder_class that implements the method. I suggest doing this in one pass: with a first loop over itable, check pointer equality to both holder_class and resolved_class. Once we have finished searching for resolved_class, continue searching for holder_class in a separate loop if it has not yet been found.
>> 
>> This approach gives 1-10% improvement on the synthetic benchmarks and 3% improvement on Naive Bayes benchmark from the Renaissance Benchmark Suite (Intel Xeon X5675).
>
> Boris Ulasevich has refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. The pull request contains three new commits since the last revision:
> 
>  - readability rework
>  - cleanup
>  - 8305959: x86: Improve itable_stub

Nice. I have only minor comments left.

src/hotspot/cpu/x86/vtableStubs_x86_32.cpp line 203:

> 201: 
> 202:   start_pc = __ pc();
> 203:   __ push(temp_reg);

Why do we need to save this one? Do we care if this "tmp" is clobbered?

src/hotspot/cpu/x86/vtableStubs_x86_32.cpp line 204:

> 202:   start_pc = __ pc();
> 203:   __ push(temp_reg);
> 204:   __ lookup_interface_method_stub(recv_klass_reg,

Let's not lose the original comments here. `// Receiver subtype check against REFC` and others.

src/hotspot/cpu/x86/vtableStubs_x86_64.cpp line 197:

> 195:   start_pc = __ pc();
> 196: 
> 197:   __ lookup_interface_method_stub(recv_klass_reg,

Same, let's not lose the original comments here: `// Receiver subtype check against REFC` and others.

test/micro/org/openjdk/bench/vm/compiler/InterfaceCalls.java line 62:

> 60:     interface FirstInterfaceExtExt extends FirstInterfaceExt {
> 61:         default int getIntFirst() {return 45;}
> 62:     }

Style:

Suggestion:

    interface FirstInterfaceExt extends FirstInterface {
        default int getIntFirst() { return 44; }
    }

    interface FirstInterfaceExtExt extends FirstInterfaceExt {
        default int getIntFirst() { return 45; }
    }

test/micro/org/openjdk/bench/vm/compiler/InterfaceCalls.java line 187:

> 185:     public FirstInterface[] as = new FirstInterface[asLength];
> 186:     public FirstInterface[] noninlined = new FirstInterface[5];
> 187:     public FirstInterfaceExtExt[] noninlinedextext = new FirstInterfaceExtExt[5];

Suggestion:

    public FirstInterface[] noninlined = new FirstInterface[asLength];
    public FirstInterfaceExtExt[] noninlinedextext = new FirstInterfaceExtExt[asLength];

test/micro/org/openjdk/bench/vm/compiler/InterfaceCalls.java line 203:

> 201:         noninlined[3] = new FourthClassDontInline();
> 202:         noninlined[4] = new FifthClassDontInline();
> 203:         noninlinedextext[0] = new FirstClassDontInlineExtExt();

Suggestion:

        noninlined[4] = new FifthClassDontInline();

        noninlinedextext[0] = new FirstClassDontInlineExtExt();

test/micro/org/openjdk/bench/vm/compiler/InterfaceCalls.java line 221:

> 219:     /** Tests single base interface method call */
> 220:     @Benchmark
> 221:     public void testIfaceCall(Blackhole bh) {

Why these tests are not in the form of the other benchmarks? 


    @Benchmark
    public int test1stInt5Types() {
        FirstInterface ai = as[l];
        l = ++ l % asLength;
        return ai.getIntFirst();
    }


I suspect those are carefully written so that a single call-site would be used for all receivers, thus limiting the profile-guided optimizations.

-------------

PR Review: https://git.openjdk.org/jdk/pull/13460#pullrequestreview-1455266666
PR Review Comment: https://git.openjdk.org/jdk/pull/13460#discussion_r1213030376
PR Review Comment: https://git.openjdk.org/jdk/pull/13460#discussion_r1213027973
PR Review Comment: https://git.openjdk.org/jdk/pull/13460#discussion_r1213029621
PR Review Comment: https://git.openjdk.org/jdk/pull/13460#discussion_r1213031451
PR Review Comment: https://git.openjdk.org/jdk/pull/13460#discussion_r1213032105
PR Review Comment: https://git.openjdk.org/jdk/pull/13460#discussion_r1213032310
PR Review Comment: https://git.openjdk.org/jdk/pull/13460#discussion_r1213036770