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

Improve performance of swipe-up doc loading #9147

Merged

Conversation

zxhd863943427
Copy link
Contributor

  • Please commit to the dev branch
  • For contributing new features, please supplement and improve the corresponding user guide documents
  • For bug fixes, please describe the problem and solution via code comments
  • For text improvements (such as typos and wording adjustments), please submit directly

改进了动态加载时向上滑动的性能。能问一下v姐原来的写法有什么特殊考虑吗?

@Vanessa219
Copy link
Member

Vanessa219 commented Sep 9, 2023

你这个测试过么?

好像是需要移除一个再计算下一个的位置,否则位置高度计算不对。

@zxhd863943427
Copy link
Contributor Author

v姐你说的是什么测试?正常的滚动测试是没有问题,动态加载后还能保留在原地。

@Vanessa219
Copy link
Member

Vanessa219 commented Sep 9, 2023

这个看上去是有问题的,移除后某些情况 protyle.wysiwyg.element.lastElementChild.getBoundingClientRect().top 这个值会变化,PR 中是计算了再移除,结果就会和之前不一样。

@Vanessa219 Vanessa219 closed this Sep 9, 2023
@zxhd863943427
Copy link
Contributor Author

具体来说是什么情况?我看看能不能在修复一下,这个pr能增加100倍的性能

@Vanessa219
Copy link
Member

可能是 getBoundingClientRect 比较耗费性能,你在 protyle.wysiwyg.element.lastElementChild.remove(); 处打个断点,然后看 protyle.wysiwyg.element.lastElementChild.getBoundingClientRect().top 的值。然后再对比你修改后的代码看看。

@Vanessa219
Copy link
Member

还有就是移除完一个以后需要重新计算 protyle.wysiwyg.element.childElementCount > 2 && protyle.contentElement.scrollHeight > REMOVED_OVER_HEIGHT 看是否满足条件,有些块可能包含各种渲染,有的块比较长,比如长列表等情况。

@zxhd863943427
Copy link
Contributor Author

测试用的是什么文档?我使用了set来捕获获取的protyle.wysiwyg.element.lastElementChild.getBoundingClientRect().top,从结果来看是完全一样的。
图片

以下是对比的结果:
图片

@zxhd863943427
Copy link
Contributor Author

zxhd863943427 commented Sep 9, 2023

另外 v 姐,getBoundingClientRect实际上在原来的代码中并不是最耗时的,我的pr也使用了getBoundingClientRect,真正的问题是一次移除了一个节点后重新运行getBoundingClientRect,这会迫使浏览器每次移除节点都要重绘,导致时间暴增。如果一次性移除所有节点,那么就只会重绘一次。

@Vanessa219
Copy link
Member

Vanessa219 commented Sep 10, 2023

文档
test2.sy.zip

在这里断点:
image
日志:
image
无残余元素,移除内容完整:
image

在这里断点:
image
日志:
image
残余元素:
image

优化应该是可以的,把 scrollHeight 动态计算出啦,不每次都取。

@zxhd863943427
Copy link
Contributor Author

那这一部分是我没说清楚,我在尝试提升性能时发现把
图片
改成
图片
能够有效减少跳动,如果使用同样的计算就不会出现问题了。

@zxhd863943427
Copy link
Contributor Author

把乘二的系数去掉即可

@zxhd863943427
Copy link
Contributor Author

不过,我建议保留,没有系数的话,比有系数在一定情况下更容易跳动。

@Vanessa219
Copy link
Member

建议从尾部移除,这样可以计算更少。

@zxhd863943427
Copy link
Contributor Author

一次性计算并不是怎么耗时,使用console.time可以发现在1024个动态块的情况下,计算全部块的时间只需要20ms,加上移除需要100ms左右。如果从底部开始移除,并且每次移除后重新计算导致electron强制重绘,那么总共就需要9000ms左右。

@zxhd863943427
Copy link
Contributor Author

如果将我的pr改成反序遍历并在满足条件后跳出遍历,那么也许能节约一半的计算时间,但也可能将dom转成array这一步也有性能损耗,而且相较于原pr最多提升10%,总体性能提升也不算多。
但千万不要从底部一边移除一边计算,这是性能有如此大改进的重要原因。

@Vanessa219
Copy link
Member

好的,后面有空我再看看。

@Vanessa219 Vanessa219 reopened this Sep 11, 2023
@Vanessa219 Vanessa219 added this to the 2.10.6 milestone Sep 11, 2023
@88250 88250 changed the title Improved performance of swipe up Improve performance of swipe-up doc loading Sep 12, 2023
@88250 88250 modified the milestones: 2.10.6, backlog Sep 14, 2023
@Vanessa219 Vanessa219 merged commit 111608f into siyuan-note:dev Sep 18, 2023
2 checks passed
@Vanessa219 Vanessa219 modified the milestones: backlog, 2.10.6 Sep 18, 2023
@Vanessa219 Vanessa219 self-requested a review September 18, 2023 02:48
Vanessa219 added a commit that referenced this pull request Sep 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants