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

Unlimit whitelist depth #61

Merged
merged 6 commits into from Feb 1, 2018

Conversation

Projects
None yet
4 participants
@dhontecillas
Copy link
Member

commented Nov 19, 2017

I found that current whitelist only allows two level depth, but I like to be able to select more in depth fields.

So I tried this change, that while using up to two level depth whitelist, it keeps running at similar speed / performance that was with previous code, but allows to select any deeper level fields.

(Removed implementation details comment part that no longer apply)

@coveralls

This comment has been minimized.

Copy link

commented Nov 19, 2017

Coverage Status

Coverage decreased (-0.2%) to 95.585% when pulling 8be5ed0 on dhontecillas:unlimit_whitelist_depth into e275560 on devopsfaith:master.

@coveralls

This comment has been minimized.

Copy link

commented Nov 19, 2017

Coverage Status

Coverage decreased (-0.4%) to 95.399% when pulling 4eae11b on dhontecillas:unlimit_whitelist_depth into e275560 on devopsfaith:master.

@alombarte alombarte requested a review from kpacha Jan 4, 2018

@alombarte

This comment has been minimized.

Copy link
Member

commented Jan 7, 2018

Thanks @dhontecillas for the pull request.

We will deepen into this functionality and make it a candidate for the 0.5 version (0.4 is closed and about to be released 🎉 ). It is an interesting improvement in functionality.

@dhontecillas

This comment has been minimized.

Copy link
Member Author

commented Jan 9, 2018

Ok, no problem. I think I can improve the solution a little bit.
I will do a couple of tests to check if I can make it better.

@coveralls

This comment has been minimized.

Copy link

commented Jan 15, 2018

Coverage Status

Coverage decreased (-0.6%) to 97.383% when pulling b16062c on dhontecillas:unlimit_whitelist_depth into b46eca7 on devopsfaith:master.

@dhontecillas

This comment has been minimized.

Copy link
Member Author

commented Jan 15, 2018

I added benchmark tests, and changed the implementation to an in-place recursive deletion to avoid allocs (should have thought it in the first place, also improves speed).

Results for old implementation:

BenchmarkEntityFormatter_deepWhitelistingFilter/numTargets:0,depth:1,extraFields:2,extraSiblings:0-8            100000000               65.3 ns/op            48 B/op          1 allocs/op
BenchmarkEntityFormatter_deepWhitelistingFilter/numTargets:0,depth:3,extraFields:6,extraSiblings:0-8            100000000               55.6 ns/op            48 B/op          1 allocs/op
BenchmarkEntityFormatter_deepWhitelistingFilter/numTargets:0,depth:7,extraFields:14,extraSiblings:0-8           100000000               56.0 ns/op            48 B/op          1 allocs/op
BenchmarkEntityFormatter_deepWhitelistingFilter/numTargets:1,depth:1,extraFields:3,extraSiblings:1-8            10000000               415 ns/op             720 B/op          5 allocs/op
BenchmarkEntityFormatter_deepWhitelistingFilter/numTargets:1,depth:3,extraFields:7,extraSiblings:1-8             5000000               777 ns/op            1392 B/op          9 allocs/op
BenchmarkEntityFormatter_deepWhitelistingFilter/numTargets:1,depth:7,extraFields:15,extraSiblings:1-8            3000000              1470 ns/op            2736 B/op         17 allocs/op
BenchmarkEntityFormatter_deepWhitelistingFilter/numTargets:2,depth:1,extraFields:4,extraSiblings:2-8            10000000               701 ns/op            1056 B/op          7 allocs/op
BenchmarkEntityFormatter_deepWhitelistingFilter/numTargets:2,depth:3,extraFields:8,extraSiblings:2-8             2000000              1753 ns/op            2400 B/op         15 allocs/op
BenchmarkEntityFormatter_deepWhitelistingFilter/numTargets:2,depth:7,extraFields:16,extraSiblings:2-8            1000000              3948 ns/op            5088 B/op         31 allocs/op
BenchmarkEntityFormatter_deepWhitelistingFilter/numTargets:5,depth:1,extraFields:7,extraSiblings:5-8             2000000              1955 ns/op            2064 B/op         13 allocs/op
BenchmarkEntityFormatter_deepWhitelistingFilter/numTargets:5,depth:3,extraFields:11,extraSiblings:5-8            1000000              4522 ns/op            5424 B/op         33 allocs/op
BenchmarkEntityFormatter_deepWhitelistingFilter/numTargets:5,depth:7,extraFields:19,extraSiblings:5-8             500000              7638 ns/op           12144 B/op         73 allocs/op
BenchmarkEntityFormatter_deepWhitelistingFilter/numTargets:10,depth:1,extraFields:12,extraSiblings:10-8          2000000              3075 ns/op            4038 B/op         23 allocs/op
BenchmarkEntityFormatter_deepWhitelistingFilter/numTargets:10,depth:3,extraFields:16,extraSiblings:10-8          1000000              6743 ns/op           10758 B/op         63 allocs/op
BenchmarkEntityFormatter_deepWhitelistingFilter/numTargets:10,depth:7,extraFields:24,extraSiblings:10-8           300000             13795 ns/op           24198 B/op        143 allocs/op

