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

Set Insertion Rework #4999

Merged
merged 4 commits into from Aug 15, 2022

Conversation

philipaconrad
Copy link
Contributor

This PR introduces lazy key slice sorting for the Set datatype, similar to what was done in #4830 (Object Insertion Rework). After this change, Set.Add() will be an O(1) operation, and sorting of the Set type's key slice will be delayed until just-before-use, identically to how lazy key slice sorting is done for the Object type.

This will move the sorting overhead from construction-time for Sets over to eval-time, allowing much more efficient construction and use of enormous (500k+ item) Sets. In small-scale tests, this appears to be a performance-neutral change overall, while dramatically improving performance for the "large set" edge case.

This PR also introduces a 2x new benchmarks for Sets, to keep parity with what we introduced for Objects:

  • BenchmarkSetCreationAndLookup (Identical to BenchmarkObjectCreationAndLookup)
  • BenchmarkSetMarshalJSON (We already benchmark String() operations separately for Sets)

Note: This change is independent of the PR for improving the set union builtin (#4980), but very slightly improves performance there as well (around 5% or smaller improvement), likely from improved efficiency in Set construction.

@philipaconrad philipaconrad self-assigned this Aug 10, 2022
@philipaconrad philipaconrad added this to In Progress in Open Policy Agent via automation Aug 10, 2022
@philipaconrad philipaconrad force-pushed the set-insert-rework branch 2 times, most recently from d0410d4 to 9a28e3b Compare August 12, 2022 17:29
srenatus
srenatus previously approved these changes Aug 15, 2022
Copy link
Contributor

@srenatus srenatus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM one nitpick 😄

ast/term.go Outdated Show resolved Hide resolved
Copy link
Contributor

@srenatus srenatus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@philipaconrad
Copy link
Contributor Author

Will update + squash-merge when green. 👍

Signed-off-by: Philip Conrad <philipaconrad@gmail.com>
Signed-off-by: Philip Conrad <philipaconrad@gmail.com>
Signed-off-by: Philip Conrad <philipaconrad@gmail.com>
Signed-off-by: Philip Conrad <philipaconrad@gmail.com>
@philipaconrad philipaconrad merged commit 9d2b1ad into open-policy-agent:main Aug 15, 2022
Open Policy Agent automation moved this from In Progress to Done Aug 15, 2022
@philipaconrad philipaconrad deleted the set-insert-rework branch September 14, 2022 20:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants