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

Reduce regexp memory use in tracestate #4664

Merged
merged 9 commits into from
Oct 31, 2023
Merged

Reduce regexp memory use in tracestate #4664

merged 9 commits into from
Oct 31, 2023

Conversation

johnbley
Copy link
Member

@johnbley johnbley commented Oct 23, 2023

It turns out that the regexp implementation uses a fair amount of memory for counted repetitions of the form (e.g.) [a-z]{0,255}, linear with the count (there is a documented maximum of 1000). Just 3 regexes in tracestate.go add over a megabyte of resident RAM usage for the library, owing to their usage of regex logic to enforce length caps on pieces of the tracestate syntax.
This PR reduces the memory usage of those regexes by about a megabyte by having the regexes dictate acceptable character sequences but not length, and applying length checks after/outside the regexp. The runtime for both approaches should be similar; I've tried to minimize extra iterations over the parsed strings. I've also reduced the number of captures to the minimum required.
To share validation logic I had parseMember call newMember but to keep the exact behavior I had to transmute the returned errors. There may be a more idiomatic way to do this (not a golang expert) or it may be OK to subtly change behavior and I can drop that requirement to simplify the code.

Update: I added benchmarks to show that runtime performance has improved in addition to the resident RAM savings:

Old:

BenchmarkParseTraceState/single_key-16         	  858294	      1422 ns/op	     324 B/op	       6 allocs/op
BenchmarkParseTraceState/tenant_single_key-16  	  864709	      1418 ns/op	     341 B/op	       6 allocs/op
BenchmarkParseTraceState/three_keys-16         	 1000000	      1036 ns/op	     292 B/op	       6 allocs/op
BenchmarkParseTraceState/tenant_three_keys-16  	 1000000	      1071 ns/op	     308 B/op	       6 allocs/op

New:

BenchmarkParseTraceState/single_key-16         	 1000000	      1006 ns/op	     323 B/op	       6 allocs/op
BenchmarkParseTraceState/tenant_single_key-16  	 1000000	      1016 ns/op	     339 B/op	       6 allocs/op
BenchmarkParseTraceState/three_keys-16         	 1456225	       818.4 ns/op	     290 B/op	       6 allocs/op
BenchmarkParseTraceState/tenant_three_keys-16  	 1467804	       818.2 ns/op	     306 B/op	       6 allocs/op

I want to thank the creators of tracestate_test.go for the excellent test suite!

@codecov
Copy link

codecov bot commented Oct 23, 2023

Codecov Report

❗ No coverage uploaded for pull request base (main@b2bb2ad). Click here to learn what that means.
The diff coverage is 100.0%.

Impacted file tree graph

@@          Coverage Diff           @@
##             main   #4664   +/-   ##
======================================
  Coverage        ?   81.7%           
======================================
  Files           ?     225           
  Lines           ?   18056           
  Branches        ?       0           
======================================
  Hits            ?   14762           
  Misses          ?    2996           
  Partials        ?     298           
Files Coverage Δ
trace/tracestate.go 100.0% <100.0%> (ø)

@dmathieu dmathieu added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Oct 23, 2023
@dmathieu
Copy link
Member

Could you run benchmark tests (maybe we need a new benchmark?) to show the actual memory difference?

@johnbley
Copy link
Member Author

Could you run benchmark tests (maybe we need a new benchmark?) to show the actual memory difference?

Sure. I loaded the relevant vars and consts into an otherwise empty program, ran with GODEBUG=allocfreetrace=1 and saw the total allocated bytes (via some cli munging) drop from 1119248 to 226312 for a savings of ~892kb.

@pellared
Copy link
Member

Could you run benchmark tests (maybe we need a new benchmark?) to show the actual memory difference?

Sure. I loaded the relevant vars and consts into an otherwise empty program, ran with GODEBUG=allocfreetrace=1 and saw the total allocated bytes (via some cli munging) drop from 1119248 to 226312 for a savings of ~892kb.

I am pretty sure that @dmathieu is asking for "Go Benchmarks". References:

@johnbley
Copy link
Member Author

I added a simple benchmark and it shows a reduction in runtime from the old code:
Old:

BenchmarkParseTraceState-16       	  844042	      1446 ns/op	     324 B/op	       6 allocs/op

New:

BenchmarkParseTraceState-16       	 1000000	      1030 ns/op	     322 B/op	       6 allocs/op

though of course the reduction in resident RAM usage is not captured in this.

trace/tracestate_test.go Outdated Show resolved Hide resolved
Copy link
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

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

Can you please update the PR description with the benchmark result from "before" and "after" the change?

@pellared
Copy link
Member

If nobody will complain I will merge this PR on Monday.

@pellared pellared merged commit ce7b40a into open-telemetry:main Oct 31, 2023
@johnbley johnbley deleted the reduce_regexp_memory branch October 31, 2023 15:07
@XSAM XSAM added this to the Old Untracked PRs milestone Nov 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants