Improve performance of VariableOrigin instances #3164
Merged
Conversation
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
eduar-hte
force-pushed
the
variable-origin
branch
2 times, most recently
from
790fbcc to
b165584
Compare
|
airween
requested changes
Jul 10, 2024
|
Thanks for this PR - the performance improvement is awesome! As I could measure - based on result of own I requested a change: could you remove the CRS2 test case? It's completely unnecessary. |
- The previous approach would create a std::unique_ptr and store it in
a std::list in VariableValue (Origins)
- The new approach now stores Origins in a std::vector and constructs
VariableOrigin elements in-place on insertion.
- Instead of having two heap-allocations for every added VariableOrigin
instance, this performs only one.
- If multiple origins are added, std::vector's growth strategy may even
prevent a heap-allocation. There's a cost on growing the size of the
vector, because a copy of current elements will be necessary.
- Introduced reserveOrigin method to notify that multiple insertions
will be made, so that we can use std::vector's reserve and do a
single allocation (and copy of previous elements), and then just
initialize the new elements in-place.
- Removed unused methods
- Simplified clone & checkout of CRS repository - Removed no longer maintained OWASP Core Ruleset v2
eduar-hte
force-pushed
the
variable-origin
branch
from
b165584 to
eb62cac
Compare
airween
approved these changes
Jul 17, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
what
std::unique_ptrand store it in astd::listinVariableValue'sOrigins.Originsin astd::vectorand constructsVariableOriginelements in-place on insertion.VariableOrigininstance, this performs only one.std::vector's growth strategy may even prevent the heap-allocation on additional insertions.reserveOriginmethod to notify when multiple insertions will be made, so that a call tostd::vector::reservecan be made to perform a single allocation (and copy of previous elements) if necessary, and then just initialize the new elements in-place.why
This change increases performance of the
benchmarktest by about 2.5%.Additionally, the refactored code simplifies code used to initialize
VariableOrigininstances.misc
Included script to download OWASP Core Ruleset v4 for benchmark execution.
This is part of a series of PRs to improve performance of the library (1/n).