Skip to content
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

refactor(core): MidwayMiddlewareService with dispatch() #4177

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

waitingsong
Copy link
Member

No description provided.

@waitingsong waitingsong force-pushed the core-mw branch 4 times, most recently from b6abac8 to f942fed Compare November 7, 2024 08:23
@codecov-commenter
Copy link

codecov-commenter commented Nov 7, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.13%. Comparing base (a3ca53b) to head (db4e1d1).
Report is 455 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4177      +/-   ##
==========================================
+ Coverage   84.55%   85.13%   +0.58%     
==========================================
  Files         491      514      +23     
  Lines       46778    49149    +2371     
  Branches     5601     5674      +73     
==========================================
+ Hits        39551    41845    +2294     
- Misses       7191     7273      +82     
+ Partials       36       31       -5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@czy88840616
Copy link
Member

czy88840616 commented Nov 7, 2024

提升可读性?提出来就不能复用外部的一些对象了,不过性能估计影响应该不大。

@waitingsong
Copy link
Member Author

waitingsong commented Nov 7, 2024

提升可读性?提出来就不能复用外部的一些对象了,不过性能估计影响应该不大。

目的是可复用降低资源消耗。
每个中间件会初始化一次 dispatch,洋葱模型下嵌套的 dispatch 方法的实例数量是中间件的数量,考虑并发请求场景,对于资源的消耗是成倍的。
重构后 dispatch 方法参数是个对象,依然可以通过成员对象来复用外部的变量。之前通过闭包方式引用外部变量,在方法嵌套层级深之后,向上搜索外部变量的开销会不会也增加一些。

2024-11-07_201722

@czy88840616
Copy link
Member

不会的,diapatch 函数本身的定义是在预编译阶段完成,会提升的,所以不会重复初始化,只会将所在的闭包作用域(包含 index、supportBody 等变量)重新创建,性能开销不大的。

@waitingsong
Copy link
Member Author

waitingsong commented Nov 8, 2024

不会的,diapatch 函数本身的定义是在预编译阶段完成,会提升的,所以不会重复初始化,只会将所在的闭包作用域(包含 index、supportBody 等变量)重新创建,性能开销不大的。

现在的 diapatch 函数是闭包定义的,不会全局提升,而是每个中间件进来时初始化一次且仅一次,我调试结果是这样的。

你可以这样试试看:
在 dispatch 函数内添加如下代码,然后调试

  if (typeof dispatch.foo === 'undefined') {
    dispatch.foo = Date.now();
  }

如果注册了两个中间件,那么会有两个不相同的 foo 值。

很久之前不晓得在哪儿看到,

  • 不建议在函数内通过 function foo() {} 声明新的函数
  • 建议在函数内通过 const foo = () =>{} 方式来赋值变量(函数)

@czy88840616
Copy link
Member

调试看到的是堆栈,堆栈代表的是调用,定义预编译是指 v8 在读到 js 代码做的时做的优化,这个调试看不到的。这里的堆栈少不了的。

@waitingsong
Copy link
Member Author

waitingsong commented Nov 8, 2024

假定有中间件A, B

  • 先执行A,此时 diapatch.foo === 1,会在内部再次调用 diapatch 方法,此时 foo 依然 ===1
  • 然后执行B,此时 diapatch.foo === 2,会在内部再次调用 diapatch 方法,此时 foo 依然 ===2
  • 回到A的执行末尾,此时 diapatch.foo === 1

或者不管调试和堆栈,直接在 diapatch() 内部 console 打印 foo 值,也是两种。
这样能不能说明 diapatch 初始化了两次实例呢?

@czy88840616
Copy link
Member

czy88840616 commented Nov 8, 2024

函数没有实例的说法,函数就只有执行调用,函数本身就只有一个定义,在js runtime 解析阶段提升,在执行阶段创建函数上下文,diapatch 只是会多次执行,创建不同的上下文。

dispatch 放在里面,是因为希望函数执行限制在闭包内,这货也不会有外部来调用或者复用,js 里挺常用的。

@waitingsong
Copy link
Member Author

waitingsong commented Nov 8, 2024

这样来测试:

const cache = new Set<unknown>(); // 顶层定义

const composeFn = (context: T, next?) => {
.....
....
      cache.add(dispatch);
      console.log('cacheSize', cache.size);
      return dispatch(0);

控制台输出:现在的 cacheSize 会一直增加(我一个轮子跑单测能逐步增加到 96),而重构之后一直输出 1。
这是否说明 dispatch 函数是多次初始化呢(其实关心的是资源占用是否倍增)?

或者注册超大数量的中间件,比较下重构前后内存是否会暴增呢~

@czy88840616
Copy link
Member

这个不会的,函数本身执行创建的开销很小,可能只占几百字节,即使创建96个实例,额外内存开销可能也就几十 KB,而且请求完就会释放,也不会涉及到泄露。

@waitingsong waitingsong closed this Nov 9, 2024
@czy88840616
Copy link
Member

=== Compose 性能对比测试 ===

开始运行原始版本...
开始运行新版本...

=== 性能对比结果 ===


检查点 10,000 次:
本轮耗时                4.18ms     (旧)      4.18ms     (新)      [提升: 0.00%]
平均每次                81.46ms    (旧)      79.53ms    (新)      [提升: 2.37%]
总计耗时                0.01ms     (旧)      0.01ms     (新)      [提升: 0.00%]
堆内存                 81.46MB    (旧)      79.53MB    (新)      [提升: 2.37%]

检查点 20,000 次:
本轮耗时                4.88ms     (旧)      8.12ms     (新)      [提升: -66.39%]
平均每次                73.98ms    (旧)      73.24ms    (新)      [提升: 1.00%]
总计耗时                0.01ms     (旧)      0.01ms     (新)      [提升: 0.00%]
堆内存                 155.43MB   (旧)      152.77MB   (新)      [提升: 1.71%]

检查点 30,000 次:
本轮耗时                10.00ms    (旧)      10.64ms    (新)      [提升: -6.40%]
平均每次                62.86ms    (旧)      61.03ms    (新)      [提升: 2.91%]
总计耗时                0.01ms     (旧)      0.01ms     (新)      [提升: 0.00%]
堆内存                 218.29MB   (旧)      213.80MB   (新)      [提升: 2.06%]

检查点 40,000 次:
本轮耗时                6.35ms     (旧)      11.75ms    (新)      [提升: -85.04%]
平均每次                63.13ms    (旧)      62.61ms    (新)      [提升: 0.82%]
总计耗时                0.01ms     (旧)      0.01ms     (新)      [提升: 0.00%]
堆内存                 281.42MB   (旧)      276.41MB   (新)      [提升: 1.78%]

检查点 50,000 次:
本轮耗时                10.42ms    (旧)      5.05ms     (新)      [提升: 51.54%]
平均每次                62.43ms    (旧)      61.46ms    (新)      [提升: 1.55%]
总计耗时                0.01ms     (旧)      0.01ms     (新)      [提升: 0.00%]
堆内存                 343.84MB   (旧)      337.87MB   (新)      [提升: 1.74%]

检查点 60,000 次:
本轮耗时                6.65ms     (旧)      6.09ms     (新)      [提升: 8.42%]
平均每次                61.44ms    (旧)      60.42ms    (新)      [提升: 1.66%]
总计耗时                0.01ms     (旧)      0.01ms     (新)      [提升: 0.00%]
堆内存                 405.29MB   (旧)      398.29MB   (新)      [提升: 1.73%]

检查点 70,000 次:
本轮耗时                10.73ms    (旧)      7.14ms     (新)      [提升: 33.46%]
平均每次                61.09ms    (旧)      60.82ms    (新)      [提升: 0.44%]
总计耗时                0.01ms     (旧)      0.01ms     (新)      [提升: 0.00%]
堆内存                 466.38MB   (旧)      459.11MB   (新)      [提升: 1.56%]

检查点 80,000 次:
本轮耗时                6.96ms     (旧)      8.24ms     (新)      [提升: -18.39%]
平均每次                60.84ms    (旧)      61.13ms    (新)      [提升: -0.48%]
总计耗时                0.01ms     (旧)      0.01ms     (新)      [提升: 0.00%]
堆内存                 527.22MB   (旧)      520.24MB   (新)      [提升: 1.32%]

检查点 90,000 次:
本轮耗时                11.15ms    (旧)      9.35ms     (新)      [提升: 16.14%]
平均每次                63.14ms    (旧)      60.37ms    (新)      [提升: 4.39%]
总计耗时                0.01ms     (旧)      0.01ms     (新)      [提升: 0.00%]
堆内存                 590.36MB   (旧)      580.60MB   (新)      [提升: 1.65%]

检查点 100,000 次:
本轮耗时                15.36ms    (旧)      10.46ms    (新)      [提升: 31.90%]
平均每次                64.04ms    (旧)      62.40ms    (新)      [提升: 2.56%]
总计耗时                0.01ms     (旧)      0.01ms     (新)      [提升: 0.00%]
堆内存                 654.40MB   (旧)      643.01MB   (新)      [提升: 1.74%]

10w 次内存影响在 2%左右,耗时不明显,应该 gc 影响比较大。

@waitingsong
Copy link
Member Author

waitingsong commented Nov 9, 2024

刚才我也简单测试比较了下内存占用, rss, heap 各方面没有明显差异。
不过 midway 框架有很多处这种闭包内声明函数,积少成多不知道是否也会有可感知的差异。

@waitingsong
Copy link
Member Author

=== Compose 性能对比测试 ===

开始运行原始版本...
开始运行新版本...

=== 性能对比结果 ===

10w 次内存影响在 2%左右,耗时不明显,应该 gc 影响比较大。

我再重构几处,然后你来测试下差异

@czy88840616
Copy link
Member

一方面 v8 对闭包已经优化的非常好了,另一方面你的改动参数变成对象,还要做解构等,一进一出,几乎都抵消了。

@waitingsong waitingsong force-pushed the core-mw branch 3 times, most recently from d77d751 to 0bf9176 Compare November 9, 2024 10:18
@czy88840616
Copy link
Member

可以跑项目从profile来看top重点优化

@waitingsong
Copy link
Member Author

我本地项目安装依赖

可以跑项目从profile来看top重点优化

不明白,能否详细说下如何操作?

@waitingsong
Copy link
Member Author

我本地(win10)跑不了单测,很麻烦

> @midwayjs/[email protected] test
> node --require=ts-node/register ../../node_modules/.bin/jest --runInBand
F:\project\shared\midway\node_modules\.bin\jest:2
basedir=$(dirname "$(echo "$0" | sed -e 's,\\,/,g')")
          ^^^^^^^
SyntaxError: missing ) after argument list
    at wrapSafe (node:internal/modules/cjs/loader:1469:18)

@czy88840616
Copy link
Member

czy88840616 commented Nov 9, 2024

哦,pnpm 装也是这样的报错,可以直接用 node_modules/jest/bin/jest.js 跑

}

function dispatch(options: DispatchOptions): Promise<unknown> {
const { i, context, newMiddlewareArr, name, next, supportBody } = options;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这里其实本来能复用外部对象的,现在每次得解构了。

};
if (name) {
composeFn._name = name;
}
return composeFn;
}
}

interface DispatchOptions {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

之前放在里面就是希望是内部逻辑,不暴露到外面,其实也不需要定义了。

supportBody: boolean;
}

function dispatch(options: DispatchOptions): Promise<unknown> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

分离出来了的话逻辑就零散了,其实可读性和关联性会降低

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

嗯,这只是想看看性能上是否有明显差距。等 CI 过了你对比下性能

@waitingsong
Copy link
Member Author

node_modules/jest/bin/jest.js

node --require=ts-node/register ../../node_modules/jest/bin/jest.js --runInBand
这样终于能跑起来了~

@waitingsong
Copy link
Member Author

grpc 能跑单测, core 这儿还不行

node:internal/modules/run_main:129
    triggerUncaughtException(
    ^
Error: Cannot find module 'F:\F:\project\midway\packages\core\test\util\esm-fixtures\clz-default.mts' imported from F:\project\midway\packages\core\dist\util\index.js

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

Successfully merging this pull request may close these issues.

3 participants