Open mixxxbot opened 2 years ago
mixxxbot
commented
2 years ago Commented by: daschuer Date: 2017-08-02T11:43:25Z
Aren't collapsed regions parsed anyway? I am afraid we can only slice this by introducing a michanism that allows to skip skin regions by a preferences option or something. An other idea is to allow to duplicate an ready initialised widget in memory. Mmm .. all a lot of work.
mixxxbot
commented
2 years ago Commented by: Be-ing Date: 2017-08-03T00:58:41Z
Aren't collapsed regions parsed anyway?
Yes.
An other idea is to allow to duplicate an ready initialised widget in memory. Mmm .. all a lot of work.
The sampler rows are already in singleton widgets.
I think if we remove widgets from the samplers this could work. Every widget that is added to the sampler template is instantiated 64 times. Even if we reduce the number of samplers to 32 or 16 or 8 we still have issues with scaling.
mixxxbot
commented
2 years ago Commented by: ronso0 Date: 2017-08-03T17:54:35Z
What do you mean by much faster?
With 64 samplers mixxx is ready in 12sec, with just 8 samplers it takes 10sec.
mixxxbot
commented
2 years ago Commented by: Be-ing Date: 2017-08-03T18:39:52Z
What do you mean by much faster?
Running mixxx with --logLevel debug and Tango, it takes about 10 seconds to delete the skin (measured by the time reported with "deleting menubar"). Removing everything but the tag from sample_decks.xml, that goes down to 3.6 seconds.
mixxxbot
commented
2 years ago Commented by: Be-ing Date: 2017-08-03T18:41:00Z
We have a scaling issue with just 8 samplers, so I don't think reducing the number of samplers is a good solution.
mixxxbot
commented
2 years ago Commented by: ronso0 Date: 2017-08-06T17:54:22Z
You're right, shutting down & reloading a skin is much faster.
mixxxbot
commented
2 years ago Commented by: ronso0 Date: 2017-11-07T08:56:49Z
I don't really see which Widgets could be removed from Tango samplers without cutting expected features. The only element I'm not sure about are the hotcue buttons, though I doubt that significantly improves loading time (didn't find a good way to accurately measure the loading time): As a workaround for long tracks with where the samplers cue point should be altered in samplers, those tracks could be loaded into decks first, be prepared and the dragged to any sampler deck.
mixxxbot
commented
2 years ago Commented by: ronso0 Date: 2017-11-07T09:03:35Z
A global approach could be to set the number of samplers in the Preferences window (8 being the default?) while skins could define the maximum. Controller mappings could overwrite this value if a controller is built to support more samplers. I guess this would accelerate loading on slower machines (8 vs. 64 samplers in Tango and Deere)
mixxxbot
commented
2 years ago Commented by: ronso0 Date: 2017-11-14T16:38:42Z
I wanted to check if a sampler rework is really worth the effort, so I tested this with my stopwatch. Via commandline I started just 'mixxx' with all 64 samplers loaded, all have cover art available.
'mini + maxi' means normal samplers with separate templates for collapsed and expanded version.
start shutdownmini + maxi 13s 28s mini only 12s 13s maxi only 12s 23s no samplers 12s 10s
As you can see only shutdown time is affected, startup time stays almost steady. What's done when a skin is deleted? Why does it take so much longer than building it? Maybe it's better to optimize that process instead of pruning the samplers and cutting usability.
mixxxbot
commented
2 years ago mixxxbot
commented
2 years ago Commented by: Be-ing Date: 2017-11-14T16:54:13Z
My first thought was the saving the loaded samples to the samplers.xml file could be slowing it down, but I think that would happen even if the sampler skin widgets weren't loaded. Have you tested commenting out the whole sampler section of the skin?
mixxxbot
commented
2 years ago Commented by: Be-ing Date: 2017-11-23T18:02:46Z
I did some further testing to determine the scaling issues with the skin widgets versus instantiating so many samplers in the engine. I commented out the setting of [Master],num_samplers in Deere's skin manifest and the sample_decks.xml template, plus deleted my samplers.xml file in my configuration folder. With this setup, Mixxx took about 325 MB resident memory and deleting the skin on shutdown took about 3 seconds. Uncommenting the sample_decks.xml template from the skin kept memory usage the same but increased shutdown time by a whole second. Uncommenting the setting of [Master],num_samplers to 64 in the skin manifest brought memory usage up to 575 MB and shutdown time was the same.
mixxxbot
commented
2 years ago Commented by: ronso0 Date: 2017-12-14T17:44:08Z
For now I vote for limiting the number of samplers. On my machine (quadcore @1.8GHz) it now takes around 30 seconds to shutdown when I enable all 64 samplers in Tango...
IMO only a minority of users is expecting a total of 64 samplers, while all the others have to deal with the resulting longer start/shutdown times. As I mentioned, I don't expect cutting sampler features is good approach either, see complaint above.
I see that we can't create a mechanism to set the numbers of samplers dynamically via Preferences > Interface or in another way with reasonable effort, and neither will we come come up with a good solution to the underlying skin problem soon.
But we can limit the number of samplers to 16 for Tango and Deere, and create duplicate skins called Deere (64 samplers) and Tango (64 samplers). The maintanance load would be minimal. This way it would absolutely clear for the users, and the majority would benefit until we come up with a general solution.
mixxxbot
commented
2 years ago Commented by: daschuer Date: 2017-12-14T20:06:22Z
Duplicating the skins will work for me. We but we should only duplicate the a single skin file. Thinking about it, redirecting to a different skin folder is probably the same work than a preferences option .... what do you think.
mixxxbot
commented
2 years ago Commented by: Be-ing Date: 2017-12-15T21:45:12Z
I have thought about the duplicate skins solution. It is ugly, but it may be the best option for now...
mixxxbot
commented
2 years ago Commented by: wunjo Date: 2017-12-15T21:52:54Z
From my perspective I can't imagine using more than 16 samplers and I definitely use the volume knob. Reducing the memory footprint is surely a good thing so duplicating skins might be fine :-)
mixxxbot
commented
2 years ago Commented by: ronso0 Date: 2017-12-18T00:26:00Z
Cool, feels like we have a compromise!
All skins are less than 1MB. What are your concerns to copy the whole skin folder and just alter the skin name in skin.xml?
In Deere and Tango we need to change two files: sample_decks.xml and corresponding skin menu button that sets the numbers of sampler rows. I'd use sample_decks.xml as switchpoint that loads either sample_decks_16.xml or ~_64.xml and do the same with the row selection button.
I already doing this for my personal branch. I can implement the changes for both Deere and Tango before the friday ;)
mixxxbot
commented
2 years ago Commented by: Be-ing Date: 2017-12-18T00:57:48Z
My concern with duplicating skins is that every little change would have to be made to both copies of the skin, which would be a big hassle.
"I'd use sample_decks.xml as switchpoint that loads either sample_decks_16.xml or ~_64.xml and do the same with the row selection button."
Hmm, that gives me an idea. We could make a new preference option to select the number of samplers and have the skin load a different template depending on the preference option.
mixxxbot
commented
2 years ago Commented by: Be-ing Date: 2017-12-18T00:58:30Z
This issue is annoying, but IMO it is not such a big deal that it needs to be rushed for the beta release. We can take care of it during the beta period.
mixxxbot
commented
2 years ago Commented by: ronso0 Date: 2017-12-18T01:16:22Z
My concern with duplicating skins is that every little change would have to be made to both copies of the skin, which would be a big hassle.
No hassle at all: after a skin was changed (64 sample decks), just clone it to replace the old version with 16 samplers, then
I'll put a patch into each skin directory. I'm knee-deep in the Mixxx skins right now, so it'll take less time than writing this.
mixxxbot
commented
2 years ago Commented by: ronso0 Date: 2017-12-18T01:21:01Z
Hmm, that gives me an idea. We could make a new preference option to select the number of samplers and have the skin load a different template depending on the preference option.
Nice, that's pretty much what I suggested here a month ago ;) Also, the button (button stack) to set the number of sampler rows can be instantiated as Singleton in skin.xml, so it would be really just one file to change.
mixxxbot
commented
2 years ago Commented by: ronso0 Date: 2017-12-18T02:01:58Z
It's working fine. Though, this is only a workaround, no matter if it's an option in preferences or a duplicate skin, so I won't assign this bug to me.
In the next days I'll open PRs for Deere and Tango with this change and small fixes, so both (and LateNight) should be in good shape when beta is released. Afterwards I'll have to design in the so-called real world again and won't have much time for skin design..
mixxxbot
commented
2 years ago Commented by: daschuer Date: 2017-12-18T07:07:16Z
I am pro a persistent preferences option in the view menu. This is are just a view lines and can be done before beta. Defaulting to 8 sounds reasonable. What do you think?
mixxxbot
commented
2 years ago Commented by: Be-ing Date: 2017-12-18T07:27:45Z
Adding a combobox to the preferences would be easy. But how would skins make use of it to load a different template depending on the preference?
mixxxbot
commented
2 years ago Commented by: Be-ing Date: 2017-12-18T07:29:25Z
It might be a good idea to restrict the number of samplers in the engine in PlayerManager::addSampler to respect the preference option. That would limit RAM wasted by unused samplers.
mixxxbot
commented
2 years ago Commented by: daschuer Date: 2017-12-18T16:46:30Z
I think we need a conditional widgets. That are only instance instead of a connected co has a certain value.
mixxxbot
commented
2 years ago Commented by: Be-ing Date: 2017-12-21T03:28:09Z
Lazy loading singletons: https://github.com/mixxxdj/mixxx/pull/1426
Reported by: Be-ing Date: 2017-08-01T18:49:46Z Status: Triaged Importance: Medium Launchpad Issue: lp1707991
Deere and Tango now can show 64 samplers. In regards to the engine, 64 samplers is no big deal if samplers can't be assigned to effects, but loading the skin widgets for 64 samplers is a major bottleneck for Mixxx's startup and shutdown times. If you comment out the whole sampler template in Deere or Tango they load and shut down much faster. Using completely separate templates for collapsed and expanded sampler views exacerbates this. Let's decide on which sampler features are strictly necessary to show in skins. We can keep the collapsed/expanded view toggling, but the expanded view should show a secondary WidgetGroup below what is shown in the collapsed view, not replace the whole sampler with a different WidgetGroup.