Don't recalculate the song length for every added TCO while loading #5236
Conversation
…ading project, instead do it only once afterwards.
softrabbit
changed the title
|
|
|
On my system this change cut loading time down from 26 seconds to 5. (Pathological case: https://lmms.io/lsp/?action=show&file=10232). Compiled and run under identical conditions, of course. |
|
Right. With the pathological case and on my puny netbook I get a consistent result of 2 minutes 20 seconds without the patch and 20 seconds with it. |
|
Nice improvement in loading time. In my latest project current loading time was around 31:50 seconds in default lmms 1.2.0. Exactly same project was loaded in 24:40 seconds on lmms version avalaible here. It was quite heavy project with lot of VST instruments, plugins and automation going on. I think this nicely represents 'normal user case'. |
|
Wouldn't this be a good fix for lmms-1.2.1 ? |
|
I think yes. |
|
Merge? In stable-1.2? |
|
I saw no issues with this PR while testing for a couple of hours. |
|
I am no LMMS developer, but to me it seems this change forceably changes UpdateLength() to be a no-op during load. I could imagine some other part of the code actually having good reasons to call UpdateLength() and expect it to work, even when loading a song. Is it not possible to look for unnecessary calls of UpdateLength() and not call it from there while loading instead of making it a no-op? That would also solve the issue of long loading times, but it would leave UpdateLength() working for those that might have to rely on it. Also, even if you do not strictly have to rely on it, the current patch could easily lead to hard-to-debug bugs, since a developer, rightfully so, might assume UpdateLength() do what it is supposed to be doing, while in fact, with the patch, it will not longer always do so, and there is no indication to the calling function when it does not. |
Neither am I, TBH. I just throw a PR at the project now and then.
Well, the good reasons would IMO be GUI updates, playback and audio export (which really is a special kind of playback). I expect no playback of any kind to happen while loading a project, which leaves the GUI. I can live without the song editor being up-to-date behind the import dialog. None of the actual data in the song should be affected, as that is what the song length is calculated from, not the other way around.
You aren't wrong, and this might be the more correct way.
Looks like a valid point. |
softrabbit
added
rework required
|
I think #6741 may help fixing the issue which this PR tries to fix. |
|
It certainly helps quite a bit, but there can still be noticeable delay if the song has enough stuff in it. Currently the song length calculations are only needed for the horizontal scrollbar length in the Song Editor and for export. I can't think of any reasons for song length info ever being needed during project load, even hypothetically. Merging a PR like this is still something I'd say we should aim for. |
|
On a quick review it looks good to me: The update method is called after everything is loaded and m_loadingProject is unset. I'd tend to add a comment, since that adds an exception behavior to the method and it wouldn't hurt to make it more explicit in the code (I'd probably comment above the |
|
Seems like this PR was supposed to be merged long ago but was left out. The fix doesn't seem to horrible too. @zonkmachine does this PR still have a scope? |
|
The performance problem is still in master. A debug build of master loads the file in the original description in 20 s whereas with the patch of this PR it only takes 3 s. It seems to be an open question whether this PR should be merged or #6741. |
|
I asked lost on discord, I'll update once i get the reply. I personally feel this PR looks better from the diff. |
|
I have just checked out @LostRobotMusic's branch, merged with master and loaded the file. With #6741 it takes 5 s to load the file. So it's:
Perhaps both PRs could be combined. |
|
I left a comment in this thread about this around a year ago. My PR and this PR are tackling two entirely separate and unrelated issues, they have literally nothing to do with each other and should both be merged. My PR removes the exponential nature of the song length calculation, speeding it up by hundreds of times, which mainly demolishes the enormous lag that results from moving/deleting/copying groups of clips in large projects (and coincidentally improves project loading speed). This PR makes it so the song length doesn't even need to be calculated in the first place for project loading specifically, which is a good thing because there's no reason for the song editor length to be calculated here at all. Merging only my PR would leave behind these unnecessary calculations during project load. Merging only this PR would leave behind 100% of the current debilitating lag present in all song editor operations. They're unrelated PRs solving separate issues and shouldn't be compared in any way. |
…MMS#5236) Don't make LMMS calculate the song length for every added TCO when a new project is created or a project is loaded. Instead do it only once afterwards. This is accomplished by preventing any calculations in `Song::updateLength` if a song is currently loaded. `Song::updateLength` is then called immediately after the loading flag has been set to `false` in both cases. --------- Co-authored-by: IanCaio <iancaio_dev@hotmail.com>
Don't make LMMS calculate the song length for every added TCO when loading project, instead do it only once afterwards. Fix for #3312, solution suggested and investigated by @michaelgregorius ages ago.