Skip to content

perf: optimize SkillCraftPaths sorting to avoid N+1 query problem #2

Description

@rsned

Problem

The skill_craft_paths sorting implementation has a performance concern due to N+1 queries during sorting.

Current Implementation

In internal/crafting/engine/skill_paths.go, the sorting logic performs recipe lookups inside the sort comparison function:

for i := range paths {
    sort.Slice(paths[i].RecipesUnlocked, func(j, k int) bool {
        // Each comparison does 2 recipe lookups
        recipeJ, errJ := e.recipes.GetRecipe(ctx, paths[i].RecipesUnlocked[j])
        recipeK, errK := e.recipes.GetRecipe(ctx, paths[i].RecipesUnlocked[k])
        // ... sorting logic
    })
}

Performance Impact

For a skill with M recipes:

  • Sorting performs O(M log M) comparisons
  • Each comparison makes 2 recipe lookups (GetRecipe calls)
  • Total: O(M log M) database/store calls per skill

This is significantly less efficient than craft_query.go and component_uses.go, which sort complete recipe objects that have already been fetched.

Root Cause

SkillUnlockPath.RecipesUnlocked stores recipe IDs ([]string) rather than recipe objects, requiring lookups during sorting.

Proposed Solution

  1. Fetch recipe details before sorting:

    • After collecting recipesAtNext (line 67-70), fetch full recipe objects
    • Store recipe objects (or a lighter struct with ID, Name, Category) instead of just IDs
  2. Update data structures:

    • Modify SkillUnlockPath.RecipesUnlocked from []string to []Recipe or a lighter struct
    • This aligns with the pattern used in craft_query and component_uses
  3. Sort without additional lookups:

    • After recipes are fetched, sort by accessing Recipe.Category directly
    • Eliminates O(M log M) database calls during sorting

Breaking Changes

This would require:

  • Changing crafting.SkillUnlockPath.RecipesUnlocked type from []string to []Recipe (or custom struct)
  • Updating any consumers of this field

Alternative Approaches

If changing the data structure is not feasible:

  • Cache recipe lookups during sorting to reduce duplicate calls
  • Pre-fetch all recipes into a map before sorting

Priority

Low - Current implementation is functionally correct and passes all tests. This is a performance optimization, not a bug fix.

References

  • Introduced in commit e7007ef (feat: apply category tier sorting to skill_craft_paths results)
  • Code review notes from category priority ordering implementation

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions