Skip to content

Conversation

meiravgri
Copy link
Collaborator

(late) backport of #505 to 0.7.
+
add getAlignment() to VecSimIndexAbstract API

#505)

* vaseline for BM

* fix to baseline

* replace stack with allocation

* use unique ptr

* revert format batch iterator

* fix lifetime

* fix

* fix

* fix

* rearrange

* use ref to allocator instead of pointer

* disable flow temp

(cherry picked from commit ab96a8d)
Copy link
Contributor

@Copilot 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 replaces variable length arrays (VLAs) on the stack with heap allocation to improve memory safety and portability. The change removes the use of stack-allocated arrays with dynamic sizes, which are not supported in all C++ standards and can cause stack overflow issues with large data sizes.

  • Replaces VLA declarations with allocate_aligned_unique() and allocate_unique() calls
  • Introduces new allocator methods that return std::unique_ptr with custom deleters for automatic memory management
  • Adds getAlignment() method to the VecSimIndexAbstract API

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/VecSim/vec_sim_tiered_index.h Replaces stack-allocated VLAs with heap allocation in wrapper methods
src/VecSim/vec_sim_index.h Replaces stack-allocated VLAs with heap allocation in abstract class wrapper methods
src/VecSim/memory/vecsim_malloc.h Adds new allocator methods for unique_ptr-based allocation with custom deleter
src/VecSim/memory/vecsim_malloc.cpp Implements the new allocator methods for unique_ptr-based allocation
src/VecSim/algorithms/hnsw/hnsw_tiered.h Replaces VLA blob copy with heap allocation in insert job execution
src/VecSim/algorithms/hnsw/hnsw_serializer.h Replaces VLAs with heap allocation in graph restoration methods
src/VecSim/algorithms/hnsw/hnsw.h Replaces VLA for element graph data with heap allocation

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.


private:
// Retrive the original requested allocation size. Required for remalloc.
// Retrieve the original requested allocation size. Required for remalloc.
Copy link
Preview

Copilot AI Sep 2, 2025

Choose a reason for hiding this comment

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

The word 'remalloc' should be 'realloc' to match the standard C library function name.

Suggested change
// Retrieve the original requested allocation size. Required for remalloc.
// Retrieve the original requested allocation size. Required for realloc.

Copilot uses AI. Check for mistakes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

remalloc is cute

Copy link

codecov bot commented Sep 2, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.70%. Comparing base (c761e6f) to head (5483011).
⚠️ Report is 1 commits behind head on 0.7.

Additional details and impacted files
@@            Coverage Diff             @@
##              0.7     #761      +/-   ##
==========================================
+ Coverage   95.64%   95.70%   +0.06%     
==========================================
  Files          69       69              
  Lines        4221     4238      +17     
==========================================
+ Hits         4037     4056      +19     
+ Misses        184      182       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

alonre24
alonre24 previously approved these changes Sep 2, 2025
@meiravgri meiravgri added this pull request to the merge queue Sep 2, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 2, 2025
@meiravgri meiravgri requested a review from alonre24 September 2, 2025 15:43
@meiravgri meiravgri added this pull request to the merge queue Sep 3, 2025
Merged via the queue into 0.7 with commit e66b684 Sep 3, 2025
19 checks passed
@meiravgri meiravgri deleted the backport-505-to-0.7 branch September 3, 2025 05:23
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