funkwhale merge requestshttps://dev.funkwhale.audio/funkwhale/funkwhale/-/merge_requests2019-10-28T08:04:43Zhttps://dev.funkwhale.audio/funkwhale/funkwhale/-/merge_requests/939Audio denormalization / Performance enhancement in music API2019-10-28T08:04:43ZAgateAudio denormalization / Performance enhancement in music APISo, I've been thinking about how we can reduce the load on DB when querying artists/albums/tracks that are playable by a given user (this is a common query, see https://dev.funkwhale.audio/funkwhale/funkwhale/issues/865).
Basically, I...So, I've been thinking about how we can reduce the load on DB when querying artists/albums/tracks that are playable by a given user (this is a common query, see https://dev.funkwhale.audio/funkwhale/funkwhale/issues/865).
Basically, I've found two ways of enhancing the performance of those endpoints:
1. Denormalize the audio permission logic
2. Avoid SQL JOINS as much as possible
# Denormalization
THe current permission matrix is quite complex. When we want to display playable artists, tracks and albums, we need to filter first all the uploads the user has the persmission to play:
- Uploads in public libraries
- Uploads in followed libraries
- Uploads in personal liraries
- Uploads in internal libraries (if the user is logged in)
Doing so result in long and complex SQL queries (as shown in #865). Another approach is to maintain an intermediate table with this information (all tracks available to a user), and update this table whenever uploads are created/deleted, etc.
This is achived by my first commit.
Todo:
- [x] Update the table when:
- [x] Uploads are created
- [x] Uploads are deleted (automatic, via cascade)
- [x] Library is followed
- [x] Library is unfollowed
- [x] Library follow is deleted
- [x] Library visibility is updated
- [x] Setup a feature flag to ensure this can be disabled if there are any issues (`MUSIC_USE_DENORMALIZATION`)
- [x] Data migration to setup the table on existing deployments
- [x] Django command (`rebuild_music_permissions`) to populate/reset the table completely
# Avoid SQL joins
A typically recommended django optimization is to use `select_related()` on queryset to retrieve associated objects in a single query. This is usually effective because it means fewer roundtrips to the database, and no `N+1` query issues. However, under the hood, it requires a `JOIN` which is expensive and can lead to less performant query plans.
I've found out that using `prefetch_related()` is more efficient under some scenarios. It requires an additional SQL query to retrieve the linked objects (so 2 queries if you want tracks and associated albums), but both queries together execute faster than the single, bigger one with the JOIN.
This is implemented in the second commit.
# Performance testing
To ensure the impact of both patch, I've run some load testing on open.audio. The test can be found in the `library.py` file and is executed by https://locust.io/.
Test characteristics:
- 20 simultaneous clients
- 120s total time
- 3 urls queried:
- `/api/v1/albums?playable=True`
- `/api/v1/artists?playable=True`
- `/api/v1/tracks?playable=True`
Test launch:
- `pip install locustio`
- `export JWT_TOKEN="<token>"` for logged in session
- `locust -f api/tests/loadtesting/library.py -H https://open.audio -c 20 -r 3 --no-web -t 120`
Glossary:
- `Master` branch: test run with current code from the 0.20 release
- `ORM patch` branch: test run with patch to use `prefetch_related()` instead of `select_related()`, from commit 3be4fd3d
- `Denorm patch` branch: test run with patch use an intermediate table to store available tracks for each user, from commit dd747965
- `Requests`: number of requests executed during the tests, higher is better
- `Median`: median response time (in milliseconds), lower is better
## Aggregated results (all endpoints)
| Branch | Logged in | Requests | Median (ms) |
| --------------- | --------- | ----------- | ----------- |
| Master | N | **430** | **3200** |
| Master | Y | **406** | **3500** |
| ORM patch | N | **597** | **1700** |
| ORM patch | Y | **486** | **2400** |
| Denorm patch | N | **547** | **1900** |
| Denorm patch | Y | **522** | **2200** |
| Both patches | N | **653** | **1200** |
| Both patches | Y | **673** | **1300** |
As you can see, both patches, when applied separately have a positive impact on response time and amount of handled requests. When used in conjunction, the improvement is even better, since the number of handled requests increased by ~50%, and the median response time was divided by 2.5.
## Albums endpoint
URL: `/api/v1/albums?playable=True`
| Branch | Logged in | Requests | Min (ms) | Max (ms) | Median (ms) | Avg (ms) |
| --------------- | --------- | ----------- | -------- | -------- | ----------- | --------- |
| Master | N | **148** | 1653 | 10088 | **4300** | 4473 |
| Master | Y | **138** | 1213 | 8311 | **4800** | 4618 |
| ORM patch | N | **182** | 745 | 7151 | **2600** | 2730 |
| ORM patch | Y | **158** | 732 | 11259 | **3500** | 3963 |
| Denorm patch | N | **162** | 687 | 6840 | **3000** | 3036 |
| Denorm patch | Y | **194** | 706 | 13003 | **2900** | 3149 |
| Both patches | N | **200** | 542 | 7915 | **2000** | 2456 |
| Both patches | Y | **224** | 580 | 4714 | **2000** | 2138 |
## Artists endpoint
URL: `/api/v1/artists?playable=True`
| Branch | Logged in | Requests | Min (ms) | Max (ms) | Median (ms) | Avg (ms) |
| --------------- | --------- | ----------- | -------- | -------- | ----------- | --------- |
| Master | N | **147** | 571 | 5125 | **2300** | 2526 |
| Master | Y | **130** | 584 | 5276 | **2700** | 2671 |
| ORM patch | N | **201** | 410 | 3224 | **1400** | 1506 |
| ORM patch | Y | **157** | 497 | 7516 | **2000** | 2171 |
| Denorm patch | N | **187** | 311 | 4102 | **1400** | 1663 |
| Denorm patch | Y | **160** | 332 | 11480 | **1800** | 1843 |
| Both patches | N | **243** | 256 | 6249 | **930** | 1125 |
| Both patches | Y | **228** | 366 | 3657 | **1000** | 1117 |
## Tracks endpoint
URL: `/api/v1/tracks?playable=True`
| Branch | Logged in | Requests | Min (ms) | Max (ms) | Median (ms) | Avg (ms) |
| --------------- | --------- | ----------- | -------- | -------- | ----------- | --------- |
| Master | N | **135** | 745 | 5765 | **3100** | 3128 |
| Master | Y | **138** | 704 | 7481 | **3700** | 3731 |
| ORM patch | N | **214** | 424 | 3661 | **1400** | 1556 |
| ORM patch | Y | **171** | 374 | 8697 | **2100** | 2291 |
| Denorm patch | N | **198** | 367 | 4923 | **1800** | 2024 |
| Denorm patch | Y | **168** | 508 | 11727 | **2200** | 2311 |
| Both patches | N | **210** | 292 | 6881 | **970** | 1247 |
| Both patches | Y | **221** | 352 | 3609 | **1100** | 1170 |0.20.1https://dev.funkwhale.audio/funkwhale/funkwhale/-/merge_requests/942Resolve "Pagination of results in genres in Subsonic API does not work"2019-10-28T07:58:33ZAgateResolve "Pagination of results in genres in Subsonic API does not work"Closes #954
cc @funkwhale/reviewers-pythonCloses #954
cc @funkwhale/reviewers-python0.20.1https://dev.funkwhale.audio/funkwhale/funkwhale/-/merge_requests/941Api serializers enhancements2019-10-24T09:33:03ZAgateApi serializers enhancementscc @funkwhale/reviewers-pythoncc @funkwhale/reviewers-python0.20.1https://dev.funkwhale.audio/funkwhale/funkwhale/-/merge_requests/940Django cacheops2019-10-24T09:31:59ZAgateDjango cacheopsAdded a caching package to help with expensive `COUNT` queries (cf #865).
This is disabled by default, unless `CACHEOPS_DURATION` is set to a positive integer.
cc @funkwhale/reviewers-python
# Performance impact
This use the ...Added a caching package to help with expensive `COUNT` queries (cf #865).
This is disabled by default, unless `CACHEOPS_DURATION` is set to a positive integer.
cc @funkwhale/reviewers-python
# Performance impact
This use the same load test as in !939, but with cacheops enabled/disabled.
As you can see, enabling cacheops increase the number of handled requests, and reduce the median response time.
## Cacheops disabled
```
Name # reqs # fails Avg Min Max | Median req/s
----------------------------------------------------------------------------------------------------
GET /api/v1/albums?playable=True 125 0(0.00%) 4970 933 11216 | 4900 0.90
GET /api/v1/artists?playable=True 132 0(0.00%) 3222 354 8002 | 3000 1.40
GET /api/v1/tracks?playable=True 122 0(0.00%) 4374 655 11378 | 4100 1.50
----------------------------------------------------------------------------------------------------
Total 379 0(0.00%) 3.80
```
## Cacheops enabled
```
Name # reqs # fails Avg Min Max | Median req/s
----------------------------------------------------------------------------------------------------
GET /api/v1/albums?playable=True 173 0(0.00%) 3080 664 6935 | 2900 1.70
GET /api/v1/artists?playable=True 187 0(0.00%) 1503 283 4199 | 1400 1.30
GET /api/v1/tracks?playable=True 195 0(0.00%) 2149 481 4929 | 2100 1.20
----------------------------------------------------------------------------------------------------
Total 555 0(0.00%) 4.20
```
0.20.1https://dev.funkwhale.audio/funkwhale/funkwhale/-/merge_requests/938Fix tag exclusion in custom radios (#950)2019-10-21T07:25:37ZAgateFix tag exclusion in custom radios (#950)Closes #950
cc @funkwhale/reviewers-pythonCloses #950
cc @funkwhale/reviewers-python0.20.1https://dev.funkwhale.audio/funkwhale/funkwhale/-/merge_requests/931Resolve "Landing page displays improperly on 768p screen"2019-10-18T07:38:22ZAgateResolve "Landing page displays improperly on 768p screen"Closes #933
This ensure the password input doesn't overflow outside of the container (as shown in #933). As for the really narrow layout, I have no easy fix right now, but the work done in !923 will reduce the sidebar width and leave...Closes #933
This ensure the password input doesn't overflow outside of the container (as shown in #933). As for the really narrow layout, I have no easy fix right now, but the work done in !923 will reduce the sidebar width and leave more room to content, so it should make the issue less proeminent :)
cc @funkwhale/reviewers-front
![image](/uploads/d3da40ec60a24843a0a57968b982e62f/image.png)0.20.1https://dev.funkwhale.audio/funkwhale/funkwhale/-/merge_requests/929Fix #945: Fixed escaped pod name displayed on home/about page2019-10-18T07:38:17ZAgateFix #945: Fixed escaped pod name displayed on home/about pageCloses #945
Translations with params is a bit picky and if you want to disable escaping, you must use the `v-translate` attribute on an HTML element (like `<p>` or `<span>`) instead of `<translate>`
cc @funkwhale/reviewers-frontCloses #945
Translations with params is a bit picky and if you want to disable escaping, you must use the `v-translate` attribute on an HTML element (like `<p>` or `<span>`) instead of `<translate>`
cc @funkwhale/reviewers-front0.20.1https://dev.funkwhale.audio/funkwhale/funkwhale/-/merge_requests/927Added feedback via loading spinner when searching a remote library2019-10-18T07:38:13ZAgateAdded feedback via loading spinner when searching a remote libraryI've also fixed the library cards on the same page to ensure the library link is visible.
![Peek_2019-10-15_12-10](/uploads/eb8fb717a76bedc3c65a4ed390b8d755/Peek_2019-10-15_12-10.mp4)
cc @funkwhale/reviewers-frontI've also fixed the library cards on the same page to ensure the library link is visible.
![Peek_2019-10-15_12-10](/uploads/eb8fb717a76bedc3c65a4ed390b8d755/Peek_2019-10-15_12-10.mp4)
cc @funkwhale/reviewers-front0.20.1https://dev.funkwhale.audio/funkwhale/funkwhale/-/merge_requests/937Mitigate DB Connections issues2019-10-17T13:20:27ZAgateMitigate DB Connections issuesBy default, ASGI_THREADS is set to number of CPU * 5, which quickly exhaust the number of Postgres connections, especially with `FUNKWHALE_WEB_WORKERS>1`
This set a much more reasonable default (6), and ensure we close connections whe...By default, ASGI_THREADS is set to number of CPU * 5, which quickly exhaust the number of Postgres connections, especially with `FUNKWHALE_WEB_WORKERS>1`
This set a much more reasonable default (6), and ensure we close connections whenever possible (channels don't do this automatically), cf https://github.com/django/channels/issues/8710.20.1https://dev.funkwhale.audio/funkwhale/funkwhale/-/merge_requests/930Resolve "Library Upload: missing retry upload button"2019-10-17T13:19:09ZAgateResolve "Library Upload: missing retry upload button"Closes #942
cc @funkwhale/reviewers-front
# Demo (with a server that refuses 50% of uploads)
![Peek_2019-10-16_11-15](/uploads/9b9f81ad552d2cc0a3a2140dc032330c/Peek_2019-10-16_11-15.mp4)Closes #942
cc @funkwhale/reviewers-front
# Demo (with a server that refuses 50% of uploads)
![Peek_2019-10-16_11-15](/uploads/9b9f81ad552d2cc0a3a2140dc032330c/Peek_2019-10-16_11-15.mp4)0.20.1https://dev.funkwhale.audio/funkwhale/funkwhale/-/merge_requests/935S3 proxy fails on nginx2019-10-17T08:22:00ZAgateS3 proxy fails on nginxTested with Dag Stenstad on Matrix.
Basically, nginx didn't catch the whole URL when proxying the request to the S3 storage, because we didn't escaped the S3 signature that is included in query parameters.
cc @funkwhale/reviewers-p...Tested with Dag Stenstad on Matrix.
Basically, nginx didn't catch the whole URL when proxying the request to the S3 storage, because we didn't escaped the S3 signature that is included in query parameters.
cc @funkwhale/reviewers-python0.20.1https://dev.funkwhale.audio/funkwhale/funkwhale/-/merge_requests/936Add new whitlisted class for upward menus2019-10-17T08:20:21ZCiarán Ainsworthsporiff@funkwhale.audioAdd new whitlisted class for upward menusThe `purgecss` plugin removes the `upward` class from `fomantic`, meaning that menus do not open upwards properly when at the bottom of the page. This MR adds this to the whitelist to resolve the issue.
### Before
![before__1_](/upload...The `purgecss` plugin removes the `upward` class from `fomantic`, meaning that menus do not open upwards properly when at the bottom of the page. This MR adds this to the whitelist to resolve the issue.
### Before
![before__1_](/uploads/572f2a5d9dbf35bb9b2d0b3e0ab2250c/before__1_.png)
### After
![after__1_](/uploads/131e281d2f5bdca039795db23d0072a8/after__1_.png)0.20.1Ciarán Ainsworthsporiff@funkwhale.audioCiarán Ainsworthsporiff@funkwhale.audiohttps://dev.funkwhale.audio/funkwhale/funkwhale/-/merge_requests/928Fix #946: Fix import crash when importing M4A file with no embedded cover2019-10-17T07:52:40ZAgateFix #946: Fix import crash when importing M4A file with no embedded coverCloses #946
cc @funkwhale/reviewers-pythonCloses #946
cc @funkwhale/reviewers-python0.20.1https://dev.funkwhale.audio/funkwhale/funkwhale/-/merge_requests/925Resolve "PDF icon is used to represent playlists"2019-10-16T21:10:22ZAgateResolve "PDF icon is used to represent playlists"Closes #943Closes #9430.20.1https://dev.funkwhale.audio/funkwhale/funkwhale/-/merge_requests/934Added link to third-party helm chart in documentation2019-10-16T12:18:55ZAgateAdded link to third-party helm chart in documentation0.20.1https://dev.funkwhale.audio/funkwhale/funkwhale/-/merge_requests/924Resolve "Implement fromYear/toYear in Subsonic API"2019-10-16T07:32:40ZAgateResolve "Implement fromYear/toYear in Subsonic API"Closes #936
This is used by subsonic clients like DSub to power the "albums by decade" views
cc @funkwhale/reviewers-pythonCloses #936
This is used by subsonic clients like DSub to power the "albums by decade" views
cc @funkwhale/reviewers-python0.20.1https://dev.funkwhale.audio/funkwhale/funkwhale/-/merge_requests/926Fix #934: Documented how to create DB extension by hand in case of permission...2019-10-15T10:24:56ZAgateFix #934: Documented how to create DB extension by hand in case of permission...Fix #934: Documented how to create DB extension by hand in case of permission error during migrations
Closes #934
cc @SporiffFix #934: Documented how to create DB extension by hand in case of permission error during migrations
Closes #934
cc @Sporiff0.20.1https://dev.funkwhale.audio/funkwhale/funkwhale/-/merge_requests/920Resolve "Broken embed CSS"2019-10-07T08:56:18ZCiarán Ainsworthsporiff@funkwhale.audioResolve "Broken embed CSS"Closes #935
Embed CSS was broken due to the CSS classes being purged during build. This fix whitelists the `plyr` values to ensure they are not purged.
### Before
![Before](/uploads/ed3d28426bb04efe78235bc7552305e0/Before.png)
...Closes #935
Embed CSS was broken due to the CSS classes being purged during build. This fix whitelists the `plyr` values to ensure they are not purged.
### Before
![Before](/uploads/ed3d28426bb04efe78235bc7552305e0/Before.png)
### After
![After](/uploads/97650fd7bf175b96b2d938b0466f8fbe/After.png)0.20.1Ciarán Ainsworthsporiff@funkwhale.audioCiarán Ainsworthsporiff@funkwhale.audio