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 Slow with ~1k Markers #38

Closed
vpontis opened this issue Oct 1, 2018 · 10 comments
Closed

Performance Slow with ~1k Markers #38

vpontis opened this issue Oct 1, 2018 · 10 comments
Labels
enhancement New feature or request question Further information is requested

Comments

@vpontis
Copy link

vpontis commented Oct 1, 2018

Hi everyone, I have ~1k markers that could be in the viewport at a time and I am seeing a bottleneck in the Chrome Performance profiler from Firestore.

I am not sure if this is due to large Geohash regions, too many updates to the underlying Firestore collection or what...

At this point, the map can get really laggy and non-responsive. (Sometimes users will get the "this tab is not responding" message...)

Has anyone dealt with similar problems / scale?

Here is what my profiler looks like:

image

@vpontis vpontis changed the title Performance when ~1k markers Performance Slow with ~1k Markers Oct 1, 2018
@MichaelSolati
Copy link
Owner

@vpontis so we will run a few queries for the primary hash and the surrounding area, because this is based on geofire-js we're keeping track of whats in your query so we can update you on changes in documents (wether thats based on your query location changing, or something else). So we're not only just passing to you docs from our queries, but we're also keeping track of everything we've queried for you (until it falls out of your query)

So as of right now, unfortunately some of the scale issue comes from firestore queries, but also how we handle queries and tracking isn't scaling.

This WILL change in v3.0.0 which I started working on this weekend actually. That is at least a month out though, and will be a bit of a massive change from the current geofirestore. (Data structure stays the same though, so you don't need to worry about that, it's just how the library queries work)

I'll ask that you be patient, big changes are coming which will make this better (but it may not be a perfect fix, I can't scale firestore 😅)

@MichaelSolati MichaelSolati added the enhancement New feature or request label Oct 1, 2018
@MichaelSolati MichaelSolati added this to the v3.0.0 milestone Oct 1, 2018
@vpontis
Copy link
Author

vpontis commented Oct 1, 2018

Yep, thanks @MichaelSolati. I understand on a basic level how the querying model works and on how onSnapshot will mean that any changes to the docs will push an update.

I thought about this when designing my Firestore collection. I made sure to only update the Firestore collection when there is a meaningful change rather than on every update.

I will test if Firestore updates are causing the sluggishness by preventing Firestore from updating.

Are there any other performance tweaks that I should try to make right now? I feel semi-comfortable modifying the GeoFirestore library and maybe if I try a few things out, my results can help inform the v3.0.0 design.

One idea I have is that when I have too large of a Geohash query the onSnapshot will have a large payload that is expensive to process. So I could restrict the Geohash library to have a minimum Geohash length for every query.

@vpontis
Copy link
Author

vpontis commented Oct 2, 2018

One idea would be to enable GeoFirestore to do an approximate of "choose the K nearest points" by having the Geohashes spiral out from the center point.

I guess I could do this in my code by dynamically increasing the radius of the GeoQuery until I get a certain number of results... What do you think? Maybe that's worth a shot...

EDIT:

The question here would be if GeoFirestore is smart enough not to recompute everything if I expand the radius of the GeoQuery. Or will expanding the radius cause GeoFirestore to re-create queries for geohashes that are already tracked.

I'll test this out...

@MichaelSolati
Copy link
Owner

So geofirestore shouldn't run new queries for hashes that are already being tracked. As for obvious tweaks, I don't have any right now.

For the next iteration of geofirestore you'll see that the data returned and the library will look ALOT more like Firestore.

@vpontis
Copy link
Author

vpontis commented Oct 3, 2018

For the next iteration of geofirestore you'll see that the data returned and the library will look ALOT more like Firestore.

Hmm, does this mean that it will be better for performance?

I had to remove GeoFirestore due to the perf issues. Do you have a plan for v3 that you can share? I'd be interested in looking at it to see if any of the performance stuff would make GeoFirestore work for my project :)

@MichaelSolati
Copy link
Owner

Well I did push up a branch for v3.0.0, it still has a bit of work (maybe another week or so), then I'll start updating tests and docs for it.

And yes, I think there will be significant performance improvements.

@MichaelSolati
Copy link
Owner

Hey, as a quick question, when you update your Firestore collection are you by chance updating docs individually or by batch?

@MichaelSolati MichaelSolati added the question Further information is requested label Oct 9, 2018
@vpontis
Copy link
Author

vpontis commented Oct 9, 2018

Hey, as a quick question, when you update your Firestore collection are you by chance updating docs individually or by batch?

I'm updating docs individually. Maybe batching would help with GeoFirestore perf?

For now I have removed GeoFirestore from the project :(

@MichaelSolati
Copy link
Owner

Ok, hey if it didn't work for you it didn't work for you. But I do appreciate you raising the issue as it may provide a learning experience for other developers. Anyway, in v3.0.0 batches will be supported, until then if you wanted to batch update/set docs something like this would be useful.

import * as admin from 'firebase-admin';
import { Geokit } from 'geokit';

const collection = admin.firestore().collection('restaurants');

// Use a custom function to configure doc to be inserted to look like how GeoFirestore inserts docs
function parse(unprocessed: any[]): any[] {
  return unprocessed.map((doc: any) => {
    doc.coordinates = new admin.firestore.GeoPoint(doc.coordinates[0], doc.coordinates[1]);
    return {
      g: Geokit.hash({ lat: doc.coordinates.latitude, lng: doc.coordinates.longitude }),
      l: doc.coordinates,
      d: doc
    };
  });
}

// Parse data to look like how GeoFirestore inserts docs
const batchedData: any[] = parse(data);

// Create Firestore Batch
const batch = admin.firestore().batch();

// Iterate over data and commit them into batch
batchedData.forEach((item) => {
  // Create a reference to create the new doc
  const insert = collection.doc();
  batch.create(insert, item);
});

// Batch commit all changes all at once
batch.commit()

Know two things, 1) we're not using geofirestore, it's simply unnecessary here as we're skipping over it, instead we're using a library called geokit to hash our coordinates. 2) You can only batch 500 changes at a time.

@vpontis
Copy link
Author

vpontis commented Oct 10, 2018

Awesome, thanks @MichaelSolati.

Yea, I am actually doing the updating of collections on a python server.

I will probably take another look at performance / GeoFirestore in the next month. So I'll definitely try out batch updates when I do that. And I'll let you know how that goes.

I appreciate all the help.

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants