Skip to content

contrib/{internal/httptrace, net/http}: fix memory leak and request closure (cherry-pick #2138)#2139

Merged
knusbaum merged 4 commits into
mainfrom
knusbaum/main-perf-fix
Jul 25, 2023
Merged

contrib/{internal/httptrace, net/http}: fix memory leak and request closure (cherry-pick #2138)#2139
knusbaum merged 4 commits into
mainfrom
knusbaum/main-perf-fix

Conversation

@knusbaum

Copy link
Copy Markdown
Contributor

What does this PR do?

This pull request addresses one major and one minor performance issue.

The major issue is in contrib/net/http, where (*ServeMux).ServeHTTP adds
an element to it's spanOpts slice every time a request is served. This is
both a race, and it grows the slice unbounded resulting in a memory leak,
and a huge amount of CPU time spent copying the slice.

The second, smaller issues is in contrib/internal/httptrace's
HeaderTagsFromRequest. This function created a closure which captured the
request including all of its headers. This may keep the request alive
longer than necessary. Instead we build the list of required headers and
return a function closing over just that list.

This cherry-picks #2138

Reviewer's Checklist

  • Changed code has unit tests for its functionality.
  • If this interacts with the agent in a new way, a system test has been added.

…losure (#2138)

This pull request addresses one major and one minor performance issue.

The major issue is in contrib/net/http, where (*ServeMux).ServeHTTP adds
an element to it's spanOpts slice every time a request is served. This is
both a race, and it grows the slice unbounded resulting in a memory leak,
and a huge amount of CPU time spent copying the slice.

The second, smaller issues is in contrib/internal/httptrace's
HeaderTagsFromRequest. This function created a closure which captured the
request including all of its headers. This may keep the request alive
longer than necessary. Instead we build the list of required headers and
return a function closing over just that list.
@knusbaum
knusbaum requested a review from a team July 20, 2023 21:04
@pr-commenter

pr-commenter Bot commented Jul 20, 2023

Copy link
Copy Markdown

Benchmarks

Benchmark execution time: 2023-07-25 16:33:37

Comparing candidate commit 68d660c in PR branch knusbaum/main-perf-fix with baseline commit 0373ddc in branch main.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 27 metrics, 0 unstable metrics.

@knusbaum
knusbaum merged commit 6703a52 into main Jul 25, 2023
@knusbaum
knusbaum deleted the knusbaum/main-perf-fix branch July 25, 2023 16:40
katiehockman pushed a commit that referenced this pull request Jul 26, 2023
…losure (cherry-pick #2138) (#2139)

This pull request addresses one major and one minor performance issue.

The major issue is in contrib/net/http, where (*ServeMux).ServeHTTP adds
an element to it's spanOpts slice every time a request is served. This is
both a race, and it grows the slice unbounded resulting in a memory leak,
and a huge amount of CPU time spent copying the slice.

The second, smaller issues is in contrib/internal/httptrace's
HeaderTagsFromRequest. This function created a closure which captured the
request including all of its headers. This may keep the request alive
longer than necessary. Instead we build the list of required headers and
return a function closing over just that list.

This cherry-picks #2138
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