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

Fix very slow response to checkbox changes #2354

Merged
merged 1 commit into from
Mar 18, 2018

Conversation

HebaruSan
Copy link
Member

@HebaruSan HebaruSan commented Mar 11, 2018

Problem

Checking or unchecking a checkbox in the main mod list can be very slow, sometimes taking over a minute! Some slowness was observed previously, but it got worse with v1.24.0.

Causes

My investigation found three sources of slowness.

Redundant ZIP validation

This line took 8+ seconds in one test that I observed, while everything else was quick:

ListViewItem item = new ListViewItem()
{
Text = CurrentInstance.Cache.IsCachedZip(change.Mod.ToModule())
? $"{change.Mod.Name} {change.Mod.Version} (cached)"
: $"{change.Mod.Name} {change.Mod.Version} ({change.Mod.ToModule()?.download.Host ?? ""}, {change.Mod.DownloadSize})"
};

The culprit is IsCachedZip. This function performs validation of the ZIP file in the cache for the given module, including CRC checks. This could easily take several seconds for a large download. As of #2277, Main.UpdateChangesDialog was running it for every mod listed in the (un)installation confirmation dialog so it could determine whether to tell the user the mod was already "(cached)".

If the mod isn't in the cache, this call is fast. That's why the problem seemed more severe for uninstallation: if you're uninstalling a mod, that means it's installed, which means it's probably in your cache.

Double reaction to clicks

Relatedly, all of the post-click work is being done twice. When the user clicks, ModList_CellValueChanged calls GUIMod.SetInstallChecked, which sets DataGridViewCheckBoxCell.Value, which calls ModList_CellValueChanged again! It stops after two iterations because GUIMod.IsInstallChecked is set and checked, but each time through, ModList_CellValueChanged recalculates all the dependencies and conflicts. So the baseline response time for clicking a checkbox is twice as long as it should be.

Slow lookups in Registry.LatestAvailableWithProvides

This function is responsible for the core of a lot of CKAN's relationship logic. It takes as its parameters a module identifier and two constraints, a game version and a relationship specification. Its job is to return every CkanModule that could reasonably satisfy the given requirement (but only the latest one per identifier).

Unfortunately it works by searching the entire registry for modules compatible with the given version, then exhaustively checking every one of them for a matching "provides" property. A typical call to this function takes 400 milliseconds, and we need a lot of them for many common operations. For example, if you click to install USI-LS, you'll see around 3 seconds of delay while the registry is searched and searched again for each of its 4 dependencies.

Changes

Fixes #2310.

Less ZIP validation

Now we use IsMaybeCachedZip instead. This function just checks whether a file associated with the requested URL exists in the cache, without running the extra validation checks. This makes more sense now that we prevent invalid files from entering the cache as of #2243.

The same change is made for the same reasons to a similar call in UpdateRecommendedDialog, affecting recommended and suggested mods, and for ModuleInstaller's installation confirmation prompt.

Single reaction to clicks

GUIMod.SetInstallChecked no longer sets DataGridViewCheckBoxCell.Value if it already has the desired value. SetInstallChecked is a bit funny in that the source of its input is not clearly defined; it might be called to react to a change in the grid (when all that should happen is that we set GUIMod.IsInstallChecked), or it might be called to initiate a change to the grid (when DataGridViewCheckBoxCell.Value needs to be changed). The behavior now should be better in the reactive case without harming the initiating case.

Index the "provides" property

Now instead of exhaustively searching the registry in LatestAvailableWithProvides, we precalculate an index for it to use when the registry is first initialized. This index contains a mapping from provided identifiers to providing AvailableModules. When we need to look up a provided module, the index lets us jump to the specific set of AvailableModules that we need to consider, and we simply ask them to provide us their latest modules matching our constraints. We then filter these to ensure that the latest version actually provides our target identifier, in case their "provides" relationships have since been modified to no longer do so.

This change reduces the typical run time of LatestAvailableWithProvides from 400 milliseconds to essentially 0. There are positive knock-on effects throughout the code base, wherever this function is used:

  • Selecting or deselecting mods is instantaneous
  • The relationships tab in GUI is noticeably faster
  • The install flow is sped up
  • Recommendation and suggestion listings are sped up

Recommendations obey version requirements

Building on these changes, since LatestAvailableWithProvides now always obeys the version properties of a relationship, the GUI code for optional dependencies is updated to pass the specific versions found based on those relationships to the installer. This is done by tracking suggested and recommended modules as full CkanModule objects instead of string identifiers, which also saves a few redundant lookups here and there.

This is also fixed in ConsoleUI, which now also shows the versions of optional dependencies.

Fixes #2319.

Other changes

A bunch of the mod list code in Main.cs is moved to MainModList.cs to make it easier to find when investigating this sort of thing. No functional changes are made to this code.

GUIUser is split out from Main.cs into its own GUIUser.cs file. No functional changes here either.

The GUI change preview screen could give wrong versions for dependencies, because we were using GUIMod.Version, which is always the latest compatible version. Now it's fixed by using CkanModule.version instead, which is the true version being installed.

@HebaruSan HebaruSan added Bug Something is not working as intended GUI Issues affecting the interactive GUI Pull request and removed Bug Something is not working as intended labels Mar 11, 2018
@linuxgurugamer
Copy link
Contributor

Oh Yay, this was sooo bad. Thank you for fixing it.

@HebaruSan HebaruSan force-pushed the fix/checkbox-delay branch 5 times, most recently from e47fe09 to 6a80031 Compare March 12, 2018 17:44
@HebaruSan HebaruSan added the Registry Issues affecting the registry label Mar 12, 2018
@HebaruSan HebaruSan force-pushed the fix/checkbox-delay branch 5 times, most recently from 5741423 to 900701b Compare March 12, 2018 23:08
@HebaruSan HebaruSan added the Relationships Issues affecting depends, recommends, etc. label Mar 15, 2018
@politas politas merged commit 6fe078c into KSP-CKAN:master Mar 18, 2018
politas added a commit that referenced this pull request Mar 18, 2018
@politas politas removed Bug Something is not working as intended Pull request labels Mar 18, 2018
@HebaruSan HebaruSan deleted the fix/checkbox-delay branch March 18, 2018 16:16
politas added a commit to politas/CKAN that referenced this pull request Mar 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GUI Issues affecting the interactive GUI Registry Issues affecting the registry Relationships Issues affecting depends, recommends, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Version requirements not obeyed for recommends Slow when choosing mods to install or uninstall
3 participants