-
Notifications
You must be signed in to change notification settings - Fork 297
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
Works search performance #1163
Works search performance #1163
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting! Would it make sense to simplify things and always create the primary key? I guess the potential drawback would be that creating the index might be more of a penalty than the win of a faster lookup could be?
I think the reason why I changed the SQL in most other queries to be based on the search results probably were the result of similar challenges. And that wouldn't take advantage of the index. Would you mind measuring performance of those searches with and without the primary key? It would simplify the change - I don't like additional parameters unless they're really needed.
Slim/Control/Queries.pm
Outdated
@@ -4722,8 +4727,7 @@ sub worksQuery { | |||
} | |||
|
|||
if (defined $libraryID) { | |||
$sql .= 'JOIN library_track ON library_track.track = tracks.id '; | |||
push @{$w}, 'library_track.library = ?'; | |||
push @{$w}, 'EXISTS (SELECT * FROM library_album WHERE library_album.album = albums.id AND library_album.library = ?)'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to SELECT 1 ...
instead of SELECT * ...
, as we don't really need the data? Similar to when they say that SELECT COUNT(1)
was faster than SELECT COUNT(*)
.
Slim/Plugin/FullTextSearch/Plugin.pm
Outdated
my $searchSQL = "SELECT SUBSTR(fulltext.id, 33) AS id, FULLTEXTWEIGHT(matchinfo(fulltext)) AS fulltextweight FROM fulltext WHERE fulltext MATCH 'type:$type $tokens' $orderOrLimit"; | ||
if ($createPrimaryKey) { | ||
$dbh->do("CREATE $temp TABLE $name (id integer primary key, fulltextweight integer)"); | ||
$searchSQL = "INSERT INTO $name $searchSQL"; | ||
} else { | ||
$searchSQL = "CREATE $temp TABLE $name AS $searchSQL"; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer if you did a separate CREATE $temp
in all cases, so we can have the actual $searchSQL
identical.
I'd re-consider reversing the SQL from starting on tracks to start on the search result. So instead of this:
do something like
? That would solve the index creation challenge, wouldn't it? That's not the actual code change, but you get the idea. Start with the |
I've rerun my tests and done some extra ones. See below. Caveat: These results are in sqllitebrowser, running sqlite 3.39.2 (my LMS runs sqlite 3.22). For some reason the queries run faster in LMS/DBI, but the relative performance improvements are similar. Based on this and your suggestion, I'll go ahead and rewrite the SQL in worksQuery when $search is passed in (and bypass processing other parameters, just in case). I'll also address the other points you've made. Particularly see my test of the albumsQuery search with/without creating a primary key on the temporary table: based on this I'll make the temp table primary key creation unconditional. worksSearch: SQL 1 (same as standard Works query)
SQL 2 (starting with workssearch, also omitting the dobule join to contributors because I don't think we ever run worksQuery with both $search and non-composer $artist/$roleID)
Live Search (example, Albums. Searching for "b" - 24,268 rows in temporary table):
And albumsQuery with $search
|
Nice job - thanks a lot! |
Changes pushed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's give that a try! Thanks a lot!
Add parameter to create search helper tables with a primary key. This is the magic bullet.
Works Query: replace joins to
library_track
withEXISTS
check - runs much faster with large libraries. The Works search query can be made even faster by using different SQL when$search
is passed in, but this is pretty good already and avoids possible inadvertent logic changes.Tested with a large library (25,000 albums, 288,000 tracks, 8400 works, 20,000 contributors).
The livesearch still takes about 30 seconds when restricting to local music only (on an old Chromebook running Debian) to resolve for example "beethoven 5" with the above library because as we know it runs 5 queries for every typed character.
It's substantially faster when searching all music (ie without the
library_track
check) so that might be improved further with more work on join columns and/or indexes. But I think we should go with this and see what the users think.