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

Improve graph performance #191

Closed
6 tasks done
oscarlorentzon opened this issue Sep 13, 2016 · 6 comments
Closed
6 tasks done

Improve graph performance #191

oscarlorentzon opened this issue Sep 13, 2016 · 6 comments

Comments

@oscarlorentzon
Copy link
Member

oscarlorentzon commented Sep 13, 2016

The performance for building the graph is currently bad in areas where there is high node density. The problem has many reasons, the most important one being that tiles are loaded with too many nodes, the nodes contain too much data and the tiles contains all the sequences for all the nodes in the tile. The sequences are retrieved for every tile, i.e. duplicates that are not needed are retrieved for adjacent tiles. The tiles can therefor be vary large which requires good network connection for fast retrieval. The aim of this issue is to completely rebuild the graph implementation to make it fast and dynamic.

To resolve the performance issue, the graph implementation needs to be completely rewritten. When doing this a number of other points should be addressed as well. The new implementation should be written in a way that supports the following:

The idea of the new graph implementation is for data to flow in the following way:

                key 
                 |
                 |
                 v
              imByKey (Core + Spatial + H) ------> Node
                / \                                 |
               /   \                                |
              v     v                               v
           imsByH  seqByKey           Image + Mesh + Core + Spatial
           (Core)   |                               |
              |     |                               |
              |     |                               v
              v     |                          Put on state 
        imsByKey    |                         (i.e. display)
  (bbox, Spatial)   |
              |   / |
              |  /  |
              | /   |
              v     v
   Spatial edges   Sequence edges
               \   /
                \ /
                 v
               Node
                / \                            
               /   \ 
              v     v
           Display arrows

In this way the node could potentially be put on the state once the node assets (image, mesh) have arrived but before the edges have been calculated. The lightweight tiles would not contain heavy node or sequence data and be fast to retrieve. The spatial edge calculation would rely on the heavy data but it would only be retrieved for exactly the image keys that are needed. The rbush spatial index will be used for determining what keys need to be turned in nodes by fetching the heavy node data. The sequence edge calculation would need to fetch the sequences for the nodes that requires edge calculation only. Falcor will be used to cache as well as batch load requests.

The implementation of the new graph issue includes:

  • Data flow through observables from GraphService, Graph, Node and Edges
  • Falcor API usage for building graph
  • Ligthweight tile fetching
  • Heavy imByKey fetching for areas required for edge calculation
  • Two data streams for edge calculation (spatial, sequence)
  • Changed graph, node, state and navigation implementations.

This issue requires breaking changes to the public API, therefor the library version needs to be pushed to 2.0 when implemented.

@oscarlorentzon
Copy link
Member Author

The implementation of this issue should resolve #25, #67 and #131.

@oscarlorentzon
Copy link
Member Author

#192 depends on the implementation of this issue.

@oscarlorentzon
Copy link
Member Author

mapillary/mapillary_issues#2257 depends on the implementation of this issue.

@oscarlorentzon
Copy link
Member Author

#25, #67 and #131 have not been implemented in the scope of this issue but the graph implementation supports all scenarios in those issues.

@gyllen
Copy link
Member

gyllen commented Oct 27, 2016

@oscarlorentzon you wanna close this issue and open a new one with remaining tasks?

@oscarlorentzon
Copy link
Member Author

Graph implemented according to diagram above. Separate issues created for remaingin functionality from description.

@oscarlorentzon oscarlorentzon added this to the 2.0.0 milestone Oct 28, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants