-
Notifications
You must be signed in to change notification settings - Fork 576
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
base: main
Are you sure you want to change the base?
Conversation
b6abac8
to
f942fed
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ 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. |
提升可读性?提出来就不能复用外部的一些对象了,不过性能估计影响应该不大。 |
不会的,diapatch 函数本身的定义是在预编译阶段完成,会提升的,所以不会重复初始化,只会将所在的闭包作用域(包含 index、supportBody 等变量)重新创建,性能开销不大的。 |
现在的 diapatch 函数是闭包定义的,不会全局提升,而是每个中间件进来时初始化一次且仅一次,我调试结果是这样的。 你可以这样试试看: if (typeof dispatch.foo === 'undefined') {
dispatch.foo = Date.now();
} 如果注册了两个中间件,那么会有两个不相同的 foo 值。 很久之前不晓得在哪儿看到,
|
调试看到的是堆栈,堆栈代表的是调用,定义预编译是指 v8 在读到 js 代码做的时做的优化,这个调试看不到的。这里的堆栈少不了的。 |
假定有中间件A, B
或者不管调试和堆栈,直接在 diapatch() 内部 console 打印 foo 值,也是两种。 |
函数没有实例的说法,函数就只有执行调用,函数本身就只有一个定义,在js runtime 解析阶段提升,在执行阶段创建函数上下文,diapatch 只是会多次执行,创建不同的上下文。 dispatch 放在里面,是因为希望函数执行限制在闭包内,这货也不会有外部来调用或者复用,js 里挺常用的。 |
这样来测试: const cache = new Set<unknown>(); // 顶层定义
const composeFn = (context: T, next?) => {
.....
....
cache.add(dispatch);
console.log('cacheSize', cache.size);
return dispatch(0);
} 控制台输出:现在的 cacheSize 会一直增加(我一个轮子跑单测能逐步增加到 96),而重构之后一直输出 1。 或者注册超大数量的中间件,比较下重构前后内存是否会暴增呢~ |
这个不会的,函数本身执行创建的开销很小,可能只占几百字节,即使创建96个实例,额外内存开销可能也就几十 KB,而且请求完就会释放,也不会涉及到泄露。 |
10w 次内存影响在 2%左右,耗时不明显,应该 gc 影响比较大。 |
刚才我也简单测试比较了下内存占用, rss, heap 各方面没有明显差异。 |
我再重构几处,然后你来测试下差异 |
一方面 v8 对闭包已经优化的非常好了,另一方面你的改动参数变成对象,还要做解构等,一进一出,几乎都抵消了。 |
d77d751
to
0bf9176
Compare
…rocessAsync() and processSync()
可以跑项目从profile来看top重点优化 |
我本地项目安装依赖
不明白,能否详细说下如何操作? |
我本地(win10)跑不了单测,很麻烦
|
哦,pnpm 装也是这样的报错,可以直接用 node_modules/jest/bin/jest.js 跑 |
} | ||
|
||
function dispatch(options: DispatchOptions): Promise<unknown> { | ||
const { i, context, newMiddlewareArr, name, next, supportBody } = options; |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
分离出来了的话逻辑就零散了,其实可读性和关联性会降低
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
嗯,这只是想看看性能上是否有明显差距。等 CI 过了你对比下性能
|
grpc 能跑单测, core 这儿还不行
|
No description provided.