new implementation gives me these numbers in my PC:

BenchmarkEntityFormatter_deepWhitelistingFilter/numTargets:0,depth:1,extraFields:2,extraSiblings:0-8            100000000               56.6 ns/op            48 B/op          1 allocs/op
BenchmarkEntityFormatter_deepWhitelistingFilter/numTargets:0,depth:3,extraFields:6,extraSiblings:0-8            100000000               55.9 ns/op            48 B/op          1 allocs/op
BenchmarkEntityFormatter_deepWhitelistingFilter/numTargets:0,depth:7,extraFields:14,extraSiblings:0-8           100000000               56.8 ns/op            48 B/op          1 allocs/op
BenchmarkEntityFormatter_deepWhitelistingFilter/numTargets:1,depth:1,extraFields:3,extraSiblings:1-8            30000000               150 ns/op              48 B/op          1 allocs/op
BenchmarkEntityFormatter_deepWhitelistingFilter/numTargets:1,depth:3,extraFields:7,extraSiblings:1-8            20000000               267 ns/op              48 B/op          1 allocs/op
BenchmarkEntityFormatter_deepWhitelistingFilter/numTargets:1,depth:7,extraFields:15,extraSiblings:1-8           10000000               485 ns/op              48 B/op          1 allocs/op
BenchmarkEntityFormatter_deepWhitelistingFilter/numTargets:2,depth:1,extraFields:4,extraSiblings:2-8            20000000               215 ns/op              48 B/op          1 allocs/op
BenchmarkEntityFormatter_deepWhitelistingFilter/numTargets:2,depth:3,extraFields:8,extraSiblings:2-8            10000000               436 ns/op              48 B/op          1 allocs/op
BenchmarkEntityFormatter_deepWhitelistingFilter/numTargets:2,depth:7,extraFields:16,extraSiblings:2-8            5000000               823 ns/op              48 B/op          1 allocs/op
BenchmarkEntityFormatter_deepWhitelistingFilter/numTargets:5,depth:1,extraFields:7,extraSiblings:5-8            10000000               466 ns/op              48 B/op          1 allocs/op
BenchmarkEntityFormatter_deepWhitelistingFilter/numTargets:5,depth:3,extraFields:11,extraSiblings:5-8            5000000               988 ns/op              48 B/op          1 allocs/op
BenchmarkEntityFormatter_deepWhitelistingFilter/numTargets:5,depth:7,extraFields:19,extraSiblings:5-8            2000000              1932 ns/op              48 B/op          1 allocs/op
BenchmarkEntityFormatter_deepWhitelistingFilter/numTargets:10,depth:1,extraFields:12,extraSiblings:10-8          5000000              1090 ns/op              48 B/op          1 allocs/op
BenchmarkEntityFormatter_deepWhitelistingFilter/numTargets:10,depth:3,extraFields:16,extraSiblings:10-8          2000000              2743 ns/op              48 B/op          1 allocs/op
BenchmarkEntityFormatter_deepWhitelistingFilter/numTargets:10,depth:7,extraFields:24,extraSiblings:10-8          1000000              5707 ns/op              48 B/op          1 allocs/op

@dhontecillas dhontecillas force-pushed the dhontecillas:unlimit_whitelist_depth branch from e7dd1eb to b16062c Jan 30, 2018

@dhontecillas

This comment has been minimized.

Copy link
Member Author

commented Jan 30, 2018

I've rebased the PR to master. It should be ready for review.

@kpacha

This comment has been minimized.

Copy link
Member

commented Jan 30, 2018

great, thx!

@kpacha kpacha merged commit d86c311 into devopsfaith:master Feb 1, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.6%) to 97.383%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.