Repository navigation
feat(taxon-tiles): improve performance and scope to collection - #8584
grantfitzsimmons wants to merge 7 commits into
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
grantfitzsimmons
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
github-project-automation
Bot
moved this from 📋Back Log
to Dev Attention Needed
in General Tester Board
|
@coderabbitai review |
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Yayyy, more performance stuff! 🚀
For some reason, taxon rows were filtered by collection discipline, but determination counts were not restricted to the active collection.
I don't think it was ever a requirement to only return Determinations in the current collection? 🤷
I can see there being some benefit in getting a birds-eye view of how large the default Taxon tree is as a whole (across all Collections within the Discipline), though it is probably more useful to each individual Collection Manager to see results scoped to just one Collection so I'm in favor of the change.
Perhaps CollectionMemberID was considered too unreliable (though MariaDB should still be able to fairly cheaply do the Collection scoping through the CollectionObject?
On a side note, we should probably look into changing these CollectionMemberID columns across tables to dedicated Foreign Keys).
I just very briskly looked through the non-test code and did not do any testing, profiling, etc.
Few notes here: mainly for gathering better profiling information and ways to enhance this in the future.
Though I would like to see us remove the existing index which contains just CollectionMemberID. The new composite index already covers the old single-column index. Can't think of a good reason to keep the old single column index. If there is one, we can keep it though.
| taxons = Taxon.objects.annotate( | ||
| current_determination_count=Coalesce( | ||
| Subquery( | ||
| current_determination_counts, | ||
| output_field=IntegerField(), | ||
| ), | ||
| 0, | ||
| ) | ||
| ).values_list( | ||
| 'id', 'rankid', 'parent_id', 'name', 'current_determination_count' | ||
| ) |
There was a problem hiding this comment.
(future enhancement notes!)
Just quick note on scalability here. None of these would be strictly required, but something to consider for future enhancements!
These QuerySets can still suffer from #7864.
That is, if there is a large number of Taxon objects returned from the database to this QuerySet, Django will hold them all in memory at once (values_list is very nice, but a few million Taxon records could still hold a lot of memory and converting it all to a list might use the CPU for a time).
To make this even more performant, I would recommend a streaming/pagination approach (specifically using a keyset/cursor method) to the frontend, where the frontend fetches pages of Taxon records at a time.
That would limit the amount of work the frontend*, backend, and database has to do for each individual fetch.
Alongside the memory and performance benefits, this would also perform much better for #7880 a Gunicorn worker being caught with a long-running task can not respond to other requests.
This is especially true for CPU intensive operations: in a high-thread low-process setup like we have in production environments, a CPU hungry operation can theoretically impact how long it holds the python GIL, impacting other threads in the same process.
Barring with or in addition a streaming approach, it would be nice to move these large and memory intensive operations to a separate process, so the memory can be immediately reclaimed when they're finished (or so that the OOMKiller doesn't have to kill the entire worker process, if it is invoked).
| ).values_list( | ||
| 'id', 'rankid', 'parent_id', 'name', 'current_determination_count' | ||
| ) | ||
| return list(filter_by_collection(taxons, collection)) |
There was a problem hiding this comment.
(optional / note to self)
When I have a moment, I will do so, but if someone is up for it can they run this Query through a MariaDB ANALYZE statement? 😅
Something like ANALYZE FORMAT=JSON to see the MariaDB execution/optimizer plan.
Any MariaDB version, buffer pool size, or page size would do. Would be interesting to see how it compares across different databases and from the previous version 👀
| migrations.AddIndex( | ||
| model_name='determination', | ||
| index=models.Index( | ||
| fields=['collectionmemberid', 'iscurrent', 'taxon'], | ||
| name='DetColMemCurrentTaxonIDX', | ||
| ), | ||
| ), |
There was a problem hiding this comment.
If we do keep this index as-is we should remove the DetMemIDX Index on Determination. That index is just CollectionMemberID.
It is (almost*) completely redundant (taking up storage space, slowing down writes, taking up room in the Buffer Pool, etc.) if we have a composite index with CollectionMemberID as the first column.
| models.Index( | ||
| fields=['collectionmemberid', 'iscurrent', 'taxon'], | ||
| name='DetColMemCurrentTaxonIDX', | ||
| ), |
There was a problem hiding this comment.
(note / optional)
Are there any other Queries we can modify to utilize this index? 👀
Would be nice if there were.
If I have the time, I'll do some investigation into this index (how it plays with other index pages in the buffer pool-- specifically whether it can significantly impact future buffer pool hits when the buffer is already otherwise full), increased write times for updating Determinations, cases where InnoDB will usually fall back to a full table scan (e.g., when using a single column of an index is estimated to return ~20-30% of objects in the table), verify the index size assuming default page size, see if there's another optimal ordering of the index, etc.
Fixes #303
Warning
This PR affects database migrations. See migration testing instructions.
This PR makes
taxon_barcollection-scoped and improves the performance of current-determination counts.For some reason, taxon rows were filtered by collection discipline, but determination counts were not restricted to the active collection. The count query also lacked an index matching its collection-scoped predicates (so it took sooo long to complete). In the future, we should consider using Materialized Views in PostgreSQL once we migrate since this should be cached for general cases (like statistics visualizations in most contexts).
performance_tiles.mp4
Changes
collectionmemberidmatches the active collection.DetColMemCurrentTaxonIDXon(collectionmemberid, iscurrent, taxon).User-visible changes
Performance
On the KU entomology database with 1,786,726 determinations and 42,520 taxa:
taxon_barwith the index: approximately 0.29–0.39 seconds.taxon_barwithout the index: approximately 4.10–4.22 seconds.Checklist
Testing
mainandissue-303(noting that this PR introduces a migration)Summary by CodeRabbit