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

Use caching to improve performance of large profiles #1151

Merged
merged 1 commit into from
Jan 22, 2024

Conversation

anttimaki
Copy link
Collaborator

When the local mod list is browsed vue seems to render the whole list again on some user actions. This causes three methods of the component to get called repeatedly:

  • getMissingDependencies, which uses modifiableModList property, which is based on the mod list stored in VueX store. modifiableModList is used by other parts of the component as well, so I thought better not touch it at this time since figuring out unintended side effects would be a lot of work
  • getThunderstoreModFromMod, which used the Thunderstore mod list stored in VueX, but now uses the list stored in the ThunderstorePackages. As far as I can tell both are updated in the Splash view and by the scheduled background process in UtilityMixin. Ergo this shouldn't break things, but this is the most significant functional change in this commit, and therefore most likely culprit should problems arise
  • isLatestVersion, which did and still does use ThunderstorePackages for its shenanigans

So while this commit doesn't reduce the incessant function calls, it caches the results to a simple object to reduce required calculations. Effects were tested with a profile containing a mod pack with 109 mods. Completing the following tasks were timed (roughly and manually), with the accompanying results (original vs. cached):

  • Initial rendering of the local mod list when moving from profile selection view: 7.0s vs. 5.4s
  • Opening modal to disable a mod with two dependants: 4.4s vs. 1,2s
  • Closing the modal without disabling the mod: 4.4s vs. 1.2s
  • Opening modal to uninstall the same mod: 4.5s vs. 1.0s
  • Uninstalling the mod: 15.8s vs 6.0s

(There might be further changes for optimizing the uninstall process, since it seems some stuff is done after each dependant is uninstalled, while it MIGHT be enough to do it just once in the end.)

For a small profile with 3 mods there's no noticeable difference between the performance of the old and new implementation.

@anttimaki anttimaki marked this pull request as draft January 10, 2024 16:23
@anttimaki
Copy link
Collaborator Author

Forgot to implement the cache clearing when the package list is updated, so changed this to draft.

When the local mod list is browsed vue seems to render the whole list
again on some user actions. This causes three methods of the component
to get called repeatedly:

- getMissingDependencies, which uses modifiableModList property, which
  is based on the mod list stored in VueX store. modifiableModList is
  used by other parts of the component as well, so I thought better not
  touch it at this time since figuring out unintended side effects
  would be a lot of work
- getThunderstoreModFromMod, which used the Thunderstore mod list
  stored in VueX, but now uses the list stored in the
  ThunderstorePackages. As far as I can tell both are updated in the
  Splash view and by the scheduled background process in UtilityMixin.
  Ergo this shouldn't break things, but this is the most significant
  functional change in this commit, and therefore most likely culprit
  should problems arise
- isLatestVersion, which did and still does use ThunderstorePackages
  for its shenanigans

So while this commit doesn't reduce the incessant function calls, it
caches the results to a simple object to reduce required calculations.
Effects were tested with a profile containing a mod pack with 109 mods.
Completing the following tasks were timed (roughly and manually), with
the accompanying results (original vs. cached):

- Initial rendering of the local mod list when moving from profile
  selection view: 7.0s vs. 5.4s
- Opening modal to disable a mod with two dependants: 4.4s vs. 1,2s
- Closing the modal without disabling the mod: 4.4s vs. 1.2s
- Opening modal to uninstall the same mod: 4.5s vs. 1.0s
- Uninstalling the mod: 15.8s vs 6.0s

(There might be further changes for optimizing the uninstall process,
since it seems some stuff is done after each dependant is uninstalled,
while it MIGHT be enough to do it just once in the end.)

For a small profile with 3 mods there's no noticeable difference
between the performance of the old and new implementation.
@anttimaki anttimaki marked this pull request as ready for review January 11, 2024 10:11
@anttimaki
Copy link
Collaborator Author

Now clears cache when new package list is downloaded from the API.

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.

Seems to work as expected, local mod imports included (which I'm guessing you didn't explicitly account for)

Comment on lines 32 to 38
const mod: ManifestV2 = new ManifestV2().fromReactive(vueMod);
const latestVersion: ThunderstoreVersion | void = ModBridge.getLatestVersion(mod, ThunderstorePackages.PACKAGES);
if (latestVersion instanceof ThunderstoreVersion) {
return mod.getVersionNumber()
.isEqualTo(latestVersion.getVersionNumber()) || mod.getVersionNumber().isNewerThan(latestVersion.getVersionNumber());
return mod.getVersionNumber().isEqualOrNewerThan(latestVersion.getVersionNumber());
}
return true;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could this entire function be removed? Doesn't seem to be used outside of a single place now

@MythicManiac MythicManiac merged commit 64acae7 into develop Jan 22, 2024
4 checks passed
@MythicManiac MythicManiac deleted the profile-performance branch January 22, 2024 22:57
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.

2 participants