search

hotosm / osm-tasking-manager2

Designed and built for Humanitarian OpenStreetMap Team collaborative emergency/disaster mapping, the OSM Tasking Manager 2.0 divides an area into individual squares that can be rapidly mapped by thousands of volunteers.
http://tasks.hotosm.org
Other
425 stars 156 forks source link

Cache users.json in browser for 10 minutes, unless query is present. #926

Closed Piskvor closed 7 years ago

Piskvor commented 7 years ago

This keeps the @mentions reasonably current, while reducing this file's load time to ~0 per tile select from the current ~2000 ms - which improves both client-side experience and server-side load. The downside is not autocompleting brand new users (newer than the caching interval) - this seems, to me, as an acceptable tradeoff.

Piskvor commented 7 years ago

Probably most relevant to https://github.com/hotosm/osm-tasking-manager2/issues/923 instead of 918.

pgiraud commented 7 years ago

Hey! I just figured that we never use any server side filtering on the users list, ie. the users.json is never called with a q parameter. We always load the complete list of users once for all. The filtering is then done on the client. We should probably change this. It looks like At.js should let us do that pretty easily. https://github.com/ichord/At.js/wiki/How-to-use-remoteFilter

Also we could probably merge the code in https://github.com/hotosm/osm-tasking-manager2/blob/master/osmtm/views/project.py#L435 and in https://github.com/hotosm/osm-tasking-manager2/blob/master/osmtm/views/user.py#L44. The first one is helpful for the assign to feature.

Piskvor commented 7 years ago

Hello :) Anyway, the filtering is not really the crucial part of the PR: in mapathons, I'm seeing requests for /users.json slow down and frustrate users - and with 30+ people clicking around in the tile selection, it might exert a significant load on the server as well.

The essential part is setting the Cache-Control headers: after first load, site becomes two orders of magnitude faster. Magic! ;)

Given how small this change is, and how much it improves user experience, I would be very happy to see this live, while we perhaps explore more sophisticated caching schemes.

pgiraud commented 7 years ago

I'd like to give this a try and prevent as much requests as possible. I'll come back with a different proposal. FYI the current performance issues are not due to the tasking manager but to an other application running on the same machine. Even though I think that adding a cache mechanism would be really nice.

Piskvor commented 7 years ago

So, the client-side caching is not going to be merged at all, in favor of server-side caching? Or is there some way I could re-write this in a more general way? (The 10-minute cache freshness might be too long, I admit - I just picked a number that I have considered reasonable)

pgiraud commented 7 years ago

@Piskvor Have you seen the #934 pull request? With this pull request the users.json is queried only when needed and is very limited. This means that there's no real need to use some caching. Can you please give it a look?

Piskvor commented 7 years ago

Aha, I have not seen that, looks far more sensible than "just cache it, done." Closing this one then - aven though client-side caching might be useful, this is not the place.