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

chore: optimize query #633

Closed
wants to merge 12 commits into from
Closed

Conversation

Cologler
Copy link
Contributor

@Cologler Cologler commented Apr 7, 2024

optimize the number of SQL queries when creating backups.

Before:

For each manga, execute one SQL query

Now:

For all manga, execute one SQL query

@@ -295,8 +297,11 @@ class MangaRestorer(
categories: List<Long>,
backupCategories: List<BackupCategory>,
) {
val dbCategories = getCategories.await()
val dbCategoriesByName = dbCategories.associateBy { it.name }
if (this.dbCategoriesMapByNameCache == null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Having this inline with a force non-null is really messy. Put it behind a function which fills the cache on first call instead. then this reduces to something like

val dbCategoriesByName = dbCategoriesByName()

Even though it is still effectively a side effect, it makes it more clear that this is just a cache fill. I would have suggested turning it into a delegated property, but suspend makes it difficult.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have moved this up to BackupRestorer

}
}.groupBy({ x -> x.first }, { x -> x.second })
} else {
null
Copy link
Contributor

Choose a reason for hiding this comment

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

Replace this with an empty map

manga_syncQueries.getTracks(backupTrackMapper)
}.groupBy({ x -> x.first }, { x -> x.second })
} else {
null
Copy link
Contributor

Choose a reason for hiding this comment

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

Here as well, replace this with an empty map

private suspend fun backupManga(
manga: Manga,
options: BackupOptions,
categoriesMap: Map<Long, List<Category>>?,
Copy link
Contributor

Choose a reason for hiding this comment

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

Make these non-null

@@ -48,16 +70,18 @@ class MangaBackupCreator(
}

if (options.categories) {
assert(categoriesMap != null)
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove the assert and null checks to the category map

mangaObject.categories = categoriesForManga.map { it.order }
}
}

if (options.tracking) {
val tracks = handler.awaitList { manga_syncQueries.getTracksByMangaId(manga.id, backupTrackMapper) }
if (tracks.isNotEmpty()) {
assert(trackingMap != null)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same thing, remove the null checks

suspend fun backupMangas(mangas: List<Manga>, options: BackupOptions): List<BackupManga> {
val categoriesMap: Map<Long, List<Category>>? = if (options.categories) {
handler.awaitList {
categoriesQueries.getCategoriesWithMangaId { mangaId, categoryId, name, order, flags ->
Copy link
Contributor

Choose a reason for hiding this comment

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

Move this up to an interactor

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This looks unnecessary and overly designed, as it will only be used during backup (load all into memory).

@Cologler
Copy link
Contributor Author

Cologler commented Apr 9, 2024

Even after refactoring, MangaBackupCreator.backupMangas() still takes 1 minute to process my 506 mangas in DEBUG mode.

Copy link
Contributor Author

@Cologler Cologler left a comment

Choose a reason for hiding this comment

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

I have no idea why this takes 17 seconds

image

@sirlag
Copy link
Contributor

sirlag commented Apr 9, 2024

I have no idea why this takes 17 seconds

image

If you profile your backups, where is it hanging?

@Cologler
Copy link
Contributor Author

Cologler commented Apr 9, 2024

I guess it's a debugger issue. After disconnecting the debugger, creating a backup is fast, almost close to 1 second.

@Cologler
Copy link
Contributor Author

Close now, as this may cause bugs. See #647.

@Cologler Cologler closed this Apr 15, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants