string-matcher: introduce OO string-matcher prior to refactor#37324
Closed
adisuissa wants to merge 10 commits intoenvoyproxy:mainfrom
Closed
string-matcher: introduce OO string-matcher prior to refactor#37324adisuissa wants to merge 10 commits intoenvoyproxy:mainfrom
adisuissa wants to merge 10 commits intoenvoyproxy:mainfrom
Conversation
Signed-off-by: Adi Suissa-Peleg <adip@google.com>
Contributor
Author
|
In my draft branch (that includes this one and the future PRs), I've executed the router_config_impl_speed_test, using the following: and got the following results - showing only the mean of each 10 runs, and the second column is "diff%" (negative number implies that the new version is better - less time, positive number implies that the old/HEAD version is better):
Here are the full results in the files: Looking a bit closer a some of these differences, it seems that there is a wide variation, which makes me think that the above diff% are not highly reliable. I've also executed the |
Contributor
Author
|
/assign @ggreenway |
Signed-off-by: Adi Suissa-Peleg <adip@google.com>
Signed-off-by: Adi Suissa-Peleg <adip@google.com>
Signed-off-by: Adi Suissa-Peleg <adip@google.com>
Signed-off-by: Adi Suissa-Peleg <adip@google.com>
Signed-off-by: Adi Suissa-Peleg <adip@google.com>
…late_method_part1 Signed-off-by: Adi Suissa-Peleg <adip@google.com>
Contributor
Author
|
@ggreenway PTAL |
Member
|
There are a few rows in the performance table that are large degradations. Do you know why those got so much worse? Do we change from values to pointers in some cases? That could conceivably hurt performance, especially if there's cache pressure (large working set). /wait-any |
|
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
github-actions
Bot
added
the
stale
Contributor
Author
|
This PR will probably be superseded by #37782. Keeping it open until an agreement is reached. |
github-actions
Bot
removed
the
stale
|
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
github-actions
Bot
added
the
stale
yanavlasov
pushed a commit
that referenced
this pull request
Feb 7, 2025
This is similar to #37324 but doesn't use virtual functions (uses std::variant instead). These are the goals for this new StringMatcherImpl class: 1. improve encapsulation and use polymorphism that will make the code a bit more readable. 2. reduce the amount of memory used by the StringMatcher (e.g., an object of type ExactStringMatcher doesn't need to also hold a pointer to a regex object). 3. reduce the amount of memory that is essentially repeated (removing the kept protobuf matcher object that holds redundant information). 4. replace the uses of the [matcher()](https://github.com/envoyproxy/envoy/blob/9aad3320cf58a19cb4bbbbbaa357dbedf8715900/source/common/common/matchers.h#L157) that are needed to find the contents of the string-matcher by introducing the stringRepresentation() method. A [simple test](#37782 (comment)) showed that for 1000 perfix (non-regex) matchers, the new approach reduced the amount of memory from 570368 to 530176 bytes (~40KiB). For a case of 1000 regex-matchers with a varying length of prefix (1-1001), the new approach reduced the amount of memory from 15671616 to 15021632 bytes (~635KiB). Compared to #37324, this version enjoys the inlining compiler optimizations that can be made when virtual tables aren't used. The downside is the use of std::variant, which may require a bit more memory, and results in a more coupled code for the string matchers, but IMHO it improves upon the current implementation. Risk Level: low - not being used at the moment (but future PRs will). Testing: duplicated the tests needed Docs Changes: N/A Release Notes: N/A Platform Specific Features: N/A --------- Signed-off-by: Adi Suissa-Peleg <adip@google.com>
Member
|
@adisuissa should this be closed now in favor of the other one that just merged? |
github-actions
Bot
removed
the
stale
Contributor
Author
Yes, this can be closed now as #37782 supersedes it. |
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
2 participants
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.
Commit Message: string-matcher: introduce OO string-matcher prior to refactor
Additional Description:
This is a first part in a refactor that replaces the current StringMatcherImpl class.
These are the goals for this new class:
stringRepresentation()method.The followup PR(s) will change the many locations that use
StringMatcherImplto use the new matchers.Risk Level: low - not being used at the moment (but future PRs will).
Testing: duplicated the tests needed
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A