Implementation of radix trie #153
Merged
Conversation
This file contains 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
|
Note These results are affected by shared workloads on GitHub runners. Use the results only to detect possible regressions, but always rerun on more stable machine before making any conclusions! Benchmark results (pull-request, a22beae)Benchmark results (master, 007bb6a) |
gavlyukovskiy
force-pushed
the
radix-trie
branch
2 times, most recently
from
50895c5 to
6d0b359
Compare
|
gavlyukovskiy
force-pushed
the
radix-trie
branch
3 times, most recently
from
b4cce84 to
0702a2d
Compare
|
Was testing on of the benchmarks locally, the key matching actually got faster by quite a bit: Before:After:so it looks like the reason we don't see much improvement on the existing benchmarks is because we have |
gavlyukovskiy
force-pushed
the
radix-trie
branch
3 times, most recently
from
5f4bc37 to
ff235e4
Compare
|
donavdey
approved these changes
Oct 22, 2024
There was a problem hiding this comment.
I guess there is a room to optimize JsonMakser instance creation time if we set out minds to do so, but otherwise the implementation seems pretty nice.
As discussed before, I will do something about the JSONPath stack so that we don't have to have a separate StatefulRadixTrieNode implementation
Breus
requested changes
Oct 28, 2024
src/test/java/dev/blaauwendraad/masker/json/InstanceCreationMemoryUsageTest.java
Show resolved
Hide resolved
|
@gavlyukovskiy ignore the comments and last commit for now, I am applying the comments myself and want to play around with it a bit more adding lower level unit tests and some JavaDoc |
gavlyukovskiy
force-pushed
the
radix-trie
branch
from
0aa5bdb to
930468d
Compare
gavlyukovskiy
force-pushed
the
radix-trie
branch
from
930468d to
db1f605
Compare
Breus
reviewed
Dec 18, 2024
Breus
reviewed
Dec 18, 2024
src/main/java/dev/blaauwendraad/masker/json/config/JsonMaskingConfig.java
Outdated
Show resolved
Hide resolved
Breus
reviewed
Dec 18, 2024
src/main/java/dev/blaauwendraad/masker/json/config/JsonMaskingConfig.java
Outdated
Show resolved
Hide resolved
Breus
reviewed
Dec 18, 2024
Breus
reviewed
Dec 18, 2024
donavdey
reviewed
Dec 20, 2024
Breus
reviewed
Dec 27, 2024
Closed
|
Breus
approved these changes
Jan 26, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Radix tree
This PR implements radix trie that has reduced retained memory footprint. I hoped to also see some improvements in benchmarks that affect processing, but I don't see anything conclusive 🤷 Perhaps that is because of branching in the matching methods that is now a bit more complicated.