-
Notifications
You must be signed in to change notification settings - Fork 104
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
Workaround for issue #120 and a bug of tag filter #124
Conversation
}else if(this.state==='works'){ | ||
return ( | ||
this.pxer.taskList.filter(pr=>pr.completed).length |
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.
问题是不是只是这个filter花费太久呢?我没有遇到你那样卡的情况,但我看了下profile如果只把这里拉出来论询问题就会缓解很多
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.
是的,即使仅将taskList变成轮询也可以缓解很多。不过既然都拉出来一个了为何不做的彻底一些呢?所以我将所有会超快速变化的值都搞成轮询了,其余直接绑定的引用也是最小化数量。
src/view/vm.js
Outdated
@@ -40,26 +61,26 @@ afterLoad(function(){ | |||
}, | |||
showLoadBtn:true, | |||
errmsg:'', | |||
analytics:new PxerAnalytics(), | |||
analytics: {}, |
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.
这里是干嘛的?不用就直接去掉吧
ptmConfig: pxer_app.ptmConfig, | ||
}; | ||
|
||
setInterval(function() { |
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.
可不可以用getter / setter 来完成 pxerApp 与 给vue追踪的 pxerApp 的双向绑定呢?这样可以响应式更新而不是 setInterval 隔一段时间一更新。
我猜,Vue 的 vm.$watch 可以帮助更快更好的完成这个工作
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.
大多数不频繁更改的变量都是用的getter/setter。只有completedTaskCount、failCount、worksNum是轮询,因为这三个变量更改极为频繁且每次更改计算量较重,用轮询无论是写起来还是性能上都更适合些。
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.
你是说这样是在用 getter 和 setter 吗?这个和我所知道是用法有些出入,我很很怀疑他是否能正常工作。
周末我会尝试运行一下
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.
我试了一下,可以运行但整个界面显得很卡,为了减少cpu占用而让整个界面变得很lag我不是很能接受
我使用react重写的vm做到了及时更新+fps~60
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.
我觉得这种情况可以参考一下我在 #125 里的方案:给pxerapp增加tick事件,绑定一个监听器来同步
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.
相比较的话,PR原有的方案:
- 解决的更彻底
- 成本小,代码更直观
- 会让UI显的很卡
- 不太优雅
而使用 syncValue 函数:
- 当前版本解决的不够彻底
- 可以进步一步解决性能问题。理论上可以达到和PR中原有方案一致
- 成本较大
而 @eternal-flame-AD 的方案 —— 增加一个 tick 事件通知 Vue 更新视图
- 成本很大,需要找到所有需要更新视图的地方手动 emit 一个事件
- 易出错,某个地方若漏掉 emit 事件,难以被测试
- 不太优雅
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.
其实如果觉得完成数更新的卡可以把第一个setInterval时间设置成100,对性能没什么明显影响。
不过我认为没什么必要,毕竟那个数量也不会总盯着看。
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.
这样会不会导致vue又盯上pxer_app.taskList里所有元素呢?vue最累的追踪应该就是这部分
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.
看起来这个方案只是剥离掉了pxerapp里不直接跟视图有关的props,但真正大幅影响性能,快速变化的props还是被保留并被跟踪了
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.
我分析导致性能问题的主要原因是在taskList被追踪的时候,taskList里自己的属性也在快速变化——PxerHtmlParse.getUrlList改动url,task.html先被ptm初始化,xhr完成后又被填充,这些都是通过vue的reactiveSetter而且会触发这些computed的重新计算,而这些更新完全是没必要的
这个PR包含两个修复
针对Issue #120 的缓解方案
症状:见Issue #120
修复:将PxerApp和PxerAnalytics从Vue对象移出。将UI显示的变量放入pxer_app_delayed_syncer对象给Vue。worksNum、completedTaskCount、failCount与PxerApp定时轮询同步,其余需要的变量直接引用。
于Edge 42, Chrome 68, FF 61测试爬取50000个未再发现该问题。
针对Tag Filter的Bug修复
症状:在 作品中必须含有以下任意一个标签 或 作品中必须同时含有以下所有标签 中填写内容后清空会导致结果数量永远为0。
修复:因为"".split(' ')结果为[""],所以检测value为""就直接赋值[],不进行split