Trying to improve rendering performance

[michaelrambeau]

Using the default grid (20 * 20 = 400 cells), it seems that there are some rendering performance issues. For example, if you try to draw a line quickly by drag and drop, some pixels are missing on the screen. By moving slowly the mouse, it is OK.

In addition, after having made that line, even if I don't drag any more, pixels are still added on the grid when I simply move the mouse over the grid (I can see in the console that dragging flag is still set to true).

Note: in production, the performance issues are less obvious than in dev because of several optimizations that make the code run faster. Redux dev tools for example are known to slow down things a lot (a lot of information is logged in the console after every action).

Measurements (using React Perf Chrome plugin)

Let's check the rendering process by measuring time before and after rendering a single pixel on the grid.

Total time is not only the same, it is around 500 ms, which is too much time for a single pixel. I can see that Connect(PixelCell) is "wasted" 799 times!

Using a 2 times bigger grid (40 * 20) Connect(PixelCell) is "wasted" 1599 times.

Let's try to fix that!

[michaelrambeau]

Optimization step 1

The Cell component, used to render every pixel on the grid, should be as simple as possible. Instead of connecting it to the state, let's connect its parent component (the grid) using props to pass actions and context data.

This is much better, no more 799 occurrences wasted, but it is still slow. We can do better.

First let's analyze what happens when a cell is clicked (to be filled with the current color). 3 actions are dispatched:

• SET_GRID_CELL_VALUE
• START_DRAG
• END_DRAG

Adding some log in the Grid component, I can see that the Grid is rendered twice to render a single cell. Can we fix that ?

[jvalen]

Thanks a lot @michaelrambeau! Optimization step 1 is a great performance improvement.

Related to the 3 actions dispatched situation, just to clarify:

1. SET_GRID_CELL_VALUE: give a color to the cell clicked.
2. START_DRAG: Once we click, we start dragging, meaning that until we release the button we keep giving colors to the cells we move the cursor over.
3. END_DRAG: Dispatched when we release the button after dragging.

I should review this mechanism, because looks kind of hacky... The problem is in the handleDrag method, inside PixelCell. You can see the related commit here: https://github.com/jvalen/pixel-art-react/commit/0404f01eedbbee1d232e73985270edf0e7e4d825#diff-fcf8f1d2ccbb63938fdda8e82c871866

The reason why I did that is related to undo history topic.

The original way of doing that was the following:

handleDrag(event) {    
    if (this.props.dragging) {   
      this.drawCell(event);   
    }
  }

The problem with the code above was that because we are only storing the following actions dispatched:

const store = createStore(undoable(reducer, {
  filter: includeAction([
    'SET_STATE',
    'SET_GRID_DIMENSION',
    'SET_GRID_CELL_VALUE',
    'SET_DRAWING',
    'SET_CELL_SIZE',
    'SET_RESET_GRID'
  ]),
  debug: false
}));

When we go back, we remove the last pixel draw but the state contains the dragging property to true so it keep dragging.

The current way of solving that issue is:

handleDrag(event) {
    if (this.props.dragging) {
      this.props.endDrag();
      this.drawCell(event);
      this.props.startDrag();
    }
  }

Which thanks to you I realized is not good for performance and we should find a better way of doing that. I don't like having the dragging attribute inside the state, it might be the root of the evil.

If you have any suggestions please let me know.

Thanks again!

[michaelrambeau]

Thank you @jvalen for the explanation. I will go back to the drag and drop stuff later, for now I am doing optimizations that are easy to implement.

Optimization step 2: make the cell mark-up minimum

In the previous version, every grid cell require 2 <div>.

We can make the mark-up simpler, using only one div per cell... and "save" the creation of 400 DOM elements (you cannot imagine the amount of things that the browser creates for a simple <div>) !

I used CSS flexbox to get that result.

.grid-container{
  display: flex;
  flex-wrap: wrap;
}
.grid-container .grid-cell{
  border: 1px solid #585858;
  border-width: 0 1px 1px 0;
  color: white;
}

Inline style applied on every cell:

const styles = {
    flex: `0 0 ${width}%`,
    paddingBottom: `${width}%`,
    backgroundColor: `#${selectedColor}`
};

Impact on performance: it seems the grid render 2 times faster than before (95 ms instead of 185 ms for Connect(Grid) )

Sounds good but we still can do better!

[jvalen]

That is also a great improvement, thank you @michaelrambeau.

[michaelrambeau]

Optimization step 3: render the grid only once

• Avoid unnecessary grid rendering, using shouldComponentUpdate
• Move event mouse handler from the <Cell> to the <Grid> component

  shouldComponentUpdate(nextProps) {
    const same = (key) => { return nextProps[key].equals(this.props[key]); };
    if (same('grid')) {
      // console.info('No need to render the grid!');
      return false;
    }
    return true;
  }

it works: when a single pixel is clicked, the grid is rendered only once. Grid component disappeared from the "wasted time" list.

We still have work to do because drawing a line is not smooth yet, we should try to empty the list of "wasted" components. Every time a single pixel is clicked, the whole application is rendered, I am going to check if we can optimize this behavior.

[michaelrambeau]

Tracking reducer performance

An other idea: checking if reducers are fast using a tool like redux-perf-middleware to see how much time it takes to update the state when an action is dispatched.

For a single click that generated 3 actions, state updates by the reducer take a total of about 30ms. (but the number fluctuate a lot between tests) That is pretty slow for pure JavaScript functions; we cannot expect the app to run in 60 FPS ( = 16 ms per frame!)

[jvalen]

Thank you @michaelrambeau, your performance improvements are merged and up in production.

I'm going to keep this issue opened at least until the drag and drop topic is solved :smile: .

[Mayurkoli]

How i learn this

[jvalen]

I'm closing this issue since it's been a long time with no activity. We might reopen it in the future to talk about performance improvements.

Thanks!