Skip to content

built-ins: use strings.Builder in glob.match()#4777

Merged
srenatus merged 1 commit intoopen-policy-agent:mainfrom
charlesdaniels:glob-optimizations
Jun 14, 2022
Merged

built-ins: use strings.Builder in glob.match()#4777
srenatus merged 1 commit intoopen-policy-agent:mainfrom
charlesdaniels:glob-optimizations

Conversation

@charlesdaniels
Copy link
Copy Markdown
Contributor

Use strings.Builder to construct the ID for use in glob.match(). This
offers a slight performance improvement.

This also adds a benchmark to the glob.match() builtin.

Signed-off-by: Charles Daniels charles@styra.com


Before I made this optimization, this is what I got on the new benchmark:

pkg: github.com/open-policy-agent/opa/topdown
BenchmarkGlob/10-8         	   15644	     76413 ns/op
BenchmarkGlob/100-8        	    1770	    656534 ns/op
BenchmarkGlob/1000-8       	     178	   6645135 ns/op
BenchmarkGlob/10000-8      	      16	  69947432 ns/op
PASS
ok  	github.com/open-policy-agent/opa/topdown	9.025s

And after:

pkg: github.com/open-policy-agent/opa/topdown
BenchmarkGlob/10-8         	   19495	     61227 ns/op
BenchmarkGlob/100-8        	    2337	    511508 ns/op
BenchmarkGlob/1000-8       	     231	   5206028 ns/op
BenchmarkGlob/10000-8      	      20	  54776096 ns/op
PASS
ok  	github.com/open-policy-agent/opa/topdown	8.682s

Both tests were run on a 2021 14-inch Macbook Pro with an M1 Pro, 32GB of RAM, and running macOS Monterey 12.4.

I was hoping to achieve a much better speedup here, as glob.match() is a big performance hotspot for one of my use cases. However since I already did the work to try this patch out and analyze the performance, and it is a marginal performance uplift...

srenatus
srenatus previously approved these changes Jun 14, 2022
Copy link
Copy Markdown
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 a lot!

Comment thread topdown/glob.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Would it make a difference to use the pattern length and the number of delimiters to grow the builder to the required size upfront?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I actually did try this, and there was not a measurable performance impact. My hypothesis is that the initial size that gets set when the pattern gets written includes enough extra space for the delimiters, so there is still only one memory allocation.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

So perhaps we'd allocate less...? But I suppose it doesn't matter.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I tried a number of different approaches, including just using bare strings and the + operator directly. Nothing really seemed to move the needle, so I think the real benefit here is just eliminating the constant factor Sprintf() overhead. That and not paying for string quoting by doing a direct typecast.

@charlesdaniels
Copy link
Copy Markdown
Contributor Author

Need a rubber stamp after rebasing and tweaking the benchmark so the CI won't timeout. I don't have write permissions, so feel free to merge once approved.

Use strings.Builder to construct the ID for use in glob.match(). This
offers a slight performance improvement.

This also adds a benchmark to the glob.match() builtin.

Signed-off-by: Charles Daniels <charles@styra.com>
@srenatus srenatus merged commit 7fa701c into open-policy-agent:main Jun 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants