Skip to content

Conversation

@panbanda
Copy link
Owner

Summary

  • Fix critical performance bug where get_element was doing O(n) linear search instead of O(1) indexed lookup
  • The elements_by_id index was being populated but not used effectively

Problem

The get_element method retrieved an index from elements_by_id but then discarded it and did a linear search through the element vectors:

pub fn get_element(&self, path: &str) -> Option<&dyn Element> {
    let _idx = self.elements_by_id.get(path)?;  // Index retrieved but NEVER USED!
    
    // Then does O(n) linear search...
    let parts: Vec<&str> = path.split('.').collect();
    match parts.len() {
        1 => {
            if let Some(sys) = self.systems.iter().find(|s| s.get_full_path() == path) {
                return Some(sys as &dyn Element);
            }
            self.persons.iter().find(|p| p.get_full_path() == path)
                .map(|p| p as &dyn Element)
        }
        // ...more linear searches
    }
}

Solution

Changed elements_by_id to store (ElementType, usize) instead of just usize. Now get_element uses true O(1) lookup:

pub fn get_element(&self, path: &str) -> Option<&dyn Element> {
    let (element_type, idx) = self.elements_by_id.get(path)?;
    
    match element_type {
        ElementType::Person => self.persons.get(*idx).map(|p| p as &dyn Element),
        ElementType::System => self.systems.get(*idx).map(|s| s as &dyn Element),
        ElementType::Container => self.containers.get(*idx).map(|c| c as &dyn Element),
        ElementType::Component => self.components.get(*idx).map(|c| c as &dyn Element),
    }
}

Performance Impact

For a model with N elements:

  • Before: O(N) per lookup (linear search through vectors)
  • After: O(1) per lookup (hash map + direct array index)

This affects:

  • get_element - directly optimized
  • get_elements_by_type - calls get_element internally
  • get_elements_by_tag - calls get_element internally
  • get_children - calls get_element internally

Other Changes

Test plan

  • All 214 tests pass
  • cargo clippy passes with no warnings
  • cargo fmt --check passes
  • CI pipeline green

Generated with Claude Code

The elements_by_id index was storing (path -> vec_index) but the
get_element method was discarding the index and doing a linear search
through the element vectors anyway.

This change:
- Stores (ElementType, index) in elements_by_id instead of just index
- Rewrites get_element to use direct array indexing based on the stored
  element type, achieving true O(1) lookup
- Uses enumerate() instead of manual index tracking in build_indexes

For models with many elements, this provides significant performance
improvement for element lookups.

Also includes the conditional asset test fix from PR #8.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@panbanda
Copy link
Owner Author

@panbanda Ready for review. This fixes a significant performance bug where get_element was O(n) instead of O(1).

@panbanda panbanda merged commit 0c2dd9f into main Jan 11, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants