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

Drag performance degrades when ghosts/insertion markers are being created/removed #200

Closed
tmickel opened this issue Apr 11, 2016 · 13 comments

Comments

@tmickel
Copy link
Contributor

tmickel commented Apr 11, 2016

@picklesrus @rachel-fenichel

On my device it seems like drag performance suffers a lot when we're adding/removing ghosts - i.e., dragging on top of blocks, things get very slow.

How could we improve this?
-Is the whole stack being re-rendered, could we avoid re-rendering most of the blocks (I guess except parent blocks, in case we need to stretch a pants?
-Is it the rendering of the ghost block itself, in which case could we cache this?
-Is it just a timing issue, in which case could we fix it by using requestAnimationFrame to update the drag position and/or draw the ghosts?

@tmickel tmickel added this to the Google I/O milestone Apr 11, 2016
@picklesrus
Copy link
Contributor

I'd be happy to take a look. What's the best way to get a look at the timeline? You could save it and send it along or let me know what you're looking at (and html file in chrome? your apk?). I haven't actually checked out and run scratch-blockly yet. Maybe it is time!

@tmickel
Copy link
Contributor Author

tmickel commented Apr 12, 2016

I've been loading the horizontal-playground on mobile devices. Happy to send a timeline if that makes it easier for you although I need to figure out how to do that :)

@picklesrus
Copy link
Contributor

Cool. You could make a timeline by connecting your device to your computer, going to chrome://inspect#devices, inspecting the playground and using chrome dev tools from your laptop. But then you have to save it, upload it somewhere I can access it. Seems not worth it since "git clone" is way easier :) I want to take a look at #207 too.

I'll check out the project to take a look, but I need to fix a bug first. Hopefully I'll do it this afternoon.

@tmickel
Copy link
Contributor Author

tmickel commented Apr 12, 2016

As a side note, if we can avoid repaint in #207, maybe we'll make the frame budget without further adjustments...

@picklesrus
Copy link
Contributor

Sorry, got back to this later than I was expecting.

Looking at the timeline, there aren't any obvious things chrome is flagging, although it is giving me less information about the js being called than I'm used to. The seemingly sporadic longer mouse move events are taking 5ms, which is kinda high, but it mostly maintains 60fps on my macbook air. I'll have to figure out why it no longer gives me detail.

As for painting, I can't quite figure out its pattern. Sometimes it just repaints the blocks, sometimes it repaints the whole workspace, including the trash, etc. I wonder if we're somehow calling workspace resize? Here's a video on my laptop: https://www.dropbox.com/s/t120lh6oxmy8gil/GhostPaints.mov?dl=0

I'm going to go look at #207 before digging in more here since I think there might be an exciting update there :)

@rachel-fenichel
Copy link
Collaborator

Now that we have tim's drag surface, we can probably get rid of https://github.com/LLK/scratch-blockly/blob/develop/core/block_svg.js#L1029

which would let us avoid one extra call to render().

It's currently there because otherwise the insertion marker would end up on top of the dragging blocks.

@tmickel tmickel modified the milestones: Backlog, Google I/O Apr 20, 2016
@tmickel tmickel modified the milestones: June 23, Backlog May 12, 2016
@tmickel tmickel modified the milestones: June 23, July 21 Jun 22, 2016
@thisandagain
Copy link
Contributor

Depends on #154

@picklesrus
Copy link
Contributor

I'm taking a look at this independent of #154 for now since there seem to be some improvements I can make that are independent of the workspace drag. For reference, here is a screenshot of a timeline I took where I dragged a single block along a stack. It is quite clear where the insertion markers show up/disappear on the top portion of that image and the bottom portion is one of those zoomed in.
screen shot 2016-07-28 at 11 37 36 am

@tmickel
Copy link
Contributor Author

tmickel commented Jul 28, 2016

@picklesrus Do you think improvements purely here could bring us under frame budget? It was my impression that the improvements in #154 caused this to slow down even more (more painting)? Ofc it does look like a lot of time is being spend in the screen CTM...

@picklesrus
Copy link
Contributor

For dragging blocks around? Maybe. The timeline above is without #154. The version with #154 looks similar. The problem is really the repeated calls to render which in turn call getScreenCTM. I need to look into the reason updateInverseScreenCTM keeps getting called. I don't believe we should have to call it that often.

@tmickel
Copy link
Contributor Author

tmickel commented Jul 28, 2016

Oh, ok, cool! I had assumed that the version with #154 looked substantially different, but I haven't done as much investigation as you have. Cool, that's encouraging :)

@thisandagain thisandagain modified the milestones: September 22, August 18 Aug 22, 2016
@tmickel tmickel modified the milestones: September 22, October 20 Sep 21, 2016
@tmickel tmickel modified the milestones: October 20, September 22 Sep 21, 2016
@thisandagain thisandagain modified the milestones: November 14, October 20 Oct 24, 2016
@thisandagain thisandagain modified the milestones: December 12, November 14 Nov 15, 2016
@thisandagain thisandagain modified the milestones: December 12, January 19 Dec 6, 2016
@picklesrus
Copy link
Contributor

I dug into this again today and realized the we fixed main offender mentioned earlier by caching the result of getScreenCTM. The blockly commit was google/blockly@c4cad3c.

screen shot 2017-01-04 at 4 33 56 pm
Attaching a screenshot of the timeline captured today, you can see all of the red triangles are gone and the javascript execution is down to ~6ms. There is still the recalculate style, layout, and paint to consider in the frame budget, but it is, at least on my macbook air, a "green" frame in chrome. The repaint area is also pretty minimal now - only the stack of blocks that is changing rather than the entire workspace.

I also looked at the frame rate while dragging a block into a stack running a small animation on https://llk.github.io/scratch-vm/ and it feels much less janky than I remember.

There isn't any immediately obvious low hanging fruit here anymore. I believe more improvements would involve significant changes to how we handle shifting blocks when a new block (or stack) is inserted. Currently, I don't think the amount of work required is worth any improvements we could squeeze out (probably only a few more ms.)

@thisandagain
Copy link
Contributor

@picklesrus Thanks for the update! Based on this I think we could close this for now and re-open should it continue to be an issue in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants