Skip to content

Support generics for MultiDimension APIs#3026

Merged
wwbmmm merged 2 commits intoapache:masterfrom
chenBright:generics_mbvar
Jul 23, 2025
Merged

Support generics for MultiDimension APIs#3026
wwbmmm merged 2 commits intoapache:masterfrom
chenBright:generics_mbvar

Conversation

@chenBright
Copy link
Copy Markdown
Contributor

@chenBright chenBright commented Jul 13, 2025

What problem does this PR solve?

Issue Number:

Problem Summary:

The current mbvar get/set operation APIs all require the construction of std::liststd::string, which requires the allocation of memory and the copying of strings, resulting in noticeable performance overhead.

What is changed and the side effects?

Changed:

The bvar get/set operation API supports more data structures through generics, thus achieving zero copy. Using Container<const char* / std::string_view / butil::StringPieces> can avoid string copying, where Container can be a container such as std::vector, std::set, std::map, etc. Furthermore, std::array and absl::InlinedVector can be used to avoid dynamic heap memory allocation.

Side effects:

  • Performance effects:

  • Breaking backward compatibility:


Check List:

@chenBright chenBright requested a review from Copilot July 13, 2025 15:20
@chenBright chenBright changed the title Support generics for MVariable api Support generics for MVariable APIs Jul 13, 2025
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 refactors the MVariable API to support generic label container types and centralizes common static methods under a new MVariableBase class.

  • Introduce MVariableBase for shared behavior and rename existing static methods (count_exposed, list_exposed, describe_exposed, dump_exposed) to live on it
  • Convert MVariable into a template class parameterized by KeyType for label containers and update MultiDimension accordingly
  • Update tests to exercise the new generic API and adjust CMake and consumers to call through MVariableBase

Reviewed Changes

Copilot reviewed 10 out of 11 changed files in this pull request and generated no comments.

Show a summary per file
File Description
test/bvar_mvariable_unittest.cpp Switched static calls from MVariable to MVariableBase and added label‐container template tests
test/CMakeLists.txt Added additional Darwin linker flags for new symbols
src/bvar/variable.cpp Redirected dump_exposed call to MVariableBase
src/bvar/mvariable.h Defined new MVariableBase, moved copy-control there, and templated MVariable on KeyType
src/bvar/mvariable.cpp Updated all definitions to match MVariableBase signatures
src/bvar/multi_dimension.h & _inl.h Templated MultiDimension on both T and KeyType, added hashing/equality functors
src/butil/strings/string_piece.h Added C++17 std::string_view constructor and conversion operator
src/butil/containers/flat_map_inl.h Removed commented-out logging
src/brpc/builtin/prometheus_metrics_service.cpp Changed MVariable::dump_exposed calls to MVariableBase::dump_exposed
Comments suppressed due to low confidence (2)

src/bvar/multi_dimension_inl.h:384

  • Typo in function name 'is_valid_lables_value'; consider renaming it to 'is_valid_labels_value' for clarity and consistency.
    os << "{\"name\" : \"" << this->name() << "\", \"labels\" : [";

src/bvar/multi_dimension_inl.h:386

  • [nitpick] Enhance this error message by adding context and separators, e.g.:
LOG(ERROR) << "Invalid labels count: expected " << this->count_labels()
           << " but got " << labels_value.size();
    for (auto& label : this->_labels) {

@wwbmmm
Copy link
Copy Markdown
Contributor

wwbmmm commented Jul 14, 2025

LGTM

@chenBright chenBright changed the title Support generics for MVariable APIs Support generics for MultiDimension APIs Jul 14, 2025
@wwbmmm wwbmmm merged commit 1ff5f3f into apache:master Jul 23, 2025
15 checks passed
@chenBright chenBright deleted the generics_mbvar branch July 23, 2025 02:17
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.

3 participants