Skip to content

Interceptor optimization #1993

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
brucelwl opened this issue Jul 26, 2020 · 13 comments
Closed

Interceptor optimization #1993

brucelwl opened this issue Jul 26, 2020 · 13 comments

Comments

@brucelwl
Copy link
Contributor

If there are multiple interceptors intercepting the same method, the same number of dynamic proxy classes will be created. However, the dynamic proxy has certain performance loss. I hope that it can be optimized to avoid the creation of multiple dynamic proxy classes.
example:

@Intercepts({
        @Signature(type = Executor.class, method = "update", args = {MappedStatement.class, Object.class})

})
public class MyInterceptor implements Interceptor {
    private static final Logger logger = LoggerFactory.getLogger(MyInterceptor.class);

    private Properties properties;

    @Override
    public void setProperties(Properties properties) {
        this.properties = properties;
    }

    @Override
    public Object intercept(Invocation invocation) throws Throwable {
        logger.info("invoke");
        return invocation.proceed();
    }
}
@Intercepts({
        @Signature(type = Executor.class, method = "update", args = {MappedStatement.class, Object.class})

})
public class MyInterceptor2 implements Interceptor {
    
}

This will cause two dynamic proxy classes to be created !!!

At present, Mybatis supports four types of interface interception
ParameterHandler, ResultSetHandler, StatementHandler, Executor

Proposal 1: change to responsibility chain mode,developers can provide specific implementation classes of these interfaces to intercept corresponding methods

Proposal 2: Only a proxy class can be used to intercept interface methods,The advantage of this approach is that it does not change the existing interceptor implementation logic

@brucelwl
Copy link
Contributor Author

brucelwl commented Aug 8, 2020

@harawata Can you reply to my issue?

@harawata
Copy link
Member

harawata commented Aug 8, 2020

Hello @brucelwl ,

I'm sorry, but I am not so familiar with this topic.
Could you post some data showing the 'certain performance loss', please?

@brucelwl
Copy link
Contributor Author

brucelwl commented Aug 8, 2020

@harawata
I did a test. If an interface is proxy only once, the performance is much higher than that of proxy multiple times, and the performance increases in proportion to the number of agents

environment:
windows 10
JDK:JDK 1.8.0_171, Java HotSpot(TM) 64-Bit Server VM, 25.171-b11
CPU: i5
pc memory:16G
Other: all defaults
Benchmarking tools:JMH

Benchmark code

public interface ExecutorHandler {
    int getProxyCount();

    int update(MappedStatement ms, Object parameter) throws SQLException;

    <E> List<E> query(MappedStatement ms, Object parameter);
}

public class ExecutorHandlerImpl implements ExecutorHandler {

    private int count;

    public ExecutorHandlerImpl(int count) {
        this.count = count;
    }

    @Override
    public int getProxyCount() {
        return count;
    }

    @Override
    public int update(MappedStatement ms, Object parameter) throws SQLException {
        return 10 * 20 / 5 * 2 + 1 + 15;
    }

    @Override
    public <E> List<E> query(MappedStatement ms, Object parameter) {
        return null;
    }
}
public class Plugin implements InvocationHandler {

    private final Object target;

    private Plugin(Object target) {
        this.target = target;
    }

    public static Object wrap(Object target) {
        Class<?> type = target.getClass();
        Class<?>[] interfaces = getAllInterfaces(type);
        if (interfaces.length > 0) {
            return Proxy.newProxyInstance(type.getClassLoader(), interfaces, new Plugin(target));
        }
        return target;
    }

    private static Class<?>[] getAllInterfaces(Class<?> type) {
        Set<Class<?>> interfaces = new HashSet<>();
        while (type != null) {
            interfaces.addAll(Arrays.asList(type.getInterfaces()));
            type = type.getSuperclass();
        }
        return interfaces.toArray(new Class<?>[0]);
    }

    @Override
    public Object invoke(Object proxy, Method method, Object[] args) throws Throwable {
        return method.invoke(target, args);
    }
}


public class InterceptorChain {

    public Object pluginAll(ExecutorHandler target) {
        int proxyCount = target.getProxyCount();

        for (int i = 0; i < proxyCount; i++) {
            target = (ExecutorHandler) Plugin.wrap(target);
        }
        return target;
    }
}
@BenchmarkMode(Mode.AverageTime)
@Warmup(iterations = 1) 
@Threads(15)
@State(Scope.Benchmark)
@Measurement(iterations = 2, time = 5, timeUnit = TimeUnit.SECONDS)
@OutputTimeUnit(TimeUnit.NANOSECONDS) 
public class ProxyBenchmarkTest {
    private static final Logger logger = LoggerFactory.getLogger(ProxyBenchmarkTest.class);

    ExecutorHandler executorHandler;
    ExecutorHandler executorHandler2;

    @Setup
    public void setup() throws Exception {
        InterceptorChain interceptorChain = new InterceptorChain();
        executorHandler = (ExecutorHandler) interceptorChain.pluginAll(new ExecutorHandlerImpl(1));
        executorHandler2 = (ExecutorHandler) interceptorChain.pluginAll(new ExecutorHandlerImpl(15));
    }

    @Benchmark
    public void jdkProxyN(Blackhole blackhole) throws Exception {
        MappedStatement mappedStatement = new MappedStatement(
                "select * from user info", 5,500,"c://aaa.mapper");

        List<Object> query = executorHandler2.query(mappedStatement, 45);
        blackhole.consume(query);
    }

    @Benchmark
    public void jdkProxy1(Blackhole blackhole) throws Exception {
        MappedStatement mappedStatement = new MappedStatement(
                "select * from user info", 5,500,"c://aaa.mapper");

        List<Object> query = executorHandler.query(mappedStatement, 45);
        blackhole.consume(query);
    }

    public static void main(String[] args) throws Exception {
        Options options = new OptionsBuilder().include(ProxyBenchmarkTest.class.getName())
                //.output("benchmark/jedis-Throughput.log")
                .forks(0)
                .build();
        new Runner(options).run();

    }
}

Proxy 1 time vs Proxy 3 times
image

Proxy 1 time vs Proxy 5 times
image

Proxy 1 time vs Proxy 8 times
image

@brucelwl
Copy link
Contributor Author

brucelwl commented Aug 8, 2020

@harawata
see PR https://github.com/mybatis/mybatis-3/pull/2001/files please
This PR has two main purposes:

  1. The first goal is performance improvement
  2. The second goal is to make the generated proxy class structure clearer

The current implementation code will make the proxy class nest another proxy. The more interceptors, the more nested the proxy class. But in fact, if only one proxy class is used, the class structure will be clearer

@harawata
Copy link
Member

harawata commented Aug 8, 2020

Thank you, @brucelwl .

I will look into it, but it may take some time as I don't have much spare time lately.

@brucelwl
Copy link
Contributor Author

brucelwl commented Aug 9, 2020

@harawata
Another more optimized way is to use the decorator pattern to form an interceptor chain, so that there is no need to use a dynamic proxy to implement the interceptor, If you agree, I can submit a pr

@harawata
Copy link
Member

@brucelwl ,

Just curious, how many plugins do you use in your project?
I don't use plugins very often and have never had any performance issue myself.

Anyway, I need some time to understand the current design before answering your question.
It would be easier for us to understand your proposals if you submit the change as a PR indeed, but please do understand there is no guarantee that your PRs will be merged.

@brucelwl
Copy link
Contributor Author

@harawata The performance problem is only relative. I just want to have an optimized solution

@wushp
Copy link

wushp commented Oct 26, 2020

@harawata ありがとう!

@harawata
Copy link
Member

I finally took some time to look into it and my conclusion is : the number of dynamic proxies does not have significant impact on performance.

Here is the JMH project I used.
https://github.com/harawata/mybatis-gh1993-jmh

There are three methods.

  • zeroInterceptor
  • fiveInterceptors
  • fiveFakes

zeroInterceptor is a baseline, so to speak.
There is no Interceptor instance in the InterceptorChain, so there will be no dynamic proxy, obviously.
On my machine, the throughput was about 200-230.

fiveInterceptors adds five Interceptor instances to the InterceptorChain, so there will be five dynamic proxies created for every update() invocation.
The throughput was about 1.4-1.7, so it's much slower than zeroInterceptor.

fiveFakes also adds five Interceptor instances to the InterceptorChain, however, their @Signature references unsupported method, so no dynamic proxies will be created.
If the number of dynamic proxies affects performance, as you suggested, the throughput of fiveFakes should be much higher than fiveInterceptors, but the actual throughput is pretty much the same as the throughput of fiveInterceptors.

Benchmark Mode Cnt Score Error Units
MyBenchmark.fiveFakes thrpt 25 1.750 ± 0.136 ops/s
MyBenchmark.fiveInterceptors thrpt 25 1.617 ± 0.127 ops/s
MyBenchmark.zeroInterceptor thrpt 25 229.677 ± 2.956 ops/s
  • The difference between fiveInterceptors and fiveFakes (or the absence thereof) shows that the number of dynamic proxies does not affect the performance significantly.
  • The difference between zeroInterceptor and fiveFakes shows that the bottleneck exists somewhere between InterceptorChain#pluginAll() and Plugin#wrap(). [1]

Please try the benchmark yourself and see if you reach the same conclusion.
And please let me know if there is any issue with my benchmark code/config (it's possible).


Just to be sure, I ran the same benchmarks against PR #2001 (master branch merged locally).
As you can see, there is no significant difference.

Benchmark Mode Cnt Score Error Units
MyBenchmark.fiveFakes thrpt 25 1.774 ± 0.135 ops/s
MyBenchmark.fiveInterceptors thrpt 25 1.655 ± 0.132 ops/s
MyBenchmark.zeroInterceptor thrpt 25 230.693 ± 4.927 ops/s

I also ran the benchmarks against PR #3396 .
The throughput increased a little bit, but not much.

Benchmark Mode Cnt Score Error Units
MyBenchmark.fiveFakes thrpt 25 3.944 ± 0.310 ops/s
MyBenchmark.fiveInterceptors thrpt 12 3.426 ± 0.435 ops/s
MyBenchmark.zeroInterceptor thrpt 25 232.382 ± 3.758 ops/s

[1] This is just an experiment, but after applying the following patch to the master, the throughput of fiveFakes was about 10 which is better, but not as good as I thought it would be.

Benchmark Mode Cnt Score Error Units
MyBenchmark.fiveFakes thrpt 25 10.229 ± 1.007 ops/s
diff --git a/src/main/java/org/apache/ibatis/plugin/Plugin.java b/src/main/java/org/apache/ibatis/plugin/Plugin.java
index feae8724c8..8da6166095 100644
--- a/src/main/java/org/apache/ibatis/plugin/Plugin.java
+++ b/src/main/java/org/apache/ibatis/plugin/Plugin.java
@@ -41,12 +41,6 @@ public class Plugin implements InvocationHandler {
   }
 
   public static Object wrap(Object target, Interceptor interceptor) {
-    Map<Class<?>, Set<Method>> signatureMap = getSignatureMap(interceptor);
-    Class<?> type = target.getClass();
-    Class<?>[] interfaces = getAllInterfaces(type, signatureMap);
-    if (interfaces.length > 0) {
-      return Proxy.newProxyInstance(type.getClassLoader(), interfaces, new Plugin(target, interceptor, signatureMap));
-    }
     return target;
   }
 

@brucelwl
Copy link
Contributor Author

@harawata Although this optimization does not approach the performance of zero interceptors, even with a small amount of optimization, it is still a worthwhile thing to do, and it has not made things worse than before
Due to the long time, my original PR has been discarded. If you are willing to merge this optimization, I can submit a new PR and resubmit the optimization proposed by 3396 together

brucelwl added a commit to brucelwl/mybatis-3 that referenced this issue Mar 11, 2025

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Interceptor optimization mybatis#1993
@harawata
Copy link
Member

harawata commented Mar 11, 2025

@brucelwl ,

You raised two points .

  1. The first goal is performance improvement
  2. The second goal is to make the generated proxy class structure clearer

To be frank, I don't think PR #2001 makes the structure clearer and the performance "gain" is trivial even with five interceptors (I have no idea how many plugins you guys use, but five seems like "a lot"), so I don't see any advantage of the design change.

@brucelwl
Copy link
Contributor Author

Just three plugins, comparison of proxy structure before and after optimization

Structure after multiple proxies before optimization, If there are five plugins, this stack will be deeper
Image

Optimized proxy structure

Image

Obviously, the second picture is clearer

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants