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

Issues with the peers page #1072

Closed
5 of 8 tasks
olizilla opened this issue Jul 23, 2019 · 5 comments · Fixed by #2181
Closed
5 of 8 tasks

Issues with the peers page #1072

olizilla opened this issue Jul 23, 2019 · 5 comments · Fixed by #2181

Comments

@olizilla
Copy link
Member

olizilla commented Jul 23, 2019

Sorting

We don't need both, or at least, we don't need to always sort by multiaddr in the selectPeers function, if we're going to sort by something else later on.

Caching

  • cache geoip lookups #1071 introduces caching the location for an IP. The geoip look up is where most of the work happens. We should remove caching by PeerID, as it's totally reasonable for PeerIDs to move IP addresses.

Rendering

  • As pointed out in cache geoip lookups #1071 we get d3 to render a dot per peer. Many peers get geolocated to the same city, and the map is at global scale, so we can optimise this to only render a dot per city / unique location.
  • The peers table should fill the available vertical space on screens with larger vertical height.

Complexity

  • We do a lot of work in peer-locations.js to expose the internal logic of the queuing of geoip look-ups as state in redux. We are not visualising the queue, nor do we plan to, so i suspect the code could be written more simply. We could extend the work in cache geoip lookups #1071 to encapsulate the geoip look up queue and caching in a class, and simply have it periodically update the redux state with batches of locations as they are found, rather than have all the queuing logic managed in redux.
@jbenet
Copy link
Member

jbenet commented Jul 23, 2019

This issue is great to have! 👏

Some more thoughts, take them or leave them.

  • The structure of the cache & loading for geoip locations seems off to me -- cache lookups are fast, and right now they're gating on the concurrency queue. We could switch things up so that we can check the cache first for all items, and only if not in the cache do we add them to the (bounded) geoip lookup queue. (this should have the effect of loading all the peers & ip addresses we've looked up before first, and then
  • i think geoip db doesnt change very often (years?) the geoip cache could stay for longer. (may be better to bound it by size too, so it doesnt get too big? not sure)
  • Rendering seems really expensive at the moment. i think there's probably inefficiency because the actual work (rendering a list with <100K items, rendering a map w/ <100K dots) doesn't seem that intensive.
    • I think the world map render is called a lot and very often, not sure why (maybe every time a peer changes?) -- maybe should call it w/ batches as well? maybe a simple debounce or throttle may be enough?
    • i think something's funky with the new list. seems to be more expensive

@JustMaier
Copy link
Contributor

I'll take a stab at the sorting issues 👍

@olizilla
Copy link
Member Author

olizilla commented Jul 24, 2019

@JustMaier that would be RAD! Go for it!

✨🎷🐩

@JustMaier
Copy link
Contributor

@olizilla during the Web UI call today, we discussed sorting of the location column. The consensus was that ordering by country then city (with Unknown at the end of the list) made the most sense when ordering by that column. With that in mind, I'm wondering if maybe it also makes sense to change what is displayed in that column to match that sorting logic.

So, instead of displaying: Houston, United States
We display: United States, Houston

I think unless we change the display, the ordering in the requested order in the location column might not make sense at first glance.


As for performance issues, looking at the render cycles, it looks like every time a location is resolved it's re-rendering the table. That's why it seems to disappear/reappear while scrolling.

I wonder if maybe it makes more sense to create a multiaddr to location component that we render inside the table's location column. The only problem with that is then we can't sort by location since it's not part of the dataset. Ideas?

@olizilla
Copy link
Member Author

olizilla commented Aug 5, 2019

@JustMaier Yep, feel free to change the display order of the location.

What if we adjust the table update logic so that it only updates if you are not interacting with it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants