Skip to content

perf: Use HashSet for O(1) secret masking lookups#1723

Merged
thomhurst merged 1 commit into
mainfrom
fix/secret-masker-hashset
Jan 1, 2026
Merged

perf: Use HashSet for O(1) secret masking lookups#1723
thomhurst merged 1 commit into
mainfrom
fix/secret-masker-hashset

Conversation

@thomhurst
Copy link
Copy Markdown
Owner

Summary

Fixes #1539

Changed BuildSystemSecretMasker from List<string> to HashSet<string> for tracking already-masked secrets.

Changes

  • Replaced List<string> with HashSet<string> for _alreadyMaskedSecrets
  • Use HashSet.Add() return value to check if already exists (returns false if duplicate)
  • Removed LINQ .Where() filter in favor of direct HashSet operation

Performance

Operation Before After
Lookup per secret O(n) O(1)
MaskSecrets overall O(n*m) O(m)

Where n = existing secrets, m = new secrets

Test plan

  • Build succeeds
  • Verify secret masking still works correctly

🤖 Generated with Claude Code

Fixes #1539

Changed BuildSystemSecretMasker from List<string> to HashSet<string> for
tracking already-masked secrets. This provides O(1) lookup instead of O(n),
improving performance when many secrets need to be masked.

Before: O(n*m) where n=existing secrets, m=new secrets
After: O(m) - each new secret is O(1) lookup

HashSet.Add() returns false if the item already exists, eliminating the
need for a separate Contains() check.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings January 1, 2026 17:07
@thomhurst
Copy link
Copy Markdown
Owner Author

Summary

Performance optimization that replaces List with HashSet for O(1) secret deduplication in BuildSystemSecretMasker.

Critical Issues

None found ✅

Suggestions

None - this is a clean performance improvement with correct implementation.

Verdict

APPROVE - No critical issues

Analysis:

  • Changes _alreadyMaskedSecrets from List to HashSet for O(1) vs O(n) lookups
  • Replaces Where(s => !_alreadyMaskedSecrets.Contains(s)) with idiomatic HashSet.Add() pattern
  • Maintains thread-safety with existing lock
  • Improves performance in secret masking hot path without changing behavior
  • Comment clearly documents the optimization rationale

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR optimizes the secret masking performance by replacing a List<string> with a HashSet<string> for tracking already-masked secrets, reducing lookup complexity from O(n) to O(1).

Key Changes:

  • Changed _alreadyMaskedSecrets from List<string> to HashSet<string>
  • Leveraged HashSet.Add() return value to eliminate duplicate checking logic
  • Removed LINQ .Where() filter in favor of direct HashSet operations

@thomhurst thomhurst merged commit d6158a3 into main Jan 1, 2026
17 of 18 checks passed
@thomhurst thomhurst deleted the fix/secret-masker-hashset branch January 1, 2026 17:26
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.

Code smell: BuildSystemSecretMasker uses List for O(n) contains checks

2 participants