Performance improvements for ImplicitObjectELResolver

[anuragdy]

Fixes 67080.

Tested the speed using following Test Code -

import java.util.Arrays;
import java.util.HashMap;

public class TestImplicitObjectELResolver {
    private static final String[] SCOPE_NAMES = new String[]{"applicationScope", "cookie", "header", "headerValues",
            "initParam", "pageContext", "pageScope", "param", "paramValues", "requestScope", "sessionScope"};

    private enum Scope {
        applicationScope(0), cookie(1), header(2), headerValues(3), initParam(4), pageContext(5), pageScope(6), param(7), paramValues(8),
        requestScope(9), sessionScope(10);

        final public int idx;

        private static final HashMap<String, Scope> lookupMap = new HashMap<>();

        static {
            lookupMap.put("applicationScope", applicationScope);
            lookupMap.put("cookie", cookie);
            lookupMap.put("header", header);
            lookupMap.put("headerValues", headerValues);
            lookupMap.put("initParam", initParam);
            lookupMap.put("pageContext", pageContext);
            lookupMap.put("pageScope", pageScope);
            lookupMap.put("param", param);
            lookupMap.put("paramValues", paramValues);
            lookupMap.put("requestScope", requestScope);
            lookupMap.put("sessionScope", sessionScope);
        }

        Scope(int idx) {
            this.idx = idx;
        }

        public static Scope fromValue(String value){
            return lookupMap.get(value);
        }

    }

    private static final String[] TEST_SCOPES = {"requestScope", "pageScope", "sessionScope", "pageContext", "header",
            "applicationScope", "cookie", "headerValues", "initParam", "param", "paramValues"};

    public static void main(String[] args) {
        new TestImplicitObjectELResolver().runTests();
    }

    private final int numTestIterations;

    private final int numTests;

    public TestImplicitObjectELResolver() {
        numTests = 10;
        numTestIterations = 10000000;
    }

    protected void runNewTest() {
        int foundScope = -1;
        for (int i = 0; i < TEST_SCOPES.length; i++) {
            String scope = TEST_SCOPES[i];
            switch (scope) {
                case "requestScope":
                    foundScope = 1;
                    break;
                case "pageScope":
                    foundScope = 2;
                    break;
                case "sessionScope":
                    foundScope = 3;
                    break;
                case "pageContext":
                    foundScope = 4;
                    break;
                case "header":
                    foundScope = 5;
                    break;
                case "applicationScope":
                    foundScope = 6;
                    break;
                case "cookie":
                    foundScope = 7;
                    break;
                case "headerValues":
                    foundScope = 8;
                    break;
                case "initParam":
                    foundScope = 9;
                    break;
                case "param":
                    foundScope = 10;
                    break;
                case "paramValues":
                    foundScope = 11;
                    break;
                default:
                    throw new RuntimeException("Did not find scope for value " + scope);
            }
            if (foundScope < 0) {
                throw new RuntimeException();
            }
        }
    }

    protected void runOldTest() {
        for (int i = 0; i < TEST_SCOPES.length; i++) {
            Arrays.binarySearch(SCOPE_NAMES, TEST_SCOPES[i]);
        }
    }

    protected void runSecondNewTest() {
        for (int i = 0; i < TEST_SCOPES.length; i++) {
            Scope.fromValue(TEST_SCOPES[i]);
        }
    }

    protected void runTests() {
        // warmup
        for (int i = 0; i < numTestIterations; i++) {
            runOldTest();
            runNewTest();
            runSecondNewTest();
        }

        for (int i = 0; i < numTests; i++) {
            System.gc();
            long startOld = System.currentTimeMillis();
            for (int j = 0; j < numTestIterations; j++) {
                runOldTest();
            }
            System.out.println("Done with old in " + (System.currentTimeMillis() - startOld));
        }

        for (int i = 0; i < numTests; i++) {
            System.gc();
            long startNew = System.currentTimeMillis();
            for (int j = 0; j < numTestIterations; j++) {
                runNewTest();
            }
            System.out.println("Done with new in " + (System.currentTimeMillis() - startNew));
        }

        for (int i = 0; i < numTests; i++) {
            System.gc();
            long startNew = System.currentTimeMillis();
            for (int j = 0; j < numTestIterations; j++) {
                runSecondNewTest();
            }
            System.out.println("Done with runSecondNewTest in " + (System.currentTimeMillis() - startNew));
        }

    }
}

Which produced following output

Done with old in 663
Done with old in 737
Done with old in 734
Done with old in 738
Done with old in 742
Done with old in 737
Done with old in 734
Done with old in 734
Done with old in 736
Done with old in 735
Done with new in 373
Done with new in 374
Done with new in 373
Done with new in 373
Done with new in 373
Done with new in 373
Done with new in 374
Done with new in 373
Done with new in 375
Done with new in 373
Done with runSecondNewTest in 170
Done with runSecondNewTest in 170
Done with runSecondNewTest in 169
Done with runSecondNewTest in 169
Done with runSecondNewTest in 169
Done with runSecondNewTest in 173
Done with runSecondNewTest in 171
Done with runSecondNewTest in 172
Done with runSecondNewTest in 171
Done with runSecondNewTest in 170

[anuragdy]

Benchmarks with JMH

JDK11

Benchmark                      Mode  Cnt         Score        Error  Units
MyBenchmark.runNewTest        thrpt    5  20981286.747 ±  52439.964  ops/s
MyBenchmark.runOldTest        thrpt    5   9725982.396 ± 887782.808  ops/s
MyBenchmark.runSecondNewTest  thrpt    5  45765078.184 ± 787752.738  ops/s

JDK8

Benchmark                      Mode  Cnt         Score         Error  Units
MyBenchmark.runNewTest        thrpt    5  25402245.677 ± 2107638.349  ops/s
MyBenchmark.runOldTest        thrpt    5  14939262.248 ±  632445.140  ops/s
MyBenchmark.runSecondNewTest  thrpt    5  51103154.143 ± 1789043.949  ops/s

JMH Test Code -

package org.sample;

import org.openjdk.jmh.annotations.Benchmark;
import org.openjdk.jmh.annotations.BenchmarkMode;
import org.openjdk.jmh.annotations.Fork;
import org.openjdk.jmh.annotations.Measurement;
import org.openjdk.jmh.annotations.Mode;
import org.openjdk.jmh.annotations.OutputTimeUnit;
import org.openjdk.jmh.annotations.Scope;
import org.openjdk.jmh.annotations.State;
import org.openjdk.jmh.annotations.Warmup;

import java.util.Arrays;
import java.util.HashMap;
import java.util.concurrent.TimeUnit;


public class MyBenchmark {

    @State(org.openjdk.jmh.annotations.Scope.Benchmark)
    public static class BenchmarkState {
        private static final String[] SCOPE_NAMES =
                new String[]{"applicationScope", "cookie", "header", "headerValues", "initParam", "pageContext", "pageScope", "param",
                        "paramValues", "requestScope", "sessionScope"};
        private static final String[] TEST_SCOPES =
                {"requestScope", "pageScope", "sessionScope", "pageContext", "header", "applicationScope", "cookie", "headerValues",
                        "initParam", "param", "paramValues"};
    }

    @Benchmark
    @BenchmarkMode(Mode.Throughput)
    @Fork(value = 1, warmups = 1)
    public void runOldTest(BenchmarkState state) {
        for (int i = 0; i < state.TEST_SCOPES.length; i++) {
            Arrays.binarySearch(state.SCOPE_NAMES, state.TEST_SCOPES[i]);
        }
    }

    @Benchmark
    @BenchmarkMode(Mode.Throughput)
    @Fork(value = 1, warmups = 1)
    public void runNewTest(BenchmarkState state) {
        int foundScope = -1;
        for (int i = 0; i < state.TEST_SCOPES.length; i++) {
            String scope = state.TEST_SCOPES[i];
            switch (scope) {
                case "requestScope":
                    foundScope = 1;
                    break;
                case "pageScope":
                    foundScope = 2;
                    break;
                case "sessionScope":
                    foundScope = 3;
                    break;
                case "pageContext":
                    foundScope = 4;
                    break;
                case "header":
                    foundScope = 5;
                    break;
                case "applicationScope":
                    foundScope = 6;
                    break;
                case "cookie":
                    foundScope = 7;
                    break;
                case "headerValues":
                    foundScope = 8;
                    break;
                case "initParam":
                    foundScope = 9;
                    break;
                case "param":
                    foundScope = 10;
                    break;
                case "paramValues":
                    foundScope = 11;
                    break;
                default:
                    throw new RuntimeException("Did not find scope for value " + scope);
            }
            if (foundScope < 0) {
                throw new RuntimeException();
            }
        }
    }

    @Benchmark
    @BenchmarkMode(Mode.Throughput)
    @Fork(value = 1, warmups = 1)
    public void runSecondNewTest(BenchmarkState state) {
        for (int i = 0; i < state.TEST_SCOPES.length; i++) {
            Scope.fromValue(state.TEST_SCOPES[i]);
        }
    }

    public static void main(String[] args) throws Exception {
        org.openjdk.jmh.Main.main(args);
    }

    private enum Scope {
        applicationScope(0), cookie(1), header(2), headerValues(3), initParam(4), pageContext(5), pageScope(6), param(7), paramValues(8),
        requestScope(9), sessionScope(10);

        final public int idx;
        private static final HashMap<String, Scope> lookupMap = new HashMap<>();

        static {
            for (Scope scope : Scope.values()) {
                lookupMap.put(scope.name().toLowerCase(), scope);
            }
        }

        Scope(int idx) {
            this.idx = idx;
        }

        public static Scope fromValue(String value) {
            return lookupMap.get(value);
        }
    }
}

[anuragdy]

Hi @michael-o I wonder if you have any further thoughts/comments on this change and if we could finish the Code Review process soon.

[michael-o]

I cannot make any judgement on any performance improvement. I'd prefer to leave this to other committers.

[anuragdy]

Okk, Thanks for letting me know. I will check with the Bug Reviewer.

[markt-asf]

The test is not a fair test since the Enum approach doesn't include the cost of the switch. If that is included, the original optimisation is better. I will be applying a version of the original patch provided via BZ 67080.