Fix multi-minute freeze when generating anvil interaction help on large modlists#132
Fix multi-minute freeze when generating anvil interaction help on large modlists#132Maeyanie wants to merge 2 commits into
Conversation
…dlists BlockAnvil.GetPlacedBlockInteractionHelp queries GetRequiredAnvilTier for every handbook stack of every IAnvilWorkable item. With CollectibleBehaviorCastToolHead attached to everything matching ToolHeadSelector, each query resolved the metal material from scratch: - The ItemStack overload of GetOrCacheMetalMaterial bypassed the material cache for all behavior-based workables (any collectible whose class is not IAnvilWorkable), calling the uncached resolver every time. - Failed resolutions (null) were never cached, so items with no metal material re-ran the full fallback on every call. - The fallback linearly scanned all smithing recipes and all grid recipes x ingredients per call. Fix: route both fallback paths of the ItemStack overload through the collectible-level cache; cache negative results once MetalMaterialLoader has resolved its materials (new MaterialsResolved flag prevents premature nulls from poisoning the cache); and replace the linear recipe scans in GetSmithingRecipe, GetSmithingRecipesAsIngredient and GetGridRecipesAsIngredient with one-time reverse indexes keyed by collectible code, built lazily via ObjectCacheUtil. On a large modlist this cuts the first anvil tooltip from ~2 minutes of freeze to ~200 ms. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 43 minutes and 59 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR improves caching reliability and performance across the metal material and recipe lookup pipelines. A new ChangesCaching and Resolution Readiness
Possibly Related PRs
Poem
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
SmithingPlus/Util/CollectibleExtensions.cs (1)
102-102: 💤 Low valueConsider adding a clarifying comment for the deduplication logic.
The condition
list[^1] != recipeprevents adding the same recipe multiple times when that recipe has repeated ingredients (e.g., a recipe requiring 2 iron ingots). While the logic is correct, a brief comment would improve maintainability.📝 Example comment
if (code == null) continue; if (!dict.TryGetValue(code, out var list)) dict[code] = list = []; + // Prevent duplicate entries when a recipe has the same ingredient multiple times if (list.Count == 0 || list[^1] != recipe) list.Add(recipe);Also applies to: 121-121
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@SmithingPlus/Util/CollectibleExtensions.cs` at line 102, Add a brief clarifying comment explaining the deduplication check "if (list.Count == 0 || list[^1] != recipe) list.Add(recipe);" (and the identical check at the other occurrence) to state that the guard prevents adding the same recipe twice when a recipe contains repeated ingredients (e.g., requires 2 iron ingots), i.e., it only deduplicates consecutive identical entries by comparing the last item (list[^1]) to the current recipe before adding; place the comment immediately above the conditional so future maintainers understand the intent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@SmithingPlus/Util/CollectibleExtensions.cs`:
- Line 102: Add a brief clarifying comment explaining the deduplication check
"if (list.Count == 0 || list[^1] != recipe) list.Add(recipe);" (and the
identical check at the other occurrence) to state that the guard prevents adding
the same recipe twice when a recipe contains repeated ingredients (e.g.,
requires 2 iron ingots), i.e., it only deduplicates consecutive identical
entries by comparing the last item (list[^1]) to the current recipe before
adding; place the comment immediately above the conditional so future
maintainers understand the intent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 69df2750-0108-4efc-bb30-f59952086fad
📒 Files selected for processing (3)
SmithingPlus/Common/Metal/MetalMaterialExtensions.csSmithingPlus/Common/Metal/MetalMaterialLoader.csSmithingPlus/Util/CollectibleExtensions.cs
Add comment suggested by coderabbitai.
1 participant
Problem
The first time a player looks at an anvil after launch,
BlockAnvil.GetPlacedBlockInteractionHelpbuilds its workable-item list by callingGetRequiredAnvilTier(stack)for every handbook stack of everyIAnvilWorkableitem. On a large modlist this froze the game for ~2 minutes per anvil tier (profiled hotspot:GetGridRecipesAsIngredient).The cost came from three compounding issues in the metal material resolution that the tier check runs through:
ItemStackoverload ofGetOrCacheMetalMaterialbypassed the cache. For any collectible whose class doesn''t implementIAnvilWorkable(i.e. every behavior-based workable such as theCastToolHeaditems matched byToolHeadSelector), it called the private uncachedGetMetalMaterialon every invocation.CacheHelper.GetOrAddskips storingnull, so items with no resolvable metal material - exactly the ones that hit the expensive fallback, e.g. non-metal items matched by the broad defaultToolHeadSelectorregex (head|blade|boss|barrel|stirrup|part) - re-ran the full resolution on every single call.Fix
GetSmithingRecipe,GetSmithingRecipesAsIngredient, andGetGridRecipesAsIngredientnow build a one-time reverse index (recipes keyed by output/ingredient collectible code) lazily viaObjectCacheUtil, turning each lookup into a dictionary hit.GetOrCacheMetalMaterial(CollectibleObject)now caches negative results too. A newMaterialsResolvedflag onMetalMaterialLoaderguards this so lookups that happen beforeAssetsFinalizecan''t poison the cache with premature nulls.ItemStackoverload routes both of its fallback paths through the cached collectible-level method.CacheHelper.GetOrAddis left unchanged since other callers rely on its retry-on-null behavior. The indexed lookups return the same recipes the old LINQ queries did (minus exact duplicates of the same recipe, which no caller depended on).Results
Tested on a large modlist: the first anvil tooltip went from a ~2 minute freeze to a ~200 ms stutter. Behavior is otherwise unchanged.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Improvements