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 #14

Open
2 of 7 tasks
giuband opened this issue Sep 11, 2016 · 13 comments
Open
2 of 7 tasks

Improve performance #14

giuband opened this issue Sep 11, 2016 · 13 comments
Milestone

Comments

@giuband
Copy link
Collaborator

giuband commented Sep 11, 2016

Much can be done to improve overall performance, some ideas:

  • When moving the map, compute the position for only the points who will be visible
  • Computation of points coordinates through tsne should happen in a separate worker
  • Some Paths components subscribe to state.sounds.byID, which means they will frequently re-render
  • Use React.Perf to identify other causes of unneeded renderings
  • Space thumbnails probably re-render while moving the map (when tab is shown)
  • Use react-virtualized on heavy tabs (like spaces one)
  • Refactor CSS code for sidebar
@noVaSon
Copy link
Collaborator

noVaSon commented May 3, 2018

@ffont: >

I'm not sure how this could be helped, but the very first implementation of FSE used raw HTML canvas and it was blazing fast. Maybe we could try with a library build on top of HTML canvas (for example, I tried Fabric.js at some point and it seems to work fine) to draw the circles and see what happens. Maybe using one of these libraries the code changes necessary to experiment with that would not be that big. In any case this is just an idea for future.

--
This was copied from #53

I measured a lot during the last days.
The most performance hungy functions where:

  • perform (React has a lot to re-render, maybe this could be optimized, I just failed until here)
  • computeTsneSolution - maybe try a webassembly solution? (only when building a new space)
  • paint - I read tricks about putting the svg inside a container and moving this instead of the

I tried to try HTML-GL but failed here, too. It also seemed to not feature retina displays...
I considered tackling this when all critical features for my thesis are implemented.

Question - why don't we use the canvas from the beginning, if it is so fast?

@noVaSon
Copy link
Collaborator

noVaSon commented May 3, 2018

I managed to reduce load in the soundlist PR #46, as the whole table was re-rendering with each map move. Just used shouldComponentUpdate and a deep comparision.

But for the spaces tab I didn't figure it out yet. Tried to set shoudComponentUpdate to false everywhere, but it still updated...

@ffont
Copy link
Owner

ffont commented May 3, 2018

The computeTsneSolution part I don't think it needs to be optimized for now as this is only done at "load results" time. However once the map is computed, navigating and moving it is quite slow if there are many sounds. I think the real deal here is the drawing part (although I have no data to support that).

We moved away from canvas because the interaction with canvas elements was rather limited (even though we never tried a library on top of HTML canvas). Also SVG seemed better because vector graphics bla bla bla.

@giuband do you have any ideas what could we do here? Would it make sense to try some canvas-based visualization for the map? What would be the best approach to try something like that? (if it makes any sense at all).

@giuband
Copy link
Collaborator Author

giuband commented May 3, 2018

Migrating back to canvas would give the greatest positive impact on performance, as it's much more performant than SVGs when there are thousands of elements. But it would take quite a lot of time and probably some of the things will have to be hacked in order to work as they do now (like hovering or clicking on elements).

Other things that can be tried without being that complicated and time demanding:

  • using CSS transform: translate3d() and transform: scale3d() on the entire SVG element instead of the single circles when moving around the map or zooming on it. It could get a bit tricky but I'd give it a try.
  • getting a bit creative with optimization. For instance, while the user is moving the map only a subset of points could be shown. Or even some clustering technique could be tried out. The ultimate goal in both cases would be of drastically reducing the elements being rendered in the document.

Honestly, I'd try with all of them and see how feasible they are. The third option is the easiest one and I'd start from that just to have some benefit in the short term.

@noVaSon
Copy link
Collaborator

noVaSon commented May 3, 2018

A search in Chrome with 300 resulting sounds results in a max 185 ms frame movement (5.5 fps!) when dragging the map.

A short measure shows:
roughly 7.5% of calucation time a frame is caused by Recaluclate Style (in CSSPropertyOperations)
12.8% is caused by the sound byID reducer.js L84 (UPDATE_MAP_POSITION)
82% is caused by React/Redux state updates (connect)

I think we'd need to improve the whole thing about state changes.
Styles and painting will only save 20 ms per frame (which is definetely noticeable...)

screenshot 2018-05-03 16 57 34

screenshot 2018-05-03 16 57 15

@ffont
Copy link
Owner

ffont commented May 3, 2018

I like your ideas @giuband. To start we should try "randomly" hiding some objects when moving the map to see how performance is affected. To do it less randomly we could hide the objects which are far from the mouse pointer.

Another idea, we could replace the whole map for a rasterized version of it when moving. I guess this is lot of work but I think this is how PDF viewers do it for example.

@noVaSon
Copy link
Collaborator

noVaSon commented May 3, 2018

Challenging isn't it? :)

@ffont
Copy link
Owner

ffont commented May 3, 2018

That’s the spirit!

@noVaSon
Copy link
Collaborator

noVaSon commented May 3, 2018

My first and quickest idea:
use a debounce in the MapCircleContainers to avoid the unnecessary plethora of re-renders caused by the events. Something like this should work: https://www.npmjs.com/package/react-debounce-render

@ffont
Copy link
Owner

ffont commented May 4, 2018

Sounds like a good idea, although I guess the problem is that circles receive too many updates of x, y positions and using debouncing might stop drawing them until the end, but it will be an interesting idea. Another idea, could we “sample” the solution of the tsne at a lower sampling rate (eg, 250ms) and animate the circles from one position to another (interpolate)?

@noVaSon
Copy link
Collaborator

noVaSon commented Feb 6, 2020

If it is even possible with the React Library, it could dramatically improve animation performance to use the object pool design pattern for the Circle Objects. Currently we are re-instanciating each circle each frame of movement. Don’t know how to achieve this yet. Maybe with the use of the react lifecycle methods.

@ffont
Copy link
Owner

ffont commented Feb 7, 2020

Hey, welcome back! :)
The project has not evolved since your latest changes @noVaSon, but it would be awesome to improve performance :)
Any comments @giuband ??

@giuband
Copy link
Collaborator Author

giuband commented Feb 10, 2020

Can't check this out properly at the moment, but keep in mind that using the key prop properly is the way to let react create an unambiguous one-to-one bound between component and dom element, so that react won't create again a new dom node every time the component updates. I think we are already doing this correctly in this repo by giving each circle the id of its sound as circle, aren't we? In that case react is not remounting the circles everytime.

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

3 participants