-
Notifications
You must be signed in to change notification settings - Fork 32
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
Optimize common package functions for improved performance #187
Conversation
1. when valid namespace and empty name (strKey ends with '/') then return valid ObjectKey with namespace only 2. when valid name and empty namespace (strKey starts with '/') then return valid ObjectKey with name only Changes were discussed here: #187 (comment)
The tests regarding |
@eguzki, could you please clarify this moment? I talked to DD and the idea was to split "Tests" and "Improvements" to different PR's. I think, despite the changes made in the codebase, we still planning to keep all new unit-tests that are created right now, because they are relevant to the functionality, not just particular implementation. If I am wrong, please, correct me! So, why should we include the tests regarding |
If you do not change the code, then it is right to have a dedicated PR for testing to increase coverage. However, when you open a PR with changes in the code, I also expect some tests in the same PR making sure the new code works as expected. |
Thanks @eguzki 👼 |
…s in UnMarshallLimitNamespace and UnMarshallObjectKey Discussion: #187 (comment) Kudos to Eguzki
As I mentioned here change the tests base of the PR to this branch, so they'll be included in this one. We merge here, then we can review and merge with main |
* test: Add unit-tests for UnMarshallLimitNamespace (#167) * test: Add two FAILING unit-tests for UnMarshallLimitNamespace (#167) The two failing test cases include: - when namespace has no gateway name then return an error - when namespace has no domain name then return an error In these test-cases we leave the separator (either "/" or "#"), which causes the function to return an error when the gateway name or domain name is missing. In the last two test cases: - when namespace only has gateway name (missing 'namespace/') and domain then return an error - when namespace only has namespace name (missing '/gwName') and domain then return an error the separator (/) is missing along with the namespace or gateway names, causing the function to return the correct error message. * refactor: Add comment to UnMarshallLimitNamespace (#167) * test: Add unit-tests for MarshallNamespace (#167) * test: Add unit-tests for UnMarshallObjectKey (#167) * test: Add unit-tests for HostnamesToStrings (#167) * [test] Optimizations, improvements, and unit tests for common/common.go (part 2 of 3) (#182) * test: Add unit-tests for Contains (#167) * refactor: Add comment to Contains (#167) * test: Add unit-tests for Map (#167) * refactor: Add comment to Map (#167) * refactor: Add names to Map test cases (#167) * test: Add unit-tests for SliceCopy (#167) * refactor: Add comment to SliceCopy (#167) * test: Add unit-tests for ReverseSlice (#167) * refactor: Add comment to ReverseSlice (#167) * test: Add unit-tests for MergeMapStringString (#167) * refactor: Add missing import 'reflect' * fix: Ensure Istio gateways created in the tests are ready ('Programmed') by setting ClusterIP service type (#185) * changelog (#184) * test: Add two test-cases for UnMarshallObjectKey (#167) 1. when valid namespace and empty name (strKey ends with '/') then return valid ObjectKey with namespace only 2. when valid name and empty namespace (strKey starts with '/') then return valid ObjectKey with name only Changes were discussed here: #187 (comment) * test: Delete test case for UnMarshallLimitNamespace The test case "when namespace has no gateway name then return an error" has been removed from the test suite for UnMarshallLimitNamespace. This test case was tightly connected to the implementation of UnMarshallObjectKey, making it impossible to properly test the desired behavior in the context of UnMarshallLimitNamespace. * refactor: Add clarity to the test name Describing the '/' separator as a default one we show that it is a default constant value. --------- Co-authored-by: Guilherme Cassolato <guicassolato@gmail.com> Co-authored-by: Eguzki Astiz Lezaun <eastizle@redhat.com>
Rebase with origin/main |
This commit optimizes the UnMarshallObjectKey function by avoiding unnecessary allocations and using a simple string split operation instead of manual string parsing. This results in performance improvements when processing large volumes of Kubernetes objects. Additionally, the function now validates input strings more accurately to prevent errors when working with invalid object keys.
This commit optimizes the UnMarshallLimitNamespace function to improve its runtime performance. The following changes were made: - Used strings.IndexByte instead of strings.Split to search for the index of the '#' delimiter character, reducing the number of iterations and memory allocation overhead. - Extracted the domain directly from the input string instead of creating a temporary slice, reducing the number of string allocations. - Passed only the first half of the input string to UnMarshallObjectKey function, avoiding the creation of unnecessary intermediate strings.
- Pre-allocated output slice using make function to reduce append overhead
…y namespace and name values + refactor the error message
…s in UnMarshallLimitNamespace and UnMarshallObjectKey Discussion: #187 (comment) Kudos to Eguzki
* test: Add unit-tests for UnMarshallLimitNamespace (#167) * test: Add two FAILING unit-tests for UnMarshallLimitNamespace (#167) The two failing test cases include: - when namespace has no gateway name then return an error - when namespace has no domain name then return an error In these test-cases we leave the separator (either "/" or "#"), which causes the function to return an error when the gateway name or domain name is missing. In the last two test cases: - when namespace only has gateway name (missing 'namespace/') and domain then return an error - when namespace only has namespace name (missing '/gwName') and domain then return an error the separator (/) is missing along with the namespace or gateway names, causing the function to return the correct error message. * refactor: Add comment to UnMarshallLimitNamespace (#167) * test: Add unit-tests for MarshallNamespace (#167) * test: Add unit-tests for UnMarshallObjectKey (#167) * test: Add unit-tests for HostnamesToStrings (#167) * [test] Optimizations, improvements, and unit tests for common/common.go (part 2 of 3) (#182) * test: Add unit-tests for Contains (#167) * refactor: Add comment to Contains (#167) * test: Add unit-tests for Map (#167) * refactor: Add comment to Map (#167) * refactor: Add names to Map test cases (#167) * test: Add unit-tests for SliceCopy (#167) * refactor: Add comment to SliceCopy (#167) * test: Add unit-tests for ReverseSlice (#167) * refactor: Add comment to ReverseSlice (#167) * test: Add unit-tests for MergeMapStringString (#167) * refactor: Add missing import 'reflect' * fix: Ensure Istio gateways created in the tests are ready ('Programmed') by setting ClusterIP service type (#185) * changelog (#184) * test: Add two test-cases for UnMarshallObjectKey (#167) 1. when valid namespace and empty name (strKey ends with '/') then return valid ObjectKey with namespace only 2. when valid name and empty namespace (strKey starts with '/') then return valid ObjectKey with name only Changes were discussed here: #187 (comment) * test: Delete test case for UnMarshallLimitNamespace The test case "when namespace has no gateway name then return an error" has been removed from the test suite for UnMarshallLimitNamespace. This test case was tightly connected to the implementation of UnMarshallObjectKey, making it impossible to properly test the desired behavior in the context of UnMarshallLimitNamespace. * refactor: Add clarity to the test name Describing the '/' separator as a default one we show that it is a default constant value. --------- Co-authored-by: Guilherme Cassolato <guicassolato@gmail.com> Co-authored-by: Eguzki Astiz Lezaun <eastizle@redhat.com>
91c57db
to
5255cf7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✌🏼
* optimisation: Improve performance of UnMarshallObjectKey This commit optimizes the UnMarshallObjectKey function by avoiding unnecessary allocations and using a simple string split operation instead of manual string parsing. This results in performance improvements when processing large volumes of Kubernetes objects. Additionally, the function now validates input strings more accurately to prevent errors when working with invalid object keys. * optimisation: Improve performance of UnMarshallLimitNamespace This commit optimizes the UnMarshallLimitNamespace function to improve its runtime performance. The following changes were made: - Used strings.IndexByte instead of strings.Split to search for the index of the '#' delimiter character, reducing the number of iterations and memory allocation overhead. - Extracted the domain directly from the input string instead of creating a temporary slice, reducing the number of string allocations. - Passed only the first half of the input string to UnMarshallObjectKey function, avoiding the creation of unnecessary intermediate strings. * optimisation: improve performance of HostnamesToStrings (#167) - Pre-allocated output slice using make function to reduce append overhead * refactor: Replace errors.New(...) with fmt.Errorf(...) * refactor: Simplify if-condition in UnMarshallObjectKey to handle empty namespace and name values + refactor the error message * fix: Use IndexRune instead of IndexByte to handle non-ascii characters in UnMarshallLimitNamespace and UnMarshallObjectKey Discussion: #187 (comment) Kudos to Eguzki * [test] Unit-tests for common/common.go (part 3 of 3) (#186) * test: Add unit-tests for UnMarshallLimitNamespace (#167) * test: Add two FAILING unit-tests for UnMarshallLimitNamespace (#167) The two failing test cases include: - when namespace has no gateway name then return an error - when namespace has no domain name then return an error In these test-cases we leave the separator (either "/" or "#"), which causes the function to return an error when the gateway name or domain name is missing. In the last two test cases: - when namespace only has gateway name (missing 'namespace/') and domain then return an error - when namespace only has namespace name (missing '/gwName') and domain then return an error the separator (/) is missing along with the namespace or gateway names, causing the function to return the correct error message. * refactor: Add comment to UnMarshallLimitNamespace (#167) * test: Add unit-tests for MarshallNamespace (#167) * test: Add unit-tests for UnMarshallObjectKey (#167) * test: Add unit-tests for HostnamesToStrings (#167) * [test] Optimizations, improvements, and unit tests for common/common.go (part 2 of 3) (#182) * test: Add unit-tests for Contains (#167) * refactor: Add comment to Contains (#167) * test: Add unit-tests for Map (#167) * refactor: Add comment to Map (#167) * refactor: Add names to Map test cases (#167) * test: Add unit-tests for SliceCopy (#167) * refactor: Add comment to SliceCopy (#167) * test: Add unit-tests for ReverseSlice (#167) * refactor: Add comment to ReverseSlice (#167) * test: Add unit-tests for MergeMapStringString (#167) * refactor: Add missing import 'reflect' * fix: Ensure Istio gateways created in the tests are ready ('Programmed') by setting ClusterIP service type (#185) * changelog (#184) * test: Add two test-cases for UnMarshallObjectKey (#167) 1. when valid namespace and empty name (strKey ends with '/') then return valid ObjectKey with namespace only 2. when valid name and empty namespace (strKey starts with '/') then return valid ObjectKey with name only Changes were discussed here: #187 (comment) * test: Delete test case for UnMarshallLimitNamespace The test case "when namespace has no gateway name then return an error" has been removed from the test suite for UnMarshallLimitNamespace. This test case was tightly connected to the implementation of UnMarshallObjectKey, making it impossible to properly test the desired behavior in the context of UnMarshallLimitNamespace. * refactor: Add clarity to the test name Describing the '/' separator as a default one we show that it is a default constant value. --------- Co-authored-by: Guilherme Cassolato <guicassolato@gmail.com> Co-authored-by: Eguzki Astiz Lezaun <eastizle@redhat.com> --------- Co-authored-by: Guilherme Cassolato <guicassolato@gmail.com> Co-authored-by: Eguzki Astiz Lezaun <eastizle@redhat.com>
* optimisation: Improve performance of UnMarshallObjectKey This commit optimizes the UnMarshallObjectKey function by avoiding unnecessary allocations and using a simple string split operation instead of manual string parsing. This results in performance improvements when processing large volumes of Kubernetes objects. Additionally, the function now validates input strings more accurately to prevent errors when working with invalid object keys. * optimisation: Improve performance of UnMarshallLimitNamespace This commit optimizes the UnMarshallLimitNamespace function to improve its runtime performance. The following changes were made: - Used strings.IndexByte instead of strings.Split to search for the index of the '#' delimiter character, reducing the number of iterations and memory allocation overhead. - Extracted the domain directly from the input string instead of creating a temporary slice, reducing the number of string allocations. - Passed only the first half of the input string to UnMarshallObjectKey function, avoiding the creation of unnecessary intermediate strings. * optimisation: improve performance of HostnamesToStrings (#167) - Pre-allocated output slice using make function to reduce append overhead * refactor: Replace errors.New(...) with fmt.Errorf(...) * refactor: Simplify if-condition in UnMarshallObjectKey to handle empty namespace and name values + refactor the error message * fix: Use IndexRune instead of IndexByte to handle non-ascii characters in UnMarshallLimitNamespace and UnMarshallObjectKey Discussion: #187 (comment) Kudos to Eguzki * [test] Unit-tests for common/common.go (part 3 of 3) (#186) * test: Add unit-tests for UnMarshallLimitNamespace (#167) * test: Add two FAILING unit-tests for UnMarshallLimitNamespace (#167) The two failing test cases include: - when namespace has no gateway name then return an error - when namespace has no domain name then return an error In these test-cases we leave the separator (either "/" or "#"), which causes the function to return an error when the gateway name or domain name is missing. In the last two test cases: - when namespace only has gateway name (missing 'namespace/') and domain then return an error - when namespace only has namespace name (missing '/gwName') and domain then return an error the separator (/) is missing along with the namespace or gateway names, causing the function to return the correct error message. * refactor: Add comment to UnMarshallLimitNamespace (#167) * test: Add unit-tests for MarshallNamespace (#167) * test: Add unit-tests for UnMarshallObjectKey (#167) * test: Add unit-tests for HostnamesToStrings (#167) * [test] Optimizations, improvements, and unit tests for common/common.go (part 2 of 3) (#182) * test: Add unit-tests for Contains (#167) * refactor: Add comment to Contains (#167) * test: Add unit-tests for Map (#167) * refactor: Add comment to Map (#167) * refactor: Add names to Map test cases (#167) * test: Add unit-tests for SliceCopy (#167) * refactor: Add comment to SliceCopy (#167) * test: Add unit-tests for ReverseSlice (#167) * refactor: Add comment to ReverseSlice (#167) * test: Add unit-tests for MergeMapStringString (#167) * refactor: Add missing import 'reflect' * fix: Ensure Istio gateways created in the tests are ready ('Programmed') by setting ClusterIP service type (#185) * changelog (#184) * test: Add two test-cases for UnMarshallObjectKey (#167) 1. when valid namespace and empty name (strKey ends with '/') then return valid ObjectKey with namespace only 2. when valid name and empty namespace (strKey starts with '/') then return valid ObjectKey with name only Changes were discussed here: #187 (comment) * test: Delete test case for UnMarshallLimitNamespace The test case "when namespace has no gateway name then return an error" has been removed from the test suite for UnMarshallLimitNamespace. This test case was tightly connected to the implementation of UnMarshallObjectKey, making it impossible to properly test the desired behavior in the context of UnMarshallLimitNamespace. * refactor: Add clarity to the test name Describing the '/' separator as a default one we show that it is a default constant value. --------- Co-authored-by: Guilherme Cassolato <guicassolato@gmail.com> Co-authored-by: Eguzki Astiz Lezaun <eastizle@redhat.com> --------- Co-authored-by: Guilherme Cassolato <guicassolato@gmail.com> Co-authored-by: Eguzki Astiz Lezaun <eastizle@redhat.com>
* optimisation: Improve performance of UnMarshallObjectKey This commit optimizes the UnMarshallObjectKey function by avoiding unnecessary allocations and using a simple string split operation instead of manual string parsing. This results in performance improvements when processing large volumes of Kubernetes objects. Additionally, the function now validates input strings more accurately to prevent errors when working with invalid object keys. * optimisation: Improve performance of UnMarshallLimitNamespace This commit optimizes the UnMarshallLimitNamespace function to improve its runtime performance. The following changes were made: - Used strings.IndexByte instead of strings.Split to search for the index of the '#' delimiter character, reducing the number of iterations and memory allocation overhead. - Extracted the domain directly from the input string instead of creating a temporary slice, reducing the number of string allocations. - Passed only the first half of the input string to UnMarshallObjectKey function, avoiding the creation of unnecessary intermediate strings. * optimisation: improve performance of HostnamesToStrings (#167) - Pre-allocated output slice using make function to reduce append overhead * refactor: Replace errors.New(...) with fmt.Errorf(...) * refactor: Simplify if-condition in UnMarshallObjectKey to handle empty namespace and name values + refactor the error message * fix: Use IndexRune instead of IndexByte to handle non-ascii characters in UnMarshallLimitNamespace and UnMarshallObjectKey Discussion: #187 (comment) Kudos to Eguzki * [test] Unit-tests for common/common.go (part 3 of 3) (#186) * test: Add unit-tests for UnMarshallLimitNamespace (#167) * test: Add two FAILING unit-tests for UnMarshallLimitNamespace (#167) The two failing test cases include: - when namespace has no gateway name then return an error - when namespace has no domain name then return an error In these test-cases we leave the separator (either "/" or "#"), which causes the function to return an error when the gateway name or domain name is missing. In the last two test cases: - when namespace only has gateway name (missing 'namespace/') and domain then return an error - when namespace only has namespace name (missing '/gwName') and domain then return an error the separator (/) is missing along with the namespace or gateway names, causing the function to return the correct error message. * refactor: Add comment to UnMarshallLimitNamespace (#167) * test: Add unit-tests for MarshallNamespace (#167) * test: Add unit-tests for UnMarshallObjectKey (#167) * test: Add unit-tests for HostnamesToStrings (#167) * [test] Optimizations, improvements, and unit tests for common/common.go (part 2 of 3) (#182) * test: Add unit-tests for Contains (#167) * refactor: Add comment to Contains (#167) * test: Add unit-tests for Map (#167) * refactor: Add comment to Map (#167) * refactor: Add names to Map test cases (#167) * test: Add unit-tests for SliceCopy (#167) * refactor: Add comment to SliceCopy (#167) * test: Add unit-tests for ReverseSlice (#167) * refactor: Add comment to ReverseSlice (#167) * test: Add unit-tests for MergeMapStringString (#167) * refactor: Add missing import 'reflect' * fix: Ensure Istio gateways created in the tests are ready ('Programmed') by setting ClusterIP service type (#185) * changelog (#184) * test: Add two test-cases for UnMarshallObjectKey (#167) 1. when valid namespace and empty name (strKey ends with '/') then return valid ObjectKey with namespace only 2. when valid name and empty namespace (strKey starts with '/') then return valid ObjectKey with name only Changes were discussed here: #187 (comment) * test: Delete test case for UnMarshallLimitNamespace The test case "when namespace has no gateway name then return an error" has been removed from the test suite for UnMarshallLimitNamespace. This test case was tightly connected to the implementation of UnMarshallObjectKey, making it impossible to properly test the desired behavior in the context of UnMarshallLimitNamespace. * refactor: Add clarity to the test name Describing the '/' separator as a default one we show that it is a default constant value. --------- Co-authored-by: Guilherme Cassolato <guicassolato@gmail.com> Co-authored-by: Eguzki Astiz Lezaun <eastizle@redhat.com> --------- Co-authored-by: Guilherme Cassolato <guicassolato@gmail.com> Co-authored-by: Eguzki Astiz Lezaun <eastizle@redhat.com>
This pull request contains several optimizations to improve the performance of the functions in the
common package
as a part of working on #167. The commits included in this pull request are:Optimized the
UnMarshallObjectKey
function to reduce unnecessary allocations and improve validity checks for input strings. This resulted in an average performance improvement of 17.3% in average across 3 benchmarks with input sizes of 1000, 100, and 10. Additionally, this change partly fixes one of two failing test-case in Add two FAILING unit-tests for UnMarshallLimitNamespace (https://github.com/Kuadrant/kuadrant-operator/issues/167), the "when namespace has no gateway name then return an error" specifically.Optimized the
UnMarshallLimitNamespace
function to reduce string allocations and use a more efficient search method for delimiter characters. This resulted in an average performance improvement of 52% in average across 3 benchmarks with input sizes of 1000, 100, and 10.Optimized the
HostnamesToStrings
function by pre-allocating the output slice and reducing the number of string allocations required. This resulted in an average performance improvement of 69% in average across 3 benchmarks with input sizes of 1000, 100, and 10.These optimizations will help to improve the performance of the common package when working with large volumes of Kubernetes objects.