Skip to content
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

Make DisableModModal more self-sustained #1216

Merged
merged 5 commits into from
Feb 22, 2024
Merged

Conversation

anttimaki
Copy link
Collaborator

No description provided.

@anttimaki
Copy link
Collaborator Author

Changes in this PR allows running "disable BepInEx and all dependants" on a profile with 282 mods in under 20 seconds, whereas before the changes it took 40+ seconds.

While the changes in this particular PR doesn't seem to affect the speed of the first local mod list rendering, it's worth noting the effect this four PR stack has on that performance (numbers with same 282 mod profile and dev build):

  • develop branch: ~16 seconds
  • update-all-query-performance on top of develop branch: ~9 seconds
  • This branch on top of develop branch: ~20 seconds
  • update-all-query-performance on top of this branch on top of develop branch: ~13 seconds

In other words the changes in this stack seems to eat roughly half of the gains from update-all-query-performance. On the other hand the changes do speed different use cases after the list have been rendered significantly, and at least IMO cleans up the code quite nicely.

Anyway the question remains, are these changes something we're willing to merge? I don't think makes much sense for me to build on top of this stack before I have the answer to that. I'll try to see if I can improve the performance without messing with the same files, and if not, then do something unrelated to performance instead.

src/utils/Profile.ts Outdated Show resolved Hide resolved
Base automatically changed from mod-card-dependencies to develop February 15, 2024 11:16
@anttimaki anttimaki force-pushed the disable-mod-modal branch 2 times, most recently from 147d83b to a2d0ab9 Compare February 15, 2024 12:55
@anttimaki
Copy link
Collaborator Author

Based on Discord discussion, changed the PR so that mod disabling is done via Vuex. Initial rendering of a profile with 282 mods now takes ~3 seconds, thanks to other changes that have been merged since the PR was first opened.

Copy link
Collaborator

@MythicManiac MythicManiac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spotted a couple of potential improvement areas within the context of this PR directly, but nothing I'd consider a blocker myself.

src/components/views/LocalModList/DisableModModal.vue Outdated Show resolved Hide resolved
src/components/views/LocalModList/DisableModModal.vue Outdated Show resolved Hide resolved
src/store/modules/ProfileModule.ts Outdated Show resolved Hide resolved
There's no need to pass dependants list as a prop since it can be as
easily be determined internally.

Dependency list prop was dropped altogether since it's not used.
The implementation imitates what's currently being done by
LocalModList.performDisable().

This helps remove business logic and driect references to providers
from components, hopefully supporting reuse and easier refactoring.
The modal now handles disabling of mods entirely internally.
LocalModCard now disables mods without dependants internally, and
delegates mods with dependants to DisableModModal, effectively cutting
out the middleman LocalModList.
@anttimaki anttimaki merged commit 54bcab7 into develop Feb 22, 2024
7 checks passed
@anttimaki anttimaki deleted the disable-mod-modal branch February 22, 2024 12:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants