optimize database access and ScoreSongOrder, MostSungSongOrder and FileTimeSongOrder #989
Conversation
|
Download the artifacts for this pull request:
This service is provided by nightly.link. These artifacts will expire in 90 days and will not be available for download after that time. |
BWagener
force-pushed
the
feature/optimize-sorting
branch
from
d3728c8
to
c7da2cc
Compare
| @@ -261,7 +261,6 @@ void ScreenSing::instrumentLayout(double time) { | |||
|
|
|||
| void ScreenSing::activateNextScreen() | |||
| { | |||
| m_database.addSong(m_song); | |||
There was a problem hiding this comment.
Looking for advice why this was being called
BWagener
force-pushed
the
feature/optimize-sorting
branch
from
457edbd
to
5c940e2
Compare
BWagener
changed the title
BWagener
force-pushed
the
feature/optimize-sorting
branch
from
a4c73fe
to
aa3643d
Compare
|
|
||
| Hiscore::HiscoreVector Hiscore::queryHiscore(std::optional<PlayerId> playerid, std::optional<SongId> songid, std::string const& track, std::optional<unsigned> max) const { |
There was a problem hiding this comment.
While this method is very generic and could be used from many contexts, it was only being used from a single one. Accessing one of the maps was the more efficient alternative so I replaced the usage and deleted this function.
| @@ -66,7 +66,7 @@ void Players::addPlayer (std::string const& name, std::string const& picture, st | |||
| pi.name = name; | |||
| pi.picture = picture; | |||
|
|
|||
| pi.id = id.value_or(assign_id_internal()); | |||
There was a problem hiding this comment.
assign_id_internal() would always be called, as value_or doesn't short circuit.
| this pointer. | ||
| */ | ||
| std::shared_ptr<Song> song; | ||
|
|
There was a problem hiding this comment.
it is no longer being used anywhere
| sort_internal(); | ||
| } | ||
|
|
||
| // TODO: determine appropriate time and location to call this function |
There was a problem hiding this comment.
Looking for help on this
| @@ -36,73 +42,54 @@ void SongItems::save(xmlpp::Element* songs) { | |||
|
|
|||
| SongId SongItems::addSongItem(std::string const& artist, std::string const& title, std::optional<SongId> _id) { | |||
| SongItem si; | |||
| si.id = _id.value_or(assign_id_internal()); | |||
There was a problem hiding this comment.
assign_id_internal() would always be evaluated here, as value_or can't short circuit
|
@BWagener there are type issues (look at the mac build). I think somewhere the song id is an |
Ah thanks, I was mistakenly assuming it's an unrelated issue as the other builds were successful. Would it be okay to explicitly cast them to SongId? I intentionally defined the id as a signed int on the Song class to handle missing ids. With the changes made in this PR it is guaranteed every loaded song has an ID, I just needed to track the state of a new song to check it here An alternative would be using an optional instead, what do you think? |
|
IIRC I started with a value for missing/undefined ids but it was refactored away with a std::optional. Even if that was not my own idea I would suggest this solution. Reason: You tell (with code) where an id is defined and where it could be undefined. That does not work if you simply use an |
BWagener
force-pushed
the
feature/optimize-sorting
branch
from
f58a1c9
to
f064e0b
Compare
BWagener
force-pushed
the
feature/optimize-sorting
branch
from
f064e0b
to
5c81eb0
Compare
|
What does this PR do?
Currently with a large library using the
ScoreSongOrderandMostSungSongOrdersorting options is very slow (for me 10s per sort and per keystroke when searching while the sort is active).I took the following approach to improve on this:
ScoreSongOrder,MostSungSongOrderandFileTimeSongOrdercan be executed on every keystroke when searchingsets, which require iteration and artist&title matching to access a specific song's information - this PR updates all data structures for DB data tounordered_maps (findoperation has complexity of O(1) on average and O(n) in the worst case, compared to O(log n) forsets)ScoreSongOrder,MostSungSongOrderandFileTimeSongOrderis stored inmaps - this PR updates all data structures for sorting data tounordered_maps (findoperation has complexity of O(1) on average and O(n) in the worst case, compared to O(log n) formaps)Here are some anecdotal results from measuring the performance
All tests conducted with Debug builds
Startup
Launch (~24k songs, empty cache, empty db)
no noticeable change
this pr (3 separate launches)
master (3 separate launches)
Launch (~24k songs, with cache, with db)
~8s vs ~12.5s - 50% faster
this pr (3 separate launches)
master (3 separate launches)
Sorting
ScoreSongOrder (~24k songs, ~1.7k high scores)
~333x faster initialization as well as subsequent sorts
this pr (repeatedly triggering the tested sort order)
master (repeatedly triggering the tested sort order)
MostSungSongOrder (~24k songs, ~1.7k high scores)
similar initialization - subsequent sorts 333x faster
this pr (repeatedly triggering the tested sort order)
master (repeatedly triggering the tested sort order)
FileTimeSongOrder (~24k songs)
similar initialization - subsequent sorts 6x faster
this pr (repeatedly triggering the tested sort order)
master (repeatedly triggering the tested sort order)
Todos