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

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

Closed
hovatterz opened this issue Oct 15, 2015 · 18 comments · Fixed by #224
Closed

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

hovatterz opened this issue Oct 15, 2015 · 18 comments · Fixed by #224

Comments

@hovatterz
Copy link

I have a dataset of about 119 items (other maps will have more), and each item is a marker on the Google map. It seems that when I change the icon for one marker (on hover) it removes and adds every single marker. This leads to some slowness and slight flickering.

Is there some way I can avoid this? Sorry if I'm missing something obvious.

var React = require('react/addons');
var GoogleMapLib = require('react-google-maps');
var GoogleMap = GoogleMapLib.GoogleMap;
var Marker = GoogleMapLib.Marker;
var InfoWindow = GoogleMapLib.InfoWindow;

var TestMap = React.createClass({
  getInitialState: function() {
    return {
      markers: []
    };
  },

  componentWillReceiveProps: function(nextProps) {
    var markers = nextProps.items.map(function(item, i) {
      return {
        icon: '/public/img/marker-blue.png',
        position: {
          lat: item.lat,
          lng: item.lon
        },
        item: item
      }
    }.bind(this));

    this.setState({ markers: markers });
  },

  render: function() {
    var containerProps = {
      className: 'items-map'
    };

    return (
      <GoogleMap containerProps={containerProps}
                 defaultZoom={13}
                 defaultCenter={this.props.center}>
        <Marker icon={'/public/img/pin-orange.png'} position={this.props.center} />
        {this.state.markers.map(function(marker, i) {
          var ref = 'marker_' + i;
          return (
            <Marker key={ref} ref={ref}
                    icon={marker.icon}
                    position={marker.position}
                    onMouseover={this._onMarkerMouseOver.bind(this, i)}
                    onMouseout={this._onMarkerMouseOut.bind(this, i)}>
              {marker.showInfo ? this._renderInfoWindow(ref, marker) : null}
            </Marker>
          );
        }.bind(this))}
      </GoogleMap>
    );
  },

  _renderInfoWindow: function(ref, marker) {
    return (
      <InfoWindow key={ref + '_info_window'}>
          <p>Test</p>
      </InfoWindow>
    );
  },

  _onMarkerMouseOver: function(index, event) {
    var update = {};
    update[index] = {
      $merge: {
        icon: '/public/img/marker-orange.png',
        showInfo: true
      }
    };

    var changedMarkers = React.addons.update(this.state.markers, update);
    this.setState({ markers: changedMarkers });
  },

  _onMarkerMouseOut: function(index, event) {
    var update = {};
    update[index] = {
      $merge: {
        icon: '/public/img/marker-blue.png',
        showInfo: false
      }
    };

    var changedMarkers = React.addons.update(this.state.markers, update);
    this.setState({ markers: changedMarkers });
  }
});

module.exports = TestMap;
@hovatterz hovatterz changed the title Performance issues when changing the icon with lots of markers Performance issues when changing a marker icon with lots of markers Oct 15, 2015
@tomchentw
Copy link
Owner

Could you provide me a gist that contains the mock marker data so that I could test it out? Thanks!

@tomchentw
Copy link
Owner

At first glance, I didn't see any problem in your code. You have key properly setup and it should work fine during render cycles ... weird.

@hovatterz
Copy link
Author

https://gist.github.com/hovatterz/be265738f8a297bad4df

You can just set the center point to any one of those lat/lon points. I fixed the markers zIndex seeming to change on hover by specifying a zIndex for each one. I think it would still be preferable for them to not re-render though.

The defaultZoom is set to 13

@tomchentw
Copy link
Owner

@hovatterz I just setup a branch in the example repo here: http://git.io/vWllh. As I ran it on my computer, I didn't experience any lags nor delays. Could you take a look and modify on the branch as your wish?

@ksavenkov
Copy link

I observe something like that. My map component has a list of polylines and markers among other props and I see render is called for each marker and polyline even if another prop is updated.

In the rest of my app I avoided such situations by using Immutable.js for props and turning PureRenderMixin on. I wish all markers and polylines were Pure components here as well, or at least shouldComponentUpdate could have been customised for them.

@tomchentw
Copy link
Owner

@ksavenkov ideally that should be the case. However, since we have options and defaultOptions nested object, it's getting more tricky here.

I'll try to fix this in the next roadmap.

@bitspook
Copy link

Shouldn't setting shouldComponentUpdate on the marker component to pure-render should this problem (of all markers re-rendering when only one of them changes)? Am I missing something here?

I am facing performance problems too with lots of markers (~100), but their markers aren't re-rendering on every prop change.

@tomchentw
Copy link
Owner

@channikhabra maybe that will do the trick. Make sure to check options and defaultOptions as well. Interested for a PR?

