Improve Performance of Sidebar resize by implementing requestAnimationFrame #942
Conversation
ghost
assigned 
|
Assigned to me |
|
I haven't tried this out yet, but I'm a little concerned about not resizing the editor window as you resize the sidebar--I think it will make it feel "non-native". Could we wait and see if this gets better with the new native shell instead of pulling this now? Or would it be easy enough to reinstate the live resize of the editor at that point? |
|
I'd give it a try NJ. I had the same concern, but while testing it, I didn't ever see a state where delaying the Editor.resize() made a huge difference. You've been in CodeMirror more than I have, but it seems like it's almost more a cleanup thing than anything else. The worst state I've gotten in is a white bar on the bottom but I've only gotten it to happen after moving the sidebar back and forth over and over and over again. And yeah, it's an easy fix to make. Seeing how bad it was on Windows I think it would be great to get this in now for basic performance reasons and then we can readress the Editor resize in the new shell. I've got to say though, that Editor resize is really, really expensive in terms of render events. Not something we can optimize without a ton of work, but worth thinking about. |
|
Ahh, just figured out how to get that state. When you drag the sidebar window to the right so that the editor window has a scrollbar, and then drag it back, a white bar appears where the scrollbar used to be. Kind of ugly, I agree, but I think the performance outweighs the ugliness for the time being. (Though I'm up for discussion on that) |
|
I'll take a look--might be fine for now. |
|
I get this weird flickering when I drag the sidebar close to the left edge to snap it shut. Do you see it? I also see the issue when sizing down the sidebar. In the extreme case with a very wide sidebar, as you shrink it down, the editor height never updates (the width is ok though). You end up with a blank region below the editor's horizontal scroll bar until the mouseup is complete. |
| @@ -88,8 +88,6 @@ define(function (require, exports, module) { | |||
| // event that we can just call from anywhere instead of hard-coding it. | |||
| // waiting on a ProjectManager refactor to add that. | |||
| $sidebar.find(".sidebar-selection").width(width); | |||
| $projectFilesContainer.triggerHandler("scroll"); | |||
| $openFilesContainer.triggerHandler("scroll"); | |||
There was a problem hiding this comment.
Do you think it's possible to change the shadow and triangle handlers to also use requestAnimationFrame?
There was a problem hiding this comment.
I took a look at it but I'm not sure how to differentiate that the scroll event is happening during a drag as opposed to a real scroll event. But the two events look like they have a totally different structure (one, I'm assuming being fired programmatically and the other from a real scroll event). But even if I can differentiate between those two events I'm not sure how to make sure it's a drag event and not another event that's calling .triggerHandler("scroll");
I'll do some more research.
|
Hmm, yeah, I see the flicker, I'll look into that. The other issue is because I'm not calling the EditorManager.resize() until the mouse-up. That's a really, really expensive call so trying to do it even on requestAnimationFrame seemed pretty brutal. But let me throw it in there and see how it responds on windows (where the main performance hangup is) |
|
Reviewed |
|
I think I've nailed down the flashing bug and I'll be updating the pull request this week. I'm still looking into the feasibility of changing the scroll events. I actually couldn't find where they were firing over the weekend but only spent a little bit looking at them. |
|
This should fix the issues talked about above. @jason-sanjose I don't think it's possible to move the shadows into requestAnimationFrame as it's set up now, but I think that's partly a limitation of my understanding how rAF would behave when running twice in two different ways. Something I need to look into but for now I think this checkin improves performance enough (especially on Windows) that it's worth merging. I tried once again to rebase this to remove the merge but things went horribly, horribly wrong so it's got that dirty commit in it. |
Forgot to check JSLint - made small fix
… into sidebaroptimization
|
Reviewing latest changes |
| if (newWidth > 10) { | ||
| forceTriangle = true; | ||
| if (newWidth > 10) { | ||
| forceTriangle = true; |
There was a problem hiding this comment.
forceTriangle is no longer used. Was that intentional?
There was a problem hiding this comment.
Yeah, because right now I'm hiding everything when we move (triangle, shadows). But forgot to remove this. Fixing it in the next pull .
|
A few minor comments. Also, it looks like double-click to toggle open/closed is broken now. The sidebar will close, but it doesn't reopen correctly on double-click. |
…l" events on double click show
|
Sorry for all the commits. I had to test it out on Windows and the only way I could do it was push my repo and pull it from my PC. But these fixes should address all the issues. Thanks @jason-sanjose . |
|
Sorry, Ryan. Somehow I missed the github notification. I'll review today. |
|
No worries, it was over the weekend. Let me know if there are any spots I need to clean up. Thanks Jason. |
Improve Performance of Sidebar resize by implementing requestAnimationFrame
This pull request addresses the issue of sidebar resize performance. I used requestAnimationFrame to make sure that Brackets optimizes when it does repaints (on the browser cycle instead of trying to do it on every mousemove). I also made a couple of other changes to make that process more refined:
With these changes it's super fast on Windows and a bit snappier on Mac as well. CPU usage however is unaffected and I think that's a CEF issue.