alibaba / druid

阿里云计算平台DataWorks(https://help.aliyun.com/document_detail/137663.html) 团队出品,为监控而生的数据库连接池
https://github.com/alibaba/druid/wiki
Apache License 2.0
27.96k stars 8.58k forks source link

shrink 算法优化 #5567

Closed xuziyang closed 10 months ago

xuziyang commented 11 months ago

当shrink()方法,checkTime传参为false时,执行下面图片的这行代码时,connections中剩余的可用连接,并没有放入shrinkBuffer中,执行下图中的代码时,导致connections中可用连接被清空

image

而且执行完这行代码后shrinkBuffer中的元素,没有被置为Null,也会产生很多问题

一些优化的想法: 执行连接可用性检测后,使用双指针算法,将connections中剩余可用的连接移动整理到connections的头部,这样可以有一下好处:

  1. 减少了shrinkBuffer和connections,之间的两次 System.arraycopy() 拷贝操作
  2. 代码更简洁高效,可读性更高
zrlw commented 11 months ago

仅当测试时checkTime为false,正常运行状态下这个方法只有DestroyTask调用,入参一直是true

xuziyang commented 11 months ago

仅当测试时checkTime为false,正常运行状态下这个方法只有DestroyTask调用,入参一直是true

我提交了一个pr,用双指针来减少了两次System.arraycopy()和 优化了代码提高了可读性 ,有时间麻烦review一下

zrlw commented 11 months ago

PR有处错误需要改一下,具体见PR回复。 另外建议把issue名称和描述内容修订一下,清空连接这几行代码应当只是为了单元测试,算不上bug。

zrlw commented 11 months ago

checkTime传参为false时:

int removeCount = evictCount + keepAliveCount;
            if (removeCount > 0) {
                System.arraycopy(shrinkBuffer, 0, connections, 0, remaining); // checkTime=false时,remaining此时为0,不会执行copy操作
                int breakedCount = poolingCount - i;  //  如果没有配置minIdle此处i就等于poolingCount,前面的检测循环处理会将所有的连接加到evictConnections数组,breakedCount=0,将直接执行下面的fill操作清空所有连接
                if (breakedCount > 0) {
                    // 假设用户配置了minIdle参数,比如是2,那么此处breakCount就等于2,就会复制末位的2个连接到数组头部
                    System.arraycopy(connections, i, connections, remaining, breakedCount);
                    // breakCount为2时,remaining也会变成2
                    remaining += breakedCount; 
                }
                // 假设用户配置了minIdle参数=2, 那么此处将2(0based)开始的元素清空,避免多处引用导致内存不能及时回收
                Arrays.fill(connections, remaining, poolingCount, null);
                poolingCount -= removeCount;
            }
zrlw commented 11 months ago

从你提交的PR看,你可能混淆了keepAliveConnections和shrinkBuffer,放入keepAliveConnections和evictConnections的连接都是需要从连接池清除的,只是keepAlive验证有效的连接会被重新放回连接池。

xuziyang commented 11 months ago

从你提交的PR看,你可能混淆了keepAliveConnections和shrinkBuffer,放入keepAliveConnections和evictConnections的连接都是需要从连接池清除的,只是keepAlive验证有效的连接会被重新放回连接池。

我觉得并没有混淆: keepAliveConnections存的是待可用性检测连接,先从connections移除,在这个方法的下面检测通过后,放到coneections的尾部 shrinkBuffer 就是一个临时存储,目的是将connections中没有放入evictConnections和keepAliveConnections的,整理到connetions的数据的前面,剔除放入evictConnections和keepAliveConnections的连接

zrlw commented 11 months ago

从你提交的PR看,你可能混淆了keepAliveConnections和shrinkBuffer,放入keepAliveConnections和evictConnections的连接都是需要从连接池清除的,只是keepAlive验证有效的连接会被重新放回连接池。

并没有混淆: keepAliveConnections存的是待可用性检测连接,先从connections移除,在这个方法的下面检测通过后,放到coneections的尾部 shrinkBuffer 就是一个临时存储,目的是将connections中没有放入evictConnections和keepAliveConnections的,整理到connetions的数据的前面,剔除放入evictConnections和keepAliveConnections的连接

但你的代码是将keepAliveConnections的连接直接留在了connections里,需要保留的连接反而直接设置了清除标记

xuziyang commented 11 months ago

从你提交的PR看,你可能混淆了keepAliveConnections和shrinkBuffer,放入keepAliveConnections和evictConnections的连接都是需要从连接池清除的,只是keepAlive验证有效的连接会被重新放回连接池。

并没有混淆: keepAliveConnections存的是待可用性检测连接,先从connections移除,在这个方法的下面检测通过后,放到coneections的尾部 shrinkBuffer 就是一个临时存储,目的是将connections中没有放入evictConnections和keepAliveConnections的,整理到connetions的数据的前面,剔除放入evictConnections和keepAliveConnections的连接

但你的代码是将keepAliveConnections的连接直接留在了connections里,需要保留的连接反而直接设置了清除标记

不是的,您仔细看看 connectionsRemoveFlag 为true的都是要移除的,放入keepAliveConnections的连接,也标记为true了

xuziyang commented 11 months ago

image 这两行代码已经在pr里,删除了

zrlw commented 11 months ago

image 这两行代码已经在pr里,删除了

哦哦,看漏了,需要看全部修订后的内容,那么后面新增的方法还需要末位补null了,否则和shrinkbuffer一样存在不置null内存不及时回收的情况,虽然我觉得影响不大,并不影响连接正常使用和关闭,只是会消耗一些内存,最多消耗maxAcitve个DruidPooledConnection的内存吧(连接全部被discard丢弃的时候)。

xuziyang commented 11 months ago

好的 我修改一下,并把pr的名字改了

zrlw commented 11 months ago

我看你只是改了一下pr名称,review建议并没有回复。 我参照你的pr提交了一个pr,把removeFlag数组也省掉了: #5570 这样可能效率更好一点吧。

xuziyang commented 11 months ago

我看你只是改了一下pr名称,review建议并没有回复。 我参照你的pr提交了一个pr,把removeFlag数组也省掉了: #5570 这样可能效率更好一点吧。

看了你的实现方式,省略了removeFlag数组,把一部分connections中整理的逻辑放到了for循环中,一部分放到了for循环结束后,有点分散 而我的实现逻辑,把conections整理的操作内聚到了compactConnections方法中,我认为有更好的可读性以及维护性

两者更有优劣。

zrlw commented 11 months ago

缩减连接池的操作都是加锁后的临界区操作,应当优先考虑执行效率,尽可能地加快执行速度,缩减持有锁的时间。 另外,我觉得druid最终优化目标应该是借鉴netty使用mpsc队列处理海量并发请求的机制,通过无锁有界队列重构连接池。

zrlw commented 11 months ago

对于maxActive=100之内的应用而言,两种处理方式的性能应该差不多,但是还要考虑maxActive配了500甚至更多的应用。 另外java的for循环执行效率比native方式的arrayCopy效率要低,后者是编译过的c代码,采用的是内存块整体复制的操作。

xuziyang commented 11 months ago

对于maxActive=100之内的应用而言,两种处理方式的性能应该差不多,但是还要考虑maxActive配了500甚至更多的应用。 另外java的for循环执行效率比native方式的arrayCopy效率要低,后者是编译过的c代码,采用的是内存块整体复制的操作。

PostgreSQL提供了一个设置预期线程池大小的公式: connections = ((core_count * 2) + effective_spindle_count)

该公式来自于: https://github.com/brettwooldridge/HikariCP/wiki/About-Pool-Sizing

其中,core_count是CPU核心, effective_spindle_count 的含义是有效主轴数,如果你的服务器使用的是带有16个磁盘的RAID,那么valid_spindle_count=16。它实质上是服务器可以管理多少个并行I / O请求的度量。 旋转硬盘一次(通常)一次只能处理一个I / O请求,如果你有16个,则系统可以同时处理16个I / O请求。

我觉得它对mysql等数据库同样适用,一般连接数量十几个就够了,上百个的场景应该很少

xuziyang commented 11 months ago

对于maxActive=100之内的应用而言,两种处理方式的性能应该差不多,但是还要考虑maxActive配了500甚至更多的应用。 另外java的for循环执行效率比native方式的arrayCopy效率要低,后者是编译过的c代码,采用的是内存块整体复制的操作。

在 Java 里面调用 Native 代码会有以下问题.

  1. 不能对短的方法做 inline;
  2. 要新建一个 native 栈;
  3. 不能对 native 方法做运行时优化;
  4. 要复制参数到 native 栈;

因此,调用native方法,会导致运行速度变慢的

zrlw commented 11 months ago

多数分布式应用maxActive配置十几个就足够了,客户端都是虚拟出来的,每台配置通常不会超过4c8g,磁盘满足日志存放即可,支撑不了大并发io。 但是也有拿druid做数据服务器的应用场景,用的是配了上百个核的物理服务器,maxActive直接配成了512,可以翻翻之前的issue。 大多数情况下System.arrayCopy比java的for循环赋值要快,并且没有普通native方法的弊端,是因为 it's very likely that System.arraycopy is just a JIT intrinsic; meaning that when code calls System.arraycopy, it will most probably be calling a JIT-specific implementation (once the JIT tags System.arraycopy as being "hot") that is not executed through the JNI interface, so it doesn't incur the normal overhead of native methods. For more details see: Is Java's System.arraycopy() efficient for small arrays? https://stackoverflow.com/questions/8526907/is-javas-system-arraycopy-efficient-for-small-arrays

zrlw commented 11 months ago

https://sudonull.com/post/61026-Full-list-of-intrinsic-functions-in-HotSpot-in-JDK-7-8-9-and-10 An intrinsic or intrinsic function is a function that the JIT compiler can embed instead of calling Java and JNI code for optimization purposes. 其中System的intrinsic functions

java.lang.System

_identityHashCode         java.lang.System.identityHashCode(Object)
_currentTimeMillis        java.lang.System.currentTimeMillis()
_nanoTime                 java.lang.System.nanoTime()
_arraycopy                java.lang.System.arraycopy(Object, int, Object, int, int)

当然并不是说用intrinsic function性能就一定好到哪里去,只是频繁的调用优先使用intrinsic function方法大概率不会有问题。 另外如果用的是IDEA,凡是for循环赋值的代码它都告警,提示你要把它改成arrayCopy,老是提示可能你就烦了。

zrlw commented 11 months ago

另外这个issue的标题建议改一下,checkTime=false这段代码只是单元测试用到的,删了也不影响实际运行。

xuziyang commented 11 months ago

另外这个issue的标题建议改一下,checkTime=false这段代码只是单元测试用到的,删了也不影响实际运行。

单元测试,要测试一条正常代码执行不到的路径,感觉不太合理。建议删除了,因为看源码的时候很容易误导人

zrlw commented 11 months ago

还有很多代码应该都是废弃不用的,只有部分在方法名上标注了Deprecated注解,可能历史包袱积累的多,很多代码都是历经了12年的老古董,可能谁都不清楚有没有基于这段逻辑的定制化需求。 近期druid的响应情况还算好的,主要是 @lizongbo 在做维护,如果没有维护,很多issue和pr都会一直在那里挂着没人管的,如果你留意一下,前些年druid几乎处于停滞状态,从有人提issue提pr,到有权限合并分支的出面复核pr,将pr合并到master发版,前后耗时超过1年的情况比比皆是,我们发现问题时都是自己定制一下版本自己搞了。 我个人推测是druid在阿里开源社区排不上优先级,所以一直以来都没有形成dubbo、nacos那样的团队支撑力量,纯个人利用业余时间在做一些支持,所以和商业软件相比,期望值就不能高了。

lizongbo commented 11 months ago

还有很多代码应该都是废弃不用的,只有部分在方法名上标注了Deprecated注解,可能历史包袱积累的多,很多代码都是历经了12年的老古董,可能谁都不清楚有没有基于这段逻辑的定制化需求。 近期druid的响应情况还算好的,主要是 @lizongbo 在做维护,如果没有维护,很多issue和pr都会一直在那里挂着没人管的,如果你留意一下,前些年druid几乎处于停滞状态,从有人提issue提pr,到有权限合并分支的出面复核pr,将pr合并到master发版,前后耗时超过1年的情况比比皆是,我们发现问题时都是自己定制一下版本自己搞了。 我个人推测是druid在阿里开源社区排不上优先级,所以一直以来都没有形成dubbo、nacos那样的团队支撑力量,纯个人利用业余时间在做一些支持,所以和商业软件相比,期望值就不能高了。

温少平时应该是太忙了,没有太多精力来维护。 我是项目中刚好深度使用了druid,基于druid开发了不少在项目组中使用的公共特性(比如where a=null这种隐患sql拦截告警之类的),然后也遇到了druid的一些问题,因此开始反馈代码,然后温少最近给我加了权限,因此开始处理历史反馈问题和pr,也欢迎大家贡献代码,我应该可以一直参与维护。 目前连接池管理代码的复杂度很高,其实有想过要不要重构写一版新的,但是现在没那么多精力,先在现有基础上维护保持稳定,将来再来看能不能把代码简化下来变得更加牢固而简单。