@bitspook
Copy link

Hey Tom,

I made an embarrassing mistake. I thought I was commenting on google-map-react project (https://github.com/istarkov/google-map-react/). We're having some performance problems and a colleague mentioned this issue. I mistook it for similarly named google-map-react which we're using.

Sorry for the inconvenience.

@tomchentw
Copy link
Owner

@channikhabra no worries.

@rewop
Copy link
Collaborator

rewop commented Mar 7, 2016

I came across the same performance problem, and I believe it is a great stopper to use this fantastic module in production.

I have been trying a map with the more than 3000 markers. The map would use MarkerClusterer to group the marker.

In my map, every time a marker is clicked, an overlay is shown on the map to visualise the information related to the marker.

With so many markers, the map becomes very slow every time is clicked. I thought the problem was with the MarkerClusterer, but the same happens also without it.

Looking at the code, it looks like the Marker components passed as children to the GoogleMap, gets rendered on every update, making the map freeze for few seconds, before it gets updated.

As an idea, I am trying to add the puremixin to the Marker element, but I am not sure it is a pure component, even though It looks like it is. However, I am facing problems to run the module in development, and test it against the examples.

I see that the folder examples/gh-pages has a package.json, used also to install a specific version of react-google-map. To reflect my changes in the examples, I have removed the folder react-google-map from the node_modules and run npm link ../../ so to get my changes linked. However, when I run the application I get the error in the inspector Uncaught Error: Cannot find module "react-google-maps/lib/async/ScriptjsLoader", and I cannot test my changes. It looks like webpack is not able to follow the symlink in the node_module folder

@tomchentw before I open a new issue with this problem, could you explain how to run the examples so that the changes are reflected?

@rewop
Copy link
Collaborator

rewop commented Mar 8, 2016

I tried to investigate further. I run this example with the map:

import React, { Component } from "react";
import { GoogleMapLoader, GoogleMap, Marker, OverlayView } from "react-google-maps";

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

function getRandomInRange(from, to, fixed) {
    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: []
        };
    }

    componentDidMount() {
        this.setState({
            markers: getMarkers()
        });
    }

    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.markers.map((marker, index) => {
                            return (
                                <Marker
                                    {...marker}
                                    onClick={() => this.onSelectMarker(marker)}
                                    key={index}
                                />
                            );
                        })}
                    </GoogleMap>
                }
            />
        );
    }
}

With this example every click on the marker, as well as click on the overlay button, are very slow. I tried to implement the shouldComponentUpdate in the Marker component, so that it always returns false (PureMixin in this case wouldn't work). The map still works but the performance issue is not solved.

Digging further I noticed that in the render function of GoogleMapHolder, all the children are cloned at every render. I believe this is done to pass down the mapHolderRef property. I think this is the problem. I haven't tried to fix this, because it may require a structural refactor. I am considering if react context api in this case may help, but first we should confirm that this is the problem. @tomchentw What do you think? I may need your help here to move forward.

@rewop
Copy link
Collaborator

rewop commented Mar 8, 2016

@tomchentw I made an attempt to use context to increase performance, and it worked very fine. You can check it in the PR I linked. However, I still need to run the examples, to make sure nothing has been broken. Looking forward to hearing your thoughts about it :)

@sohibul
Copy link

sohibul commented May 25, 2016

@rewop Could you explain, what do you mean with "context"?

@wedneyyuri
Copy link

Are you using this?

https://facebook.github.io/react/docs/context.html
Em 25 de mai de 2016 7:33 AM, "sohibul" notifications@github.com escreveu:

@rewop https://github.com/rewop Could you explain, what do you mean
with "context"?


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#135 (comment)

@rewop
Copy link
Collaborator

rewop commented May 25, 2016

@sohibul @wedneyyuri Yes I used react context api. I added the links my explanation here.

To summarise my implementation in #224:

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 the 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.

@tomchentw
Copy link
Owner

Close this for #224 .

We're also looking for maintainers. Involve in #266 to help strengthen our community!

tomchentw pushed a commit that referenced this issue May 30, 2016
* Improved performance of rendering many markers/components on the map
* Closes #135
* Closes #210
* Closes #216

BREAKING CHANGE: If you're just using the library and didn't make custom components before, feel free to ignore this implementation changes.

Now, mapHolderRef for each component are now passed in via context. This doesn't affect all components interface in general. But if you do custom components before and relies on the implementation of react-google-maps, you should be aware of this and make some changes.
@stenrdj
Copy link

stenrdj commented Oct 4, 2018

Hello @tomchentw , is this PR exist in the current version 9.0.0 please ? cause i still face performance issue with 2000 marker on map !! thanks a lot .

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 a pull request may close this issue.

8 participants