-
Notifications
You must be signed in to change notification settings - Fork 4.9k
perf(GeoIPMatcher): faster heuristic matching with reduced memory usage #5289
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
Conversation
|
review 时突然意识到 xray 目前不支持在一个 rule 里写多个反转的 geoip 现在因为我 merge 了它们,正向和反向各自合为一个 matcher,反向的内部是 and 关系 |
This comment was marked as resolved.
This comment was marked as resolved.
There were no bugs.
so or for example /// currently there is no option for intersection-IPs, if you need that you can add new option for that and you should not break current logic. |
|
|
Ok, if you still don't understand this is with middle steps: rule-1: rule-1 and rule-2 -> /// this is just elementary logic/set-theory. |
|
把我给看乐了 对于 GeoIP 来说 A 和 B 没有交集 |
|
以及你在 #4666 能写出这种代码 if isFound && !reverse {
newIPs = append(newIPs, ip)
continue
}
if !isFound && reverse {
newIPs = append(newIPs, ip)
continue
} |
This is completely correct. according to #5289 (comment) Anyway, @RPRX can also decide whether |
|
你用 AI 写了一大堆集合运算的推导 但凡上过高中,都能轻易的算出 !A or !B 结果是所有 IP 地址,懂吗? 以及你 #4666 代码问题,但凡写过两天代码都知道应该用 isFound != reverse |
|
其实就行为这点上 @patterniha 可能是对的 |
|
问题的关键点在于多个 按照最主流的 geoip 用法绝不可能同时用两个 A B 有机会产生交集 归根结底这是一个设计问题:
改成 2 只需要三秒,我把 neg 那行给注释掉就行,换来无非就是不痛不痒的 至于 @patterniha 就算了吧 |
|
原有的 matcher 间 OR 的设计 100% 原作者压根没考虑到 甚至没能想到把正向 matcher 合并会更快并且更加地节省内存 何况当时是 v2 版的 geoip 因此分类间不可能有交集 只是后来 Loyalsoldier 搞出了非标 geoip 我尝试着理解 ! OR 用法,想写一个有实际意义的用例都写不出来 你甚至不能在里面写更多 所以我说 @patterniha 是在根本不懂集合运算的情况下完全靠 AI 写了一段垃圾回复 总之多个 |
|
@Fangliding 以及你是否认为有必要 cover >24 >48 可能在不同 geoip 分类中,并且 DNS 查询结果会在一次响应同时包含它们时,当下的启发式算法导致出现 bug 的问题 我觉得不可能叠出这种 buff |
|
我不知道你说的BUG具体是什么 不过看标题滤IP速度提升就20%那我觉得还是正确性重要一点 我之前也无聊优化一个几百ns的函数纯当玩 到后面我发现造成两个版本最大的区别是找 crypto/rand.Read 多要了几个字节 这种东西平时拿来用都不眨眼的 所以我一直强调有的东西没必要啥就没必要 |
|
比如: www.example.com 同时响应这俩 ip 当下的启发式算法会把 >=24 的合并了去二分查找 导致:
如果要 cover 这种 edge case 我再实现一个 matcher |
|
Have you ever wondered why I added the also, for router-rules, you can add an option something like /// currently, if you need !(A ∪ B) you can do: or simply: there is no other way, you can't violate logic just because of your need. in short, Regardless of the application, the logic must be correct first. |
|
@patterniha 我现在十分确信你根本不知道把 ! 间 AND 到底意味着什么 在你不懂集合运算的情况下,我建议你像机器一样,简单的找个 IP 代入看看会发生什么 |
|
@patterniha is logically and theoretically correct, and from my perspective I do believe he knows set theory. And yes -- you do have a good point that If Also, I would like to remind everyone here about De Morgan's laws: |
|
我们都同意 ! OR 几乎无用 把 ! 间由 OR 改为 AND 仅是此 PR 中不值一提的改动 从实际需求的角度出发,我不提出来原有的语义问题,根本都没人在意此问题
在我看来直接把 ! AND 更实用,更符合人类直觉,搜索引擎们也是这么做的 if isFound && !reverse {
newIPs = append(newIPs, ip)
continue
}
if !isFound && reverse {
newIPs = append(newIPs, ip)
continue
}
以及,如果你知道这段神奇代码的作者,我相信你肯定不会说出这句话 |
你刚开始加这个选项纯粹是为了想要在伊朗滥用 cfworker 顺畅的访问 twitter 以致于你设计出来一个几乎完全无用的选项 |
|
Difference between: and: is just 1-nanosecond!, because it just checks an extra this is just a chore-minor-optimization!, there are many worse cases in the code. You can open pr for them. |
|
这不是性能差距问题,这是程序员的尊严问题 |
|
Well, that code is ... at least correct, though suboptimal (maybe I suggested making it an error for compatibility concerns: just make it explicit that there is a breaking change (even if it will probably only break < ~10 configs). I do agree that your implementation is intuitive. It's just I think it's better to make all users know that it's being changed. Now I think maybe it'd be good to add a temporary "ipMatcher": "legacy" (default, with error on multiple negations) | "searchEngineLike" (or any name you like for this strategy) settings somewhere, just like |
通常来说学过集合论或布尔代数甚至是 junior coder 都不至于写出这种代码 以及如果会集合论应当使用正确的符号表示补集和全集,而不是疯狂地使用 并且他的多轮回复经提醒仍意识不到最基本的不相交全集问题,说明不会最简单的集合运算 那么真相只有一个了。。。
只要有人能举出 甚至对于我来说,多个 但在自己完全不懂的情况下,乱用 AI 生成垃圾回复来否定整个 PR 是不可接受的 |
|
此 PR 带来的性能收益主要靠合并 matcher 提速 2~10 倍 其实本来也足够快的,我的用例下一条路由规则 3295ns |
|
|
|
重构 geosite 以节省内存昨天开始动工了 |
|
|
|
妥妥的 本来还担心这几个 PR 不合的话我 geosite 咋写 |
本以为重构 geosite 很简单 dns 那边需要先把域名匹配所有服务器的所有规则,好能挑选服务器去解析 而路由那边是逐条 rule 匹配,每条 rule 是一个 matcher 按我之前预想的计划节省内存,无非是让每 rule 甚至是每 geosite 共享 matcher 实例 由重构导致的任何性能下降都是不可接受的 然后我想了一个方案,改造路由的 mphmatcher 让其保存域名所在分类 dns 遇到域名去查其所属分类,再去遍历各服务器,算交集 给两个切片算交集是原本没有的步骤,为了追平差距这里可以用 []uint64 做 bitmask 的 & 运算,快速路径是单 uint64 当关注分类小于 64 个时 这样完美地解决了 dns 多次 hashmap.get 问题 然而这又导致了 router 性能下降,因为还有个大坑是 regexp 然后问题又来了,mphmatcher 需要一口气把规则集全部加载完再 build 问题又又又来了,api 调用动态添加的 rule 咋办,想了一下直接放弃它 这是个十分浩大的工程 然后正则这边还有个微优化 |
1. geoip:cn geoip:!cn 同时用,不会再占两份内存
2. 多 matcher 现在会智能合并,更有机会被内部 IPSet 合并 CIDR 以节省内存
我用的 IP 库更准,我把国家按大洲写在一条 rule 里,以前会吃很多内存
Before
localhost [~]# free -h total used free shared buff/cache available Mem: 1.9G 866.2M 938.0M 192.0K 174.1M 964.4M Swap: 256.0M 0 256.0MAfter
localhost [~]# free -h total used free shared buff/cache available Mem: 1.9G 599.7M 1.2G 192.0K 175.4M 1.2G Swap: 256.0M 0 256.0M3. 启动速度更快,现在时间减半
我用的 IP 库体积约 60m 以前光启动就要三分钟
4. 一条 rule 含多个 matcher 时提升 2~10 倍,比快更快
Before
After
5. 多 IP 匹配、过滤速度更快,具体取决于 DNS 解析结果 /24 (/48) 的聚合情况
以 youtube 为例约提升 20% 左右
这辈子都不可能写 benchmark 和 test 的