Skip to content

Quick pass #1

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Quick pass #1

wants to merge 4 commits into from

Conversation

petehunt
Copy link

@petehunt petehunt commented Nov 7, 2013

Sweet demo! Changed a few things around :)

  1. I was concerned that perf was being sampled and logged so often that it would degrade performance. I switched to stats.js (the thing they use for three.js) and removed that code. Part of that removal included getting rid of the forced reflow. I wasn't sure what the point was of that so feel free to reject :) You can use stats.js in a more fine-grained way (as you were doing) too if you want (i.e. start() and end()).

Dropped this into js/ (which needs to exist for this to run)

  1. I refactored your code into a separate component which makes it a little more idiomatic.
  2. I multiplied perf by adding shouldComponentUpdate(). If your app was rendering more complex things this would help even more than it does right now. This is the magic of React: it's usually "fast enough" out of the box (it was on par or beating native on everything I tried) and with a single line of code you can increase perf by an order of magnitude.

Note that to do this you need to avoid mutation so I slightly tweaked datasource.coffee not to reuse objects.

@steveluscher
Copy link
Owner

Thanks Pete! I'll comb over the next time I work on the talk.

The forceReflow() dance was designed to profile the browser's reflow/relayout cycle. It goes something like this:

  1. Make a bunch of edits to the DOM. These edits get pushed onto a queue in the browser, to be processed next time the decision is made to reflow the document.
  2. Start the timer
  3. Query a style property of the body that would require the browser to flush the pending reflow queue in order to be able to give you an up-to-date answer (in this case, document.body.offsetHeight). This happens synchronously.
  4. Stop the timer

Voila, an accurate record of how long it took to reflow/relayout the document. For the purposes of my demo, this measurement demonstrates how much more work a naïve approach makes for the browser vs. React's more surgical approach to DOM edits.

@steveluscher
Copy link
Owner

Thanks for this once-over, @petehunt. The bit about shouldComponentUpdate has now been incorporated into the talk, along with the part about how writing idiomatic React code makes that sort of thing really easy.

Also, in case you were wtf-ing at my setZeroTimeout implementation (I see you replaced it with setInterval), it was there to squeeze every last ms of render time out of the browser. See here.

@petehunt
Copy link
Author

petehunt commented Nov 8, 2013

One other thing to note: if you're concerned about GC you can use object pooling to reduce the number of allocations (i.e. allocate two sets of objects and alternate between them).

React basically does this under-the-hood on the virtual DOM, but shouldComponentUpdate() reduce the number of comparisons you need to do.

@steveluscher
Copy link
Owner

Do you mean to allocate two sets of React.DOM.div instances (one for every data point in the visualization), and alternate between them?

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

Successfully merging this pull request may close these issues.

2 participants