Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[core] Reduce allocation / deallocations necessary for Registration. #1744

Merged
merged 10 commits into from
Sep 26, 2024

Conversation

KerstinKeller
Copy link
Contributor

Description

In bigger, idle eCAL System there is quite a large "Grundlast" in the system, due to how Registrations are processed.
This PR tries to solve that, by providing a Util::CExpandingVector which only deallocates members upon destruction, and avoiding any unnessesary instatiations of eCAL::Sample instances.

@KerstinKeller KerstinKeller added the cherry-pick-to-NONE Don't cherry-pick these changes label Sep 25, 2024
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

ecal/core/src/util/expanding_vector.h Outdated Show resolved Hide resolved
@@ -0,0 +1,180 @@
#include <gtest/gtest.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: 'gtest/gtest.h' file not found [clang-diagnostic-error]

#include <gtest/gtest.h>
         ^

};

// Test push_back and size functionality
TEST_F(CExpandingVectorTest, PushBackAndSize) {
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: all parameters should be named in a function [readability-named-parameter]

Suggested change
TEST_F(CExpandingVectorTest, PushBackAndSize) {
TEST_F(CExpandingVectorTest /*unused*/, PushBackAndSize /*unused*/) {

}

// Test clear functionality
TEST_F(CExpandingVectorTest, ClearElements) {
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: all parameters should be named in a function [readability-named-parameter]

Suggested change
TEST_F(CExpandingVectorTest, ClearElements) {
TEST_F(CExpandingVectorTest /*unused*/, ClearElements /*unused*/) {

}

// Test resize functionality
TEST_F(CExpandingVectorTest, ResizeVector) {
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: all parameters should be named in a function [readability-named-parameter]

Suggested change
TEST_F(CExpandingVectorTest, ResizeVector) {
TEST_F(CExpandingVectorTest /*unused*/, ResizeVector /*unused*/) {

}

// Test empty() function
TEST_F(CExpandingVectorTest, EmptyFunction) {
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: all parameters should be named in a function [readability-named-parameter]

Suggested change
TEST_F(CExpandingVectorTest, EmptyFunction) {
TEST_F(CExpandingVectorTest /*unused*/, EmptyFunction /*unused*/) {

}

// Test capacity and full_size functions
TEST_F(CExpandingVectorTest, FullSize) {
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: all parameters should be named in a function [readability-named-parameter]

Suggested change
TEST_F(CExpandingVectorTest, FullSize) {
TEST_F(CExpandingVectorTest /*unused*/, FullSize /*unused*/) {

}


TEST_F(CExpandingVectorTest, Front) {
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: all parameters should be named in a function [readability-named-parameter]

Suggested change
TEST_F(CExpandingVectorTest, Front) {
TEST_F(CExpandingVectorTest /*unused*/, Front /*unused*/) {

}

// Test back function
TEST_F(CExpandingVectorTest, Back) {
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: all parameters should be named in a function [readability-named-parameter]

Suggested change
TEST_F(CExpandingVectorTest, Back) {
TEST_F(CExpandingVectorTest /*unused*/, Back /*unused*/) {

}

// Test front and back with an empty vector (should throw an exception)
TEST_F(CExpandingVectorTest, FrontAndBackEmptyVector) {
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: all parameters should be named in a function [readability-named-parameter]

Suggested change
TEST_F(CExpandingVectorTest, FrontAndBackEmptyVector) {
TEST_F(CExpandingVectorTest /*unused*/, FrontAndBackEmptyVector /*unused*/) {

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

using core_cpp_util_expanding_vector = CExpandingVectorTest;

// Test push_back and size functionality
TEST_F(core_cpp_util_expanding_vector, PushBackAndSize) {
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: all parameters should be named in a function [readability-named-parameter]

Suggested change
TEST_F(core_cpp_util_expanding_vector, PushBackAndSize) {
TEST_F(core_cpp_util_expanding_vector /*unused*/, PushBackAndSize /*unused*/) {

}

// Test clear functionality
TEST_F(core_cpp_util_expanding_vector, ClearElements) {
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: all parameters should be named in a function [readability-named-parameter]

Suggested change
TEST_F(core_cpp_util_expanding_vector, ClearElements) {
TEST_F(core_cpp_util_expanding_vector /*unused*/, ClearElements /*unused*/) {

}

// Test resize functionality
TEST_F(core_cpp_util_expanding_vector, ResizeVector) {
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: all parameters should be named in a function [readability-named-parameter]

Suggested change
TEST_F(core_cpp_util_expanding_vector, ResizeVector) {
TEST_F(core_cpp_util_expanding_vector /*unused*/, ResizeVector /*unused*/) {

}

// Test operator[] without bounds checking
TEST_F(core_cpp_util_expanding_vector, OperatorAccess) {
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: all parameters should be named in a function [readability-named-parameter]

Suggested change
TEST_F(core_cpp_util_expanding_vector, OperatorAccess) {
TEST_F(core_cpp_util_expanding_vector /*unused*/, OperatorAccess /*unused*/) {

}

// Test at() function with bounds checking
TEST_F(core_cpp_util_expanding_vector, AtFunction) {
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: all parameters should be named in a function [readability-named-parameter]

Suggested change
TEST_F(core_cpp_util_expanding_vector, AtFunction) {
TEST_F(core_cpp_util_expanding_vector /*unused*/, AtFunction /*unused*/) {

}

// Test empty() function
TEST_F(core_cpp_util_expanding_vector, EmptyFunction) {
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: all parameters should be named in a function [readability-named-parameter]

Suggested change
TEST_F(core_cpp_util_expanding_vector, EmptyFunction) {
TEST_F(core_cpp_util_expanding_vector /*unused*/, EmptyFunction /*unused*/) {

}

// Test capacity and full_size functions
TEST_F(core_cpp_util_expanding_vector, FullSize) {
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: all parameters should be named in a function [readability-named-parameter]

Suggested change
TEST_F(core_cpp_util_expanding_vector, FullSize) {
TEST_F(core_cpp_util_expanding_vector /*unused*/, FullSize /*unused*/) {

}


TEST_F(core_cpp_util_expanding_vector, Front) {
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: all parameters should be named in a function [readability-named-parameter]

Suggested change
TEST_F(core_cpp_util_expanding_vector, Front) {
TEST_F(core_cpp_util_expanding_vector /*unused*/, Front /*unused*/) {

}

// Test back function
TEST_F(core_cpp_util_expanding_vector, Back) {
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: all parameters should be named in a function [readability-named-parameter]

Suggested change
TEST_F(core_cpp_util_expanding_vector, Back) {
TEST_F(core_cpp_util_expanding_vector /*unused*/, Back /*unused*/) {

}

// Test front and back with an empty vector (should throw an exception)
TEST_F(core_cpp_util_expanding_vector, FrontAndBackEmptyVector) {
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: all parameters should be named in a function [readability-named-parameter]

Suggested change
TEST_F(core_cpp_util_expanding_vector, FrontAndBackEmptyVector) {
TEST_F(core_cpp_util_expanding_vector /*unused*/, FrontAndBackEmptyVector /*unused*/) {

Copy link
Contributor

@hannemn hannemn left a comment

Choose a reason for hiding this comment

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

Looks good to me as reviewed together in the meetingv yesterday.

@KerstinKeller KerstinKeller merged commit 6df2a8e into master Sep 26, 2024
20 checks passed
@KerstinKeller KerstinKeller deleted the feature/performance-improvements-2 branch September 26, 2024 12:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick-to-NONE Don't cherry-pick these changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants