Skip to content
This repository has been archived by the owner on Jul 31, 2023. It is now read-only.

Fix evictedQueue memory leak #1203

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix evictedQueue memory leak #1203

wants to merge 1 commit into from

Conversation

cakiecakie
Copy link

No description provided.

@cakiecakie cakiecakie requested review from rakyll, rghetia and a team as code owners March 24, 2020 08:19
@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@cakiecakie cakiecakie force-pushed the feature/fix_evicted_queue_memory_leak branch from 475672b to 1c72087 Compare March 24, 2020 08:30
@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

rghetia
rghetia reviewed Mar 29, 2020
View reviewed changes
trace/evictedqueue.go Outdated
eq.queue = eq.queue[1:]
newQueue := make([]interface{}, eq.capacity-1, eq.capacity)
copy(newQueue, eq.queue[1:])
eq.queue = newQueue
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, there will be a memory leak but only until the span is alive. In most (if not all) cases spans are short-lived. So there there isn't much concern here. On the contrary this fix will increase the allocation in the event of reaching the limit. Here is the PR that ran on master (without memleak fix) and on top of your PR (memleak fix)

Benchmark

The fix has performance impact and more allocation.

MemLeak Fix

trace git:(evicted_queue) ✗ go test -benchtime=50ms -bench=BenchmarkDropCount.* ./

goos: darwin
goarch: amd64
pkg: go.opencensus.io/trace
BenchmarkDropCount/cap-5-add-10-8         	   71155	       855 ns/op     800 B/op      10 allocs/op
BenchmarkDropCount/cap-10-add-20-8        	   29656	      2099 ns/op    3199 B/op      20 allocs/op
BenchmarkDropCount/cap-30-add-40-8        	    7581	      7355 ns/op   19198 B/op      39 allocs/op
BenchmarkDropCount/cap-40-add-50-8        	    6542	     10930 ns/op   31996 B/op      49 allocs/op
PASS
ok  	go.opencensus.io/trace	0.601s

Without MemLeak Fix

 trace git:(evicted_queue) ✗ go test -benchtime=50ms -bench=BenchmarkDropCount.* ./

goos: darwin
goarch: amd64
pkg: go.opencensus.io/trace
BenchmarkDropCount/cap-5-add-10-8         	  212794	       277 ns/op     320 B/op       2 allocs/op
BenchmarkDropCount/cap-10-add-20-8        	  147648	       397 ns/op     640 B/op       2 allocs/op
BenchmarkDropCount/cap-30-add-40-8        	  119190	       568 ns/op    1170 B/op       1 allocs/op
BenchmarkDropCount/cap-40-add-50-8        	   93718	       673 ns/op    1560 B/op       1 allocs/op
PASS
ok  	go.opencensus.io/trace	0.906s

I also did the profiling.

Memory Profile

There are three cases

  • Not saving the queue after the iteration (similar to span ending)
    • No inuse_space in both cases but alloc_space is significantly higher with the fix.
  • Saving the queue, with adding twice the capacity (similiar to span being alive)
    • inuse_space is similar but high alloc_space with the fix.
  • Saving the queue with adding 4 times the capacity
    • higher inuse_space without the fix but it is unusual to be using significantly more attributes than the capacity.

With MemLeak Fix, without saving queue, 2xCapacity

  trace git:(benchmark) ✗ go tool pprof --inuse_space -top heap.3.pprof
Type: inuse_space
Time: Mar 28, 2020 at 3:39pm (PDT)
Showing nodes accounting for 0, 0% of 0 total
      flat  flat%   sum%        cum   cum%

trace git:(benchmark) ✗ go tool pprof --alloc_space -top heap.3.pprof
Type: alloc_space
Time: Mar 28, 2020 at 3:39pm (PDT)
Showing nodes accounting for 15169.61MB, 100% of 15169.61MB total
      flat  flat%   sum%        cum   cum%
14712.61MB 96.99% 96.99% 14712.61MB 96.99%  go.opencensus.io/trace.(*evictedQueue).add (inline)
  457.01MB  3.01%   100% 15169.61MB   100%  main.main
         0     0%   100% 14712.61MB 96.99%  go.opencensus.io/trace.(*EQ).Add (inline)
         0     0%   100% 15169.61MB   100%  runtime.main

Without MemLeak Fix, without saving queue, 2xCapacity

trace git:(benchmark) ✗ go tool pprof --inuse_space -top heap.1.pprof
Type: inuse_space
Time: Mar 28, 2020 at 3:40pm (PDT)
Showing nodes accounting for 0, 0% of 0 total
      flat  flat%   sum%        cum   cum%
 trace git:(benchmark) ✗ go tool pprof --alloc_space -top heap.1.pprof
Type: alloc_space
Time: Mar 28, 2020 at 3:40pm (PDT)
Showing nodes accounting for 2369.77MB, 100% of 2370.28MB total
Dropped 7 nodes (cum <= 11.85MB)
      flat  flat%   sum%        cum   cum%
 1902.26MB 80.25% 80.25%  1902.26MB 80.25%  go.opencensus.io/trace.(*evictedQueue).add (inline)
  467.51MB 19.72%   100%  2369.77MB   100%  main.main
         0     0%   100%  1902.26MB 80.25%  go.opencensus.io/trace.(*EQ).Add (inline)
         0     0%   100%  2370.28MB   100%  runtime.main

With MemLeak Fix, saving queue, 2xCapacity

  trace git:(benchmark) ✗ go tool pprof --inuse_space -top heap.3.pprof
Type: inuse_space
Time: Mar 28, 2020 at 3:48pm (PDT)
Showing nodes accounting for 738.33MB, 100% of 738.33MB total
      flat  flat%   sum%        cum   cum%
  471.22MB 63.82% 63.82%   471.22MB 63.82%  go.opencensus.io/trace.(*evictedQueue).add (inline)
  205.50MB 27.83% 91.66%   738.33MB   100%  main.main
   38.50MB  5.21% 96.87%    38.50MB  5.21%  go.opencensus.io/trace.newEvictedQueue (inline)
   15.61MB  2.11% 98.98%    15.61MB  2.11%  main.save (inline)
    7.50MB  1.02%   100%       46MB  6.23%  go.opencensus.io/trace.NewEQ (inline)
         0     0%   100%   471.22MB 63.82%  go.opencensus.io/trace.(*EQ).Add (inline)
         0     0%   100%   738.33MB   100%  runtime.main

  trace git:(benchmark) ✗ go tool pprof --alloc_space -top heap.3.pprof
Type: alloc_space
Time: Mar 28, 2020 at 3:48pm (PDT)
Showing nodes accounting for 13677.47MB, 99.35% of 13767.24MB total
Dropped 3 nodes (cum <= 68.84MB)
      flat  flat%   sum%        cum   cum%
13269.47MB 96.38% 96.38% 13269.47MB 96.38%  go.opencensus.io/trace.(*evictedQueue).add (inline)
  408.01MB  2.96% 99.35% 13767.24MB   100%  main.main
         0     0% 99.35% 13269.47MB 96.38%  go.opencensus.io/trace.(*EQ).Add (inline)
         0     0% 99.35% 13767.24MB   100%  runtime.main

Without MemLeak Fix, saving queue, 2xCapacity

  trace git:(benchmark) ✗ go tool pprof --inuse_space -top heap.1.pprof
Type: inuse_space
Time: Mar 28, 2020 at 3:46pm (PDT)
Showing nodes accounting for 641.46MB, 99.92% of 641.99MB total
Dropped 3 nodes (cum <= 3.21MB)
      flat  flat%   sum%        cum   cum%
  425.91MB 66.34% 66.34%   425.91MB 66.34%  go.opencensus.io/trace.(*evictedQueue).add (inline)
     187MB 29.13% 95.47%   641.46MB 99.92%  main.main
   22.50MB  3.50% 98.98%    22.50MB  3.50%  go.opencensus.io/trace.newEvictedQueue (inline)
    3.55MB  0.55% 99.53%     3.55MB  0.55%  main.save (inline)
    2.50MB  0.39% 99.92%       25MB  3.89%  go.opencensus.io/trace.NewEQ (inline)
         0     0% 99.92%   425.91MB 66.34%  go.opencensus.io/trace.(*EQ).Add (inline)
         0     0% 99.92%   641.46MB 99.92%  runtime.main

  trace git:(benchmark) ✗ go tool pprof --alloc_space -top heap.1.pprof
Type: alloc_space
Time: Mar 28, 2020 at 3:46pm (PDT)
Showing nodes accounting for 1064.35MB, 99.90% of 1065.38MB total
Dropped 9 nodes (cum <= 5.33MB)
      flat  flat%   sum%        cum   cum%
  833.04MB 78.19% 78.19%   833.04MB 78.19%  go.opencensus.io/trace.(*evictedQueue).add (inline)
  190.50MB 17.88% 96.07%  1064.35MB 99.90%  main.main
   22.50MB  2.11% 98.19%    22.50MB  2.11%  go.opencensus.io/trace.newEvictedQueue (inline)
   15.81MB  1.48% 99.67%    15.81MB  1.48%  main.save (inline)
    2.50MB  0.23% 99.90%       25MB  2.35%  go.opencensus.io/trace.NewEQ (inline)
         0     0% 99.90%   833.04MB 78.19%  go.opencensus.io/trace.(*EQ).Add (inline)
         0     0% 99.90%  1064.85MB   100%  runtime.main

MemLeak Fix, saving queue, 4xCapacity

  trace git:(benchmark) ✗ go tool pprof --inuse_space -top heap.3.pprof
Type: inuse_space
Time: Mar 28, 2020 at 4:39pm (PDT)
Showing nodes accounting for 767.40MB, 100% of 767.40MB total
      flat  flat%   sum%        cum   cum%
  480.72MB 62.64% 62.64%   480.72MB 62.64%  go.opencensus.io/trace.(*evictedQueue).add (inline)
     232MB 30.23% 92.88%   767.40MB   100%  main.main
   37.50MB  4.89% 97.76%    37.50MB  4.89%  go.opencensus.io/trace.newEvictedQueue (inline)
    8.67MB  1.13% 98.89%     8.67MB  1.13%  main.save (inline)
    8.50MB  1.11%   100%       46MB  5.99%  go.opencensus.io/trace.NewEQ (inline)
         0     0%   100%   480.72MB 62.64%  go.opencensus.io/trace.(*EQ).Add (inline)
         0     0%   100%   767.40MB   100%  runtime.main

  trace git:(benchmark) ✗ go tool pprof --alloc_space -top heap.3.pprof
Type: alloc_space
Time: Mar 28, 2020 at 4:39pm (PDT)
Showing nodes accounting for 42053.74MB, 99.79% of 42142.78MB total
Dropped 3 nodes (cum <= 210.71MB)
      flat  flat%   sum%        cum   cum%
41151.72MB 97.65% 97.65% 41151.72MB 97.65%  go.opencensus.io/trace.(*evictedQueue).add (inline)
  902.01MB  2.14% 99.79% 42142.78MB   100%  main.main
         0     0% 99.79% 41151.72MB 97.65%  go.opencensus.io/trace.(*EQ).Add (inline)
         0     0% 99.79% 42142.78MB   100%  runtime.main

Without MemLeak Fix, saving queue, 4xCapacity

  trace git:(benchmark) ✗ go tool pprof --inuse_space -top heap.1.pprof
Type: inuse_space
Time: Mar 28, 2020 at 4:41pm (PDT)
Showing nodes accounting for 1091.19MB, 100% of 1091.19MB total
      flat  flat%   sum%        cum   cum%
  773.25MB 70.86% 70.86%   773.25MB 70.86%  go.opencensus.io/trace.(*evictedQueue).add (inline)
  279.50MB 25.61% 96.48%  1091.19MB   100%  main.main
      27MB  2.47% 98.95%       27MB  2.47%  go.opencensus.io/trace.newEvictedQueue (inline)
    6.94MB  0.64% 99.59%     6.94MB  0.64%  main.save (inline)
    4.50MB  0.41%   100%    31.50MB  2.89%  go.opencensus.io/trace.NewEQ (inline)
         0     0%   100%   773.25MB 70.86%  go.opencensus.io/trace.(*EQ).Add (inline)
         0     0%   100%  1091.19MB   100%  runtime.main

  trace git:(benchmark) ✗ go tool pprof --alloc_space -top heap.1.pprof
Type: alloc_space
Time: Mar 28, 2020 at 4:41pm (PDT)
Showing nodes accounting for 3687.26MB, 100% of 3687.76MB total
Dropped 2 nodes (cum <= 18.44MB)
      flat  flat%   sum%        cum   cum%
 2955.42MB 80.14% 80.14%  2955.42MB 80.14%  go.opencensus.io/trace.(*evictedQueue).add (inline)
  665.01MB 18.03% 98.17%  3687.26MB   100%  main.main
   35.33MB  0.96% 99.13%    35.33MB  0.96%  main.save (inline)
      27MB  0.73% 99.86%       27MB  0.73%  go.opencensus.io/trace.newEvictedQueue (inline)
    4.50MB  0.12%   100%    31.50MB  0.85%  go.opencensus.io/trace.NewEQ (inline)
         0     0%   100%  2955.42MB 80.14%  go.opencensus.io/trace.(*EQ).Add (inline)
         0     0%   100%  3687.76MB   100%  runtime.main

Copy link
Author

Choose a reason for hiding this comment

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

It is true that I didn't think it through, what about a ring buffer?

@cakiecakie cakiecakie force-pushed the feature/fix_evicted_queue_memory_leak branch 2 times, most recently from 91873be to c04732d Compare May 22, 2020 06:08
@cakiecakie cakiecakie force-pushed the feature/fix_evicted_queue_memory_leak branch from c04732d to b08a207 Compare May 22, 2020 08:55
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@rghetia rghetia rghetia left review comments

@rakyll rakyll Awaiting requested review from rakyll

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cakiecakie @googlebot @rghetia