Improve MeasureBaseList lookup performance #27569
Conversation
ghost
added this to 
miiizen
force-pushed
the
measureListImprovements
branch
from
68b2396 to
f965a66
Compare
miiizen
force-pushed
the
measureListImprovements
branch
from
b78fdc9 to
596d7ce
Compare
src/engraving/dom/measurebase.cpp
Outdated
| } | ||
|
|
||
| auto mbIt = findMeasureBaseIterator(m); | ||
| if (mbIt != m_tickIndex.end()) { |
There was a problem hiding this comment.
This feels like it should be an assertion? Or is there a legitimate case where it may happen? (Same in a bunch of other places below)
src/engraving/dom/measurebase.cpp
Outdated
| } | ||
| m_first = e; | ||
| m_first = m; | ||
| m_tickIndex.emplace(std::make_pair(m->tick().ticks(), m)); |
There was a problem hiding this comment.
This looks a bit weird to me. With push_back, that's fine. But with push_front, this means that all measures in this score will potentially change tick, so I would have expected that we need to reconstruct the multimap here? Instead, by just emplacing the new measure at the start (at least as far as I understand) we are putting the multimap into a state that we know to be wrong.
I think that the reason why we don't update the multimap here is that at this stage the tick of the measure may not yet be known? But then why inserting it?
src/engraving/dom/measurebase.cpp
Outdated
|
|
||
| void MeasureBaseList::updateTickIndex() | ||
| { | ||
| std::multimap<int, MeasureBase*> indexCopy = m_tickIndex; |
There was a problem hiding this comment.
This pairs with my comment above. At this point this structure is assumed have wrong ticks. But if it has wrong ticks, then it's not functioning as a map, it's just holding the list of measures... but you already have the list of measures, so why bother working on this at all (and spend the time to make a copy)? Unless there's something that I'm not understanding, I think you could entirely avoid updating m_tickIndex during all the MeasureList operations, and when you get here you simply clear it and update it by looping on MeasureList.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| return nullptr; | ||
| } | ||
|
|
||
| std::vector<MeasureBase*> MeasureBaseList::measureBasesAtTick(int tick) const |
There was a problem hiding this comment.
I think we have already something for this? Like muse::values or something similar that returns all the values for a particular key in a multimap?
This function is primarily to be used when reading or importing scores. It updates the map as we can guarantee the ticks of the other measures won't change.
miiizen
force-pushed
the
measureListImprovements
branch
from
efa559a to
315968d
Compare
github-project-automation
bot
moved this from In Progress
to Done
in MuseScore Studio 4.6
3 participants
This PR improves the performance of
tick2measure,tick2measureMMandtick2measureBaseby maintaining anstd::multimapof ticks andMeasureBase*alongside the existing linked list.multimapis much faster to query than the existing linked list, but adds some slowdown and overhead on adding and removing measures from the list. Firstly, the measure must be added to the map, then the map must be regenerated in order to update each measure's tick value. (One possible improvement would be to regenerate the map only from the start tick of layout)std::multimapis implemented as a red-black treeThis slowdown is acceptable, as we are querying this list far more often than we are adding measures. In the Beethoven 9 example, when opening the project
MeasureBaseList::addis called 2,662 times, whereas we calltick2measure&tick2measureMMa total of 281,246 times.Profiling
Any
MeasureBaseListor utility measure access function not in the table had a small enough impact to not be picked up by the profiler before or after the PR. %change is the change in sum time. A positive %change is a speedup, negative is a slowdown.