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

De-over-parallelize Versions tab #4049

Merged
merged 1 commit into from
Mar 3, 2024

Conversation

HebaruSan
Copy link
Member

@HebaruSan HebaruSan commented Mar 3, 2024

Problems

  • The Versions tab sometimes has a delay when loading; it might sit for several seconds before starting to set the background colors of compatible rows to green.
  • An extreme consequence of this can be observed by opening the Versions tab before typing a search for a mod (e.g., "ksp community fixes"): the whole app appears to hang while waiting for the Versions tab to update.

Cause

We appear to have a case of over-parallelization. RelationshipResolver.ModList is parallel (for speeding up very large changesets), and Versions.checkInstallable calls it in parallel (via ModuleInstaller.CanInstall; that's what feeds the ToList call highlighted above).

When parallel calls are nested, I think the outer one sets up some thread pooling and marshalling and so on to distribute load to your N CPUs, and then the inner calls each do the same thing, so you effectively have threads competing for N*N CPUs, which is N*(N-1) more than you have.

Changes

  • Now EnumerableExtensions.AsParallelIf is added to create a way to make a PLINQ call parallel only if its parameter is true
  • Now RelationshipResolver.ModList has a parallel parameter (defaulting to true to keep it parallel for the situations where it's helpful) and passes it to AsParallelIf
  • Now ModuleInstaller.CanInstall tells ModList not to load in parallel

This follows the recommendation of the above link to keep the outer loop parallel while making the inner loop non-parallel, and it makes the Versions tab load much quicker now.

It's unfortunate that we have to hard-code whether a specific call should be parallel instead of just writing optimal queries and letting the framework sort out the parallelization, but that's where we are. Ideally we would create our own way to toggle it automatically, but this will work for now, and we can revisit it later if we think of a better solution.

Side changes:

  • CkanModule.FromJson and CkanModule's constructor now initialize the _comparator field in a simpler way
  • The Versions tab now uses BeginUpdate/EndUpdate to minimize flickering of the listview while setting multiple row colors in a row

Fixes #4046.

@HebaruSan HebaruSan added Bug GUI Issues affecting the interactive GUI Core (ckan.dll) Issues affecting the core part of CKAN Performance Something's slower than it should be labels Mar 3, 2024
@HebaruSan HebaruSan merged commit fb53afb into KSP-CKAN:master Mar 3, 2024
8 checks passed
@HebaruSan HebaruSan deleted the fix/over-parallel branch March 3, 2024 00:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Core (ckan.dll) Issues affecting the core part of CKAN GUI Issues affecting the interactive GUI Performance Something's slower than it should be
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Performance hitch when typing in the search box while versions tab is open
1 participant