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

pre-queries for geojson geoms in bounding box before cluster query re… #10663

Merged
merged 18 commits into from
Apr 17, 2024

Conversation

whatisgalen
Copy link
Member

@whatisgalen whatisgalen commented Mar 7, 2024

#10452

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Description of Change

This PR aims to do a much simpler count_query that, if the count is 0, skips a more complex query, caches an empty tile, and raises a 404 for the MVT.

The count_query first counts how many geojson geoms for the requested node actually lie within the tile bounding box. If count >= the min_points parameter for that node config, it proceeds to the clustering query which further narrows its query with the same subquery as the count_query to speed things up. If 0 < count < min_points it proceeds to render the unclustered mvt geometry tile.

This PR also creates a method to create a MVT cache key incorporating...

  • nodeid, x, y, and zoom
  • the userid (proxy for viewable_nodegroups)
  • a snapshot of the database in the form of the editlog count for the nodegroup linked to the request node

...so that stale MVT/data won't override more recent MVT/data for that node/zoom/x/y/user.

Anecdotal benchmarks of this branch vs the target branch show up to a 30% speed improvement for rendering the largest clustered MVTs.

Testing This PR

  • To test the query-related performance enhancement, you'll want to set tile = None between lines 285 and 286 to temporarily disable MVT caching.
  • To test the caching of 404's, you'll want to clear out your cache

Issues Solved

#10452

Checklist

  • Unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Ticket Background

Further comments

@chiatt
Copy link
Member

chiatt commented Mar 7, 2024

While this looks like a nice performance improvement, it doesn't actually address #10452. A solution to #10452 would cache the fact that the database query was just performed and returned a 404, so that a subsequent request would skip any database query altogether.

@whatisgalen
Copy link
Member Author

whatisgalen commented Mar 7, 2024

@chiatt we can do that, but I would strongly advise adding in some logic that embeds the state of the db into the cache key so that we avoid a case where a 404 was cached, then a new geom was created but it doesn't get included in a tile because the cache hasn't timed out yet.

For example:

cache_key = create_mvt_cache_key(node, zoom, x, y, request.user)
...

def create_mvt_cache_key(node, zoom, x, y, user, mvt_snapshot=None):
    if not mvt_snapshot:
        mvt_snapshot = models.EditLog.objects.filter(nodegroupid=str(node.nodegroup_id)).count()
    return f"mvt_{str(node.nodeid)}_{zoom}_{x}_{y}_{mvt_snapshot}_{user.id}"

@whatisgalen whatisgalen requested a review from chiatt March 8, 2024 01:25
@whatisgalen whatisgalen removed the request for review from chiatt March 8, 2024 01:36
@whatisgalen
Copy link
Member Author

still testing a couple alternative approaches here

Copy link
Member

@chiatt chiatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It really seems like this should offer a nice performance improvement, but when I look at request times in Silk, I'm not seeing any improvement. It might be that I'm not testing it with the right data because I'm sure the scale and distribution of points makes a difference. Also, the additional query to the edit log is probably increasing the request time by a wee bit as well.

Also, it doesn't seem consequential, but I'm getting this 500 error:

Traceback (most recent call last):
  File "/usr/local/lib/python3.10/dist-packages/django/core/handlers/exception.py", line 55, in inner
    response = get_response(request)
  File "/usr/local/lib/python3.10/dist-packages/django/core/handlers/base.py", line 197, in _get_response
    response = wrapped_callback(request, *callback_args, **callback_kwargs)
  File "/usr/local/lib/python3.10/dist-packages/django/views/generic/base.py", line 104, in view
    return self.dispatch(request, *args, **kwargs)
  File "/web_root/arches/arches/app/views/api.py", line 107, in dispatch
    return super(APIBase, self).dispatch(request, *args, **kwargs)
  File "/usr/local/lib/python3.10/dist-packages/django/views/generic/base.py", line 143, in dispatch
    return handler(request, *args, **kwargs)
  File "/web_root/arches/arches/app/views/api.py", line 396, in get
    tile = bytes(cursor.fetchone()[0])
TypeError: 'NoneType' object is not subscriptable

[nodeid, zoom, x, y, nodeid, resource_ids],
)
else:
tile = ""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like this alone will prevent a requery for the MVT because the value saved to the cache isn't None which is what is being checked for prior to the query.

arches/app/views/api.py Show resolved Hide resolved
@@ -365,6 +400,10 @@ def get(self, request, nodeid, zoom, x, y):
raise Http404()
return HttpResponse(tile, content_type="application/x-protobuf")

def create_mvt_cache_key(node, zoom, x, y, user, mvt_snapshot=None):
if not mvt_snapshot:
mvt_snapshot = models.EditLog.objects.filter(nodegroupid=str(node.nodegroup_id)).count()

This comment was marked as outdated.

@chiatt
Copy link
Member

chiatt commented Mar 16, 2024

@chiatt we can do that, but I would strongly advise adding in some logic that embeds the state of the db into the cache key so that we avoid a case where a 404 was cached, then a new geom was created but it doesn't get included in a tile because the cache hasn't timed out yet.

For example:

cache_key = create_mvt_cache_key(node, zoom, x, y, request.user)
...

def create_mvt_cache_key(node, zoom, x, y, user, mvt_snapshot=None):
    if not mvt_snapshot:
        mvt_snapshot = models.EditLog.objects.filter(nodegroupid=str(node.nodegroup_id)).count()
    return f"mvt_{str(node.nodeid)}_{zoom}_{x}_{y}_{mvt_snapshot}_{user.id}"

If the value is different between what is saved to the cache when a user gets a 404 vs an MVT tile, then it seems ok to return no tile even if an editor has recently created a new geometry within those bounds. That's the price of relying on a cache, and the cache timeout can always be adjusted to an admin's preference for performance over accuracy.

@whatisgalen
Copy link
Member Author

@chiatt the SELECT COUNT query should leverage Postgres indexes and caching (vs a regular SELECT query) so while it does represent a query, it's still performant. If we want to basically rule out creating any new queries on principal, the alternative/workaround would be to cache a count of the all editlog objects every time a new editlog instance gets created. I would be a little surprised though if removing the SELECT COUNT query actually sped things up by more than a hundred milliseconds.

It really seems like this should offer a nice performance improvement, but when I look at request times in Silk, I'm not seeing any improvement. It might be that I'm not testing it with the right data because I'm sure the scale and distribution of points makes a difference. Also, the additional query to the edit log is probably increasing the request time by a wee bit as well.

How many geometries does the resource layer you're testing with have? I would expect little to no improvement for lower-density resource-layers but once you get into the tens of thousands I would expect noticeably faster responses for MVT requests.

arches/app/views/api.py Outdated Show resolved Hide resolved
arches/app/models/models.py Outdated Show resolved Hide resolved
@chiatt chiatt merged commit 866bb69 into dev/7.6.x Apr 17, 2024
4 checks passed
@chiatt chiatt deleted the 10452_mvt_optimize branch April 17, 2024 18:05
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 this pull request may close these issues.

2 participants