Performance with many markers #224

rewop · 2016-03-08T17:39:50Z

This PR solves the performance problems on the map, when rendering many markers.

Problem

The problem is that every component in the module needs a reference to the mapHolder instance to perform some operation on the google map.

GoogleMapHolder passes itself down to the children using props, but to do so it always needs to clone all the children that is rendering.

So if you render many markers, GoogleMapHolder clones all the markers every time its render function fires.

Solution

To solve the issue, I place the mapHolder instance in a context instead. This way all the components down the tree can access the mapHolder instance just by requesting it from the context (code for the marker).

This way the cloning of each child is not needed and the performance are (almost) the same if you used the plain google map library.

An example is the following:

import React, { Component } from "react";
import { GoogleMapLoader, GoogleMap, Marker, OverlayView } from "../../../../../../../../Development/react-google-maps";
// import { simplified } from "../../knd-elements/GoogleMapsThemes";

const STYLES = {
    mapContainer: {
        height: "400px"
    },
    overlayView: {
        background: "white",
        border: "1px solid #ccc",
        padding: 15
    }
};

function getRandomInRange(from, to, fixed) {
    // .toFixed() returns string, so ' * 1' is a trick to convert to number
    return (Math.random() * (to - from) + from).toFixed(fixed) * 1;
}

function createLatLng() {
    return {
        lat: getRandomInRange(-90, 90),
        lng: getRandomInRange(-180, 180)
    };
}

function createMarker(index) {
    return {
        position: createLatLng(),
        index
    };
}

function getMarkers() {
    let markers = [];
    for (let i = 0; i < 3000; i++) {
        markers = [
            ...markers,
            createMarker(i)
        ];
    }
    return markers;
}

class MapTest extends Component {

    constructor() {
        super();
        this.state = {
            markers: [],
            loaded: false
        };
    }

    componentDidMount() {
        const markers = getMarkers();
        this.setState({
            markers,
            markerComponents: markers.map((marker, index) => (
                <Marker
                    {...marker}
                    onClick={() => this.onSelectMarker(marker)}
                    key={index}
                />
            ))
        });
    }

    onSelectMarker(marker) {
        this.setState({
            selectedMarker: marker
        });
    }

    onCloseOverlay() {
        this.setState({
            selectedMarker: null
        });
    }

    render() {

        return (
            <GoogleMapLoader
                containerElement={
                    <div {... this.props} style={STYLES.mapContainer} />
                }
                googleMapElement={
                    <GoogleMap
                        defaultZoom={7}
                        defaultCenter={{
                            lat: 52.3702157,
                            lng: 4.8951679
                        }}
                    >
                        {this.state.selectedMarker &&
                            <OverlayView
                                position={this.state.selectedMarker.position}
                                mapPaneName={OverlayView.OVERLAY_MOUSE_TARGET}
                            >
                                <div style={STYLES.overlayView}>
                                    <h1>OverlayView</h1>
                                    <p>Marker: {this.state.selectedMarker.index}</p>
                                    <button onClick={() => this.onCloseOverlay()}>
                                      Close me
                                    </button>
                                </div>
                            </OverlayView>
                        }
                        {this.state.markerComponents}
                    </GoogleMap>
                }
            />
        );
    }
}

Each click on the marker on this map, takes more than 3 seconds to update it. This pr aims at solving this problem, and dramatically improve the performance issues.

Resolve #135

Now GoogleMapHolder just renders children, and defines a context to provide the reference to itself. In this way, to request the original google map obejct, it is only necessary to request it from the context. This improves rendering of 3000 markers by 3 seconds.

…his incresases the performance of the map, when rendering many markers

cristiandley · 2016-05-13T12:55:10Z

Why is this not merged yet ?

vinceprofeta · 2016-05-16T12:58:55Z

+1

Performance many markers

vinceprofeta · 2016-05-16T13:51:19Z

@rewop after taking a deeper look into your solution, I noticed that if you wanted to update the marker in any way (size, color, icon, etc) you cant. I would suggest maybe being able to pass in your own function to CDU and that way you can control all updates based on which features you may want. You could also do a comparison of each marker, but that may also have performance issues.

rewop · 2016-05-16T14:36:17Z

@vinceprofeta I worked on this PR quite long ago, and I need a moment to catch up with the changes. I see your point, and it is indeed a good one. Do you have an example that I can use to reproduce the use case? I will work on this asap.

vinceprofeta · 2016-05-16T15:59:19Z

 const markers = _.map(locations, (location, i) => {
    const icon = this.determineIcon(location, i);
    return (
      <Marker
      key={location._id+'-marker'}
      defaultAnimation={2}
      zIndex={icon.zIndex}
      icon={{
        url: icon.icon,
        size: new google.maps.Size(icon.size,icon.size),
        scaledSize: new google.maps.Size(icon.size,icon.size)
      }}
      position={{lng: location.Location[0], lat: location.Location[1]}}
      {...location}
      onClick={this.onMarkerClick.bind(this, location)}
      onMouseEnter={this.hover.bind(this, location, true)}
      onMouseLeave={this.hover.bind(this, false)} />
    );
  })

determineIcon(location, i) {
  const activeId = this.props.activeLocationId === location._id
  if (activeId) {
    return {
      icon: location.OnOffPremise === 'On Premise' ? 'src/images/barrellActive@2x.png' : 'src/images/barrellCartActive@2x.png',
      size: 110,
      zIndex: 990
    }
  } else {
    return {
      icon: location.OnOffPremise === 'On Premise' ? 'src/images/barrell@2x.png' : 'src/images/barrellCart@2x.png',
      size: 67,
      zIndex: i
    }
  }
}

vinceprofeta · 2016-05-16T16:00:56Z

@rewop within render I have a loop to create the markers and within the loop I have a function to determine whether that icon should be the active icon (bigger and a different color). For your example you could just adjust the size of the marker.

vinceprofeta · 2016-05-16T16:02:17Z

@tomchentw Do you have an opinion on this? Making this update significantly helped performance, and would love to get a fix in production.

fork merge

cristiandley · 2016-05-24T13:52:49Z

@vinceprofeta @rewop i needed some features to quickly put them into development/production so i created a repo based on this one. https://www.npmjs.com/package/react-gl-maps if you want to pull this i will be glad.

rewop · 2016-05-24T15:38:50Z

@cristiandley thanks. I have had a really busy week at work, and I couldn't put myself into it yet. I hope by the end of this week to manage.

rewop · 2016-05-25T15:50:08Z

NOTE: I updated the description to better explain my solution here.

tomchentw · 2016-05-29T09:36:35Z

@rewop Thanks for all the hard work for this. Much appreciated.

Would you please revert the changes made to lib files so that this PR only reflects changes in src? That would reduce the noise around the code.

Also, why adding shouldComponentUpdate to Marker component?

tomchentw · 2016-05-29T09:36:55Z

src/Marker.js

@@ -88,6 +99,10 @@ export default class Marker extends Component {
    }
  }

+  shouldComponentUpdate() {


Why adding shouldComponentUpdate to Marker component?

At first this was my approach to try fix performance, that is why I implemented it. However, later I didn't remove it because it because I thought that the marker doesn't need to be updated, unless it is completely replaced.

However for the sake of performance, this shouldComponentUpdate is not needed.

@tomchentw what do you think? I can push a change for it.

Please remove this for this PR. I want to implement sCU for components in another pull requests, thus making changes as small as possible.

rewop · 2016-05-29T10:40:49Z

@tomchentw I reverted the lib folder. I am waiting for your answer about shouldComponentUpdate in Marker component.

rewop · 2016-05-29T10:57:17Z

@tomchentw all changes done

tomchentw · 2016-05-29T11:12:40Z

src/Marker.js

@@ -14,6 +15,10 @@ import {
  markerEventPropTypes,
 } from "./creators/MarkerCreator";

+import GoogleMapHolder from "./creators/GoogleMapHolder";
+
+import _ from 'lodash';


Please remove lodash for now.

tomchentw · 2016-05-29T11:12:57Z

package.json

@@ -84,6 +84,7 @@
    "can-use-dom": "^0.1.0",
    "google-maps-infobox": "^1.1.13",
    "invariant": "^2.1.1",
+    "lodash": "^4.12.0",


Please remove lodash for now.

tomchentw · 2016-05-29T11:14:25Z

@rewop thanks a LOT! Just one more thing to do then we're all set!

rewop · 2016-05-29T11:28:42Z

@tomchentw done the changes. I also linted, to make sure I didn't miss anything. Let me know if more is needed.

tomchentw · 2016-05-29T11:37:40Z

package.json

@@ -84,8 +84,6 @@
    "can-use-dom": "^0.1.0",
    "google-maps-infobox": "^1.1.13",
    "invariant": "^2.1.1",
-    "lodash": "^4.12.0",
-    "lodash.isequal": "^3.0.4",


Oh no. You have to keep lodash.isequal

Oh. I knew I had to keep it. Strange it was removed because I double checked. Must have made a mistake. Going to fix it now.

tomchentw · 2016-05-29T11:38:01Z

@rewop please bring back lodash.isequal package.

tomchentw · 2016-05-29T14:41:09Z

Looks great so far. I'll review this again tomorrow morning and merge it.

tomchentw · 2016-05-30T01:38:31Z

I've tested on my local and it's working great. Thanks again!

tomchentw · 2016-08-04T04:34:15Z

Released v5.0.0.

This version(5.0.0) will never be tagged latest on npm since there's a major rewrite ongoing for 6.0.0. See more details in #267 (comment)

Simone Potenza added 2 commits March 8, 2016 18:27

Added shouldComponentUpdate that returns false to Marker component. T…

7a1a49e

…his incresases the performance of the map, when rendering many markers

cristiandley mentioned this pull request May 13, 2016

Is this repo dead ? #256

Closed

vinceprofeta and others added 3 commits May 16, 2016 08:25

Merge pull request #1 from rewop/performance-many-markers

b09c212

Performance many markers

isSame marker

d60030a

build

8c69d2c

Vincent.Profeta and others added 2 commits May 16, 2016 23:41

update

5c80d02

Merge pull request #1 from TheModevShop/master

3b404f0

fork merge

vinceprofeta mentioned this pull request May 24, 2016

Passing in a prop to marker to call during should component update. cristiandley/react-gl-maps#2

Merged

rewop mentioned this pull request May 25, 2016

Performance issues when changing a marker icon with lots of markers #135

Closed

This was referenced May 26, 2016

Todos & Question cristiandley/react-gl-maps#6

Open

Use context instead of mapHolderRef hack cristiandley/react-gl-maps#9

Open

tomchentw reviewed May 29, 2016
View reviewed changes

tomchentw added the For_maintainers label May 29, 2016

Reverted lib fodler to commit 5e672b9

9713c42

Removed shouldComponentUpdate lifecycle hook from Marker component

4404e40

tomchentw reviewed May 29, 2016
View reviewed changes

Removed unused lodash dependency

9cbf729

tomchentw reviewed May 29, 2016
View reviewed changes

Reverted lodash.isEqual dependency

3737358

tomchentw merged commit c2d265c into tomchentw:master May 30, 2016

tomchentw removed the For_maintainers label May 30, 2016

tomchentw mentioned this pull request May 30, 2016

5.0.0 #267

Closed

8 tasks

tomchentw added this to the 5.0.0 milestone May 30, 2016

tomchentw mentioned this pull request Jul 7, 2016

Map should use context to pass map reference to child components #277

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Performance with many markers #224

Performance with many markers #224

rewop commented Mar 8, 2016 •

edited

Loading

cristiandley commented May 13, 2016

vinceprofeta commented May 16, 2016

vinceprofeta commented May 16, 2016 •

edited

Loading

rewop commented May 16, 2016

vinceprofeta commented May 16, 2016

vinceprofeta commented May 16, 2016

vinceprofeta commented May 16, 2016

cristiandley commented May 24, 2016

rewop commented May 24, 2016

rewop commented May 25, 2016

tomchentw commented May 29, 2016

tomchentw May 29, 2016

rewop May 29, 2016

tomchentw May 29, 2016

rewop commented May 29, 2016

rewop commented May 29, 2016

tomchentw May 29, 2016

tomchentw May 29, 2016

tomchentw commented May 29, 2016

rewop commented May 29, 2016

tomchentw May 29, 2016

rewop May 29, 2016

tomchentw commented May 29, 2016

tomchentw commented May 29, 2016

tomchentw commented May 30, 2016

tomchentw commented Aug 4, 2016

Performance with many markers #224

Performance with many markers #224

Conversation

rewop commented Mar 8, 2016 • edited Loading

Problem

Solution

cristiandley commented May 13, 2016

vinceprofeta commented May 16, 2016

vinceprofeta commented May 16, 2016 • edited Loading

rewop commented May 16, 2016

vinceprofeta commented May 16, 2016

vinceprofeta commented May 16, 2016

vinceprofeta commented May 16, 2016

cristiandley commented May 24, 2016

rewop commented May 24, 2016

rewop commented May 25, 2016

tomchentw commented May 29, 2016

tomchentw May 29, 2016

Choose a reason for hiding this comment

rewop May 29, 2016

Choose a reason for hiding this comment

tomchentw May 29, 2016

Choose a reason for hiding this comment

rewop commented May 29, 2016

rewop commented May 29, 2016

tomchentw May 29, 2016

Choose a reason for hiding this comment

tomchentw May 29, 2016

Choose a reason for hiding this comment

tomchentw commented May 29, 2016

rewop commented May 29, 2016

tomchentw May 29, 2016

Choose a reason for hiding this comment

rewop May 29, 2016

Choose a reason for hiding this comment

tomchentw commented May 29, 2016

tomchentw commented May 29, 2016

tomchentw commented May 30, 2016

tomchentw commented Aug 4, 2016

rewop commented Mar 8, 2016 •

edited

Loading

vinceprofeta commented May 16, 2016 •

edited

Loading