Memoize lazily evaluated sequences #2953
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
HebaruSan
added
Bug
HebaruSan
force-pushed
the
fix/ienumerable
branch
from
e67caf6 to
548322e
Compare
This does reduce the startup time immensely, nice job! diff --git a/GUI/GUIMod.cs b/GUI/GUIMod.cs
index 06198755..69649ed2 100644
--- a/GUI/GUIMod.cs
+++ b/GUI/GUIMod.cs
@@ -196,9 +196,6 @@ public GUIMod(CkanModule mod, IRegistryQuerier registry, KspVersionCriteria curr
public GUIMod(string identifier, IRegistryQuerier registry, KspVersionCriteria current_ksp_version, bool? incompatible = null)
{
Identifier = identifier;
- IsIncompatible = incompatible
- ?? registry.AllAvailable(identifier)
- .All(m => !m.IsCompatibleKSP(current_ksp_version));
IsAutodetected = registry.IsAutodetected(identifier);
DownloadCount = registry.DownloadCount(identifier);
if (registry.IsAutodetected(identifier))
@@ -216,6 +213,8 @@ public GUIMod(string identifier, IRegistryQuerier registry, KspVersionCriteria c
{
}
+ IsIncompatible = incompatible ?? LatestCompatibleMod is null;
+
// Let's try to find the compatibility for this mod. If it's not in the registry at
// all (because it's a DarkKAN mod) then this might fail.
The debug output will go from (master) over (your branch) to (with my diff applied) |
|
I like it!! |
|
I'm just gonna drop the file names + line numbers where my IDE reports List of reports
|
|
Nice! Latest commit should address all of those. |
DasSkelett
approved these changes
Jan 8, 2020
|
I feel like the universe was reminding me that paying attention to weird output and tracking it down is almost always rewarded. 🎉 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Background
In #2928 I learned how
yield returnworks; yes, it pauses execution of the function until we request the next element of the sequence, but if you try to access earlier members of the sequence, the whole function will start over again from the beginning, re-running any expensive logic that we were hoping to skip. In other words, it's lazily-evaluated but not memoizing.This is particularly inconvenient when we want to retrieve and use a sequence and take a special action when it's empty. The idiomatic approach is to use
.Any()followed by aforeach, but this suffers from the duplicate execution problem described above; any expensive steps that we take at the start of the function will be repeated (including network requests!). #2928 worked around this with abool foundAny, which is kind of painful to read.Changes
Now a new
.Memoize()LINQ-style function is added toCKAN.Extensionsbased on some work from StackOverflow. It acts as a memoizing wrapper around another enumerable. Each item of the upstream sequence is requested once and only once the first time a downstream consumer requests it, after which it is stored in a list, from which any subsequent requests for the same item are served without accessing the upstream enumerable. This will allow us to use lazy evaluation safely.This function is used to clean up some instances I found of duplicated execution of enumerables (I searched for
.Any()and checked whether there was aforeachlater for the same variable). If you find more, I'd be happy to add more fixes here.Separate but related, I noticed when running GUI with
--debugthat we seem to check a lot of module compatibilities over and over. I think this is becauseAvailableModule.Latest's use ofLastOrDefaultrequires iterating over its whole sequence. Now weReversethe sequence (which can be done quickly for aSortedDictionary) and useFirstOrDefaultinstead to reduce the number of modules checked.