Skip to content

refactor: add more SIMD into OAHSet#7478

Open
BorysTheDev wants to merge 1 commit into
mainfrom
refactor_OAHSet_with_SIMD
Open

refactor: add more SIMD into OAHSet#7478
BorysTheDev wants to merge 1 commit into
mainfrom
refactor_OAHSet_with_SIMD

Conversation

@BorysTheDev
Copy link
Copy Markdown
Contributor

No description provided.

@qodo-free-for-open-source-projects
Copy link
Copy Markdown

Review Summary by Qodo

Refactor OAHSet SIMD operations into separate implementation file

✨ Enhancement 🧪 Tests

Grey Divider

Walkthroughs

Description
• Extracted inline SIMD probe logic from oah_set.h into separate oah_set.cc implementation file
  with FORCE_INLINE annotations for better code organization and maintainability
• Refactored core search operations (ProbeLanes, FindMatch, RefreshStaleCandidate,
  ProbeExtensionVector) into dedicated helper functions with clearer separation of concerns
• Simplified public API methods (Add, Find, Erase, AddMany) by moving implementation details
  to private helper functions while maintaining performance through forced inlining
• Added comprehensive stress test (SimdFindEraseStress) covering displacement window, extension
  vectors, lazy-zero hash cache, TTL expiry, and mixed live/erased/expired member resolution
• Enhanced SimdOp class with constexpr and noexcept qualifiers for compile-time evaluation
  support

Grey Divider

Qodo Logo

@qodo-free-for-open-source-projects
Copy link
Copy Markdown

qodo-free-for-open-source-projects Bot commented Jun 1, 2026

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0)

Grey Divider


Advisory comments

1. Public internal SIMD API 🐞 Bug ⚙ Maintainability
Description
OAHSet::LaneMasks and the template OAHSet::ProbeLanes are declared in the class public section even
though they are internal implementation details. This unnecessarily expands the public API and
leaves a publicly-declared template whose definition lives only in oah_set.cc, making accidental
external use/refactors riskier.
Code

src/core/oah_set.h[R167-179]

Evidence
The header exposes LaneMasks and declares the ProbeLanes template in the public section, while
the only definition is in the new .cc file and all current uses are within that TU. This indicates
they are internal implementation details and should not be part of the public interface.

src/core/oah_set.h[152-186]
src/core/oah_set.cc[18-27]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`LaneMasks` and `ProbeLanes` are internal probing helpers but are currently in `OAHSet`'s public API. This exposes implementation details and creates a publicly-declared template whose definition is only available in `oah_set.cc`.

### Issue Context
These helpers are only used by the new SIMD probing implementation in `oah_set.cc` and are not intended as part of the `OAHSet` external interface.

### Fix Focus Areas
- src/core/oah_set.h[167-186]
- src/core/oah_set.cc[18-27]

### What to change
- Move `struct LaneMasks` and `template <typename Wide> static LaneMasks ProbeLanes(...)` into the `private:` section of `OAHSet`, or alternatively keep them as `private` static helpers.
- If you expect future reuse outside the TU, move the template definition to the header (and keep it `private`) to avoid accidental ODR/visibility surprises.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

@augmentcode
Copy link
Copy Markdown

augmentcode Bot commented Jun 1, 2026

🤖 Augment PR Summary

Summary: Refactors OAHSet to push more of the hot-path probing logic into SIMD-based scans for both the displacement window and the extension vector.

Changes:

  • Moves `OAHSet` hot-path logic (`Add`, `AddMany`, `Find`, `Erase`) out of the header into a new implementation file: src/core/oah_set.cc
  • Introduces SIMD helpers (ProbeLanes, lane masks) and consolidates shared lookup logic via FindMatch/FindInternal
  • Adds refresh logic for lazy/stale extended-hash candidates during probing
  • Updates the build to compile the new TU (src/core/CMakeLists.txt)
  • Adds a stress test covering SIMD find/erase across window/vector paths and TTL expiry (SimdFindEraseStress)
  • Tightens `SimdOp` APIs with more constexpr/noexcept qualifiers

Technical Notes: The new probe path uses SIMD masks to shortlist key-compare candidates (including lazy-zero hash lanes) and applies expiry during probe to keep lookups/erases consistent.

🤖 Was this summary useful? React with 👍 or 👎

Copy link
Copy Markdown

@augmentcode augmentcode Bot left a comment

Choose a reason for hiding this comment

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

Review completed. 2 suggestions posted.

Fix All in Augment

Comment augment review to trigger a new review at any time.

Comment thread src/core/simd_op.h
}

static SimdOp Load(const T* ptr) {
static constexpr SimdOp Load(const T* ptr) noexcept {
Copy link
Copy Markdown

@augmentcode augmentcode Bot Jun 1, 2026

Choose a reason for hiding this comment

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

SimdOp::Load is now declared constexpr but it calls std::memcpy, which is not usable in constant evaluation in C++20 and can make the function definition ill-formed depending on the standard/library implementation. If this project is still built as C++20, this may break compilation (or at least makes the constexpr promise incorrect).

Severity: high

Fix This in Augment

🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.

Comment thread src/core/oah_set.h
reuse_slot = hit;
}
}
// Result of a SIMD probe over a run of OAHEntry lanes.
Copy link
Copy Markdown

@augmentcode augmentcode Bot Jun 1, 2026

Choose a reason for hiding this comment

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

LaneMasks/ProbeLanes look like internal helpers, but they currently live in the class public section, and ProbeLanes is a template only declared in the header (definition is in oah_set.cc). If any other TU ever tries to use it, it will fail to compile/link; consider keeping these helpers private (or fully header-defined) to avoid exposing a fragile API surface.

Severity: low

Fix This in Augment

🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.

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.

1 participant