Skip to content

feat: Optimize performance with dirty flag - #7

Closed
Initsnow wants to merge 2 commits into
madderscientist:mainfrom
Initsnow:feat/dirty-flag-optimization
Closed

feat: Optimize performance with dirty flag#7
Initsnow wants to merge 2 commits into
madderscientist:mainfrom
Initsnow:feat/dirty-flag-optimization

Conversation

@Initsnow

Copy link
Copy Markdown

减少空闲时的CPU usage

@netlify

netlify Bot commented Jul 27, 2025

Copy link
Copy Markdown

Deploy Preview for notedigger ready!

Name Link
🔨 Latest commit 8a21f6d
🔍 Latest deploy log https://app.netlify.com/projects/notedigger/deploys/6885f848f4f834000848ea64
😎 Deploy Preview https://deploy-preview-7--notedigger.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@madderscientist

Copy link
Copy Markdown
Owner

感谢你的优化。毫无疑问你的pr能减轻cpu负担,cpu负担也确实主要来自每帧的重绘,但出于以下原因我觉得合并需谨慎:

首先,当前修改中的置位并非最佳位置,且存在重复。你的修改集中于用户操作后,导致很多地方都需要置位。然而多处存在重复置位的情况,比如161行的 scroll2 中已经设置为 true,但下一行又跟了一个。类似的地方不止这一处,也就是你选择的位置有冗余。这是因为我的代码过于耦合。在不重构的前提下,精准找到最小需刷新的操作集合颇为困难。

其次,多处增加dirty导致维护困难。曾经我也考虑过并非帧帧需要重绘,但每加一个功能就要溯源判断是否已经会刷新 非常累,若忘了dirty设置还会有bug,所以我没有优化这一点。类似的地方是MidiAction.updateView,我选择了用户操作后才刷新,当时代码就写得非常累。如果将类似的操作拓展到整个应用的刷新,我不敢想象以后有多难维护。

再者,我并不认为cpu占用多有什么很大的影响。如果页面显示,说明用户要操作,高点就高点吧。如果页面被遮住不可见,则重绘会自己暂停,并不再占用。

最后是我认为如果要改有更好的做法。以下设想择其一即可:

  1. document增加鼠标按下、按下后滑动、松开回调,回调中设置app.dirty=true(解决用户操作带来的视图变化),在播放时强制刷新(应用自己动的时候的视图变化)。这也许是最简单最可维护的做法。
  2. 给每个组件单独设置一个dirty,能让dirty更成体系,后续更好维护。
  3. 给所有状态量设置hash,每一帧比较hash决定是否重绘。

将刷新分为四个部分
- resize
- 用户操作(鼠标键盘等)
- 分析完成(Spectrogram setter; CQT结束)
- 播放时(AudioPlayer.update)
@madderscientist

Copy link
Copy Markdown
Owner

按照设想1更改了一下,看起来能用,但不知道有没有bug
我依然倾向于不修改(这个项目的代码实在太琐碎了,不敢轻举妄动)
如你有什么想法或建议,欢迎提出。如果没有,麻烦你再关闭一次pr

@Initsnow

Copy link
Copy Markdown
Author

None

@Initsnow Initsnow closed this Jul 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants