Support shared mbvar#3129
Merged
chenBright merged 2 commits intoapache:masterfrom Nov 1, 2025
Merged
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for shared (thread-safe) bvar management in MultiDimension by introducing a template parameter Shared that controls whether get_stats returns a raw pointer or std::shared_ptr. When Shared=true, the shared pointer approach prevents use-after-free issues when deleting bvars concurrently.
Key changes:
- Added
Sharedtemplate parameter (defaulting to false for backward compatibility) toMultiDimension - Modified return types to use
std::shared_ptrwhenShared=true, raw pointers whenShared=false - Moved test code (
MyStringViewclass andTestLabelsfunction) frombvar_mvariable_unittest.cpptobvar_multi_dimension_unittest.cppand added new concurrency test
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| test/bvar_mvariable_unittest.cpp | Removed MyStringView class definition, TestLabels function, and labels test; cleaned up unused variable |
| test/bvar_multi_dimension_unittest.cpp | Added MyStringView class, TestLabels function, labels test, and new thread-safety test for shared bvar |
| src/bvar/multi_dimension_inl.h | Updated all method signatures and implementations to support Shared template parameter; replaced nullptr with NULL for consistency |
| src/bvar/multi_dimension.h | Added Shared template parameter, conditional typedef for value_ptr_type, and helper methods for creating/deleting values based on Shared flag |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
c4d16f5 to
55cd389
Compare
Contributor
|
Note: The shared mbvar may be less performant than the non-shared version. |
055426e to
c914c2d
Compare
Contributor
Author
Documentation and comments have been updated. |
Contributor
|
LGTM |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: resolve #3069
Problem Summary:
MultiDimension get_stats returns a raw pointer, making it unsafe to delete bvar via delete_stats.
What is changed and the side effects?
Changed:
MultiDimension<BvarType, KeyType, true> get_stats supports returning shared bvar(std::shared_ptr), so bvar can be safely deleted via delete_stats.
Side effects:
Performance effects:
Breaking backward compatibility:
Check List: