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

Minimal change to ensure that the bulky leaseEntry isn't kept in memory. #10726

Merged
merged 2 commits into from
Jan 19, 2021

Conversation

ncabatoff
Copy link
Collaborator

This is a subset of #10666 that just tries to get the biggest improvement for the smallest change. I re-ran my tests but only used inmem storage for expediency, so I filter out the heap pprof nodes involving BarrierView. Here are the numbers for 1.6.1, #10666, and this PR, all tested with 360000 cert leases:

1.6.1:

Active filters:
   ignore=BarrierView
Showing nodes accounting for 734.27MB, 38.55% of 1904.72MB total
Dropped 194 nodes (cum <= 9.52MB)
Showing top 15 nodes out of 82
      flat  flat%   sum%        cum   cum%
  254.57MB 13.37% 13.37%   328.59MB 17.25%  github.com/hashicorp/vault/builtin/credential/cert.(*backend).pathLogin
  191.05MB 10.03% 23.40%   191.05MB 10.03%  github.com/hashicorp/vault/vault.(*ExpirationManager).leaseTimesForExport (inline)
   47.01MB  2.47% 25.86%   367.46MB 19.29%  github.com/hashicorp/vault/vault.(*ExpirationManager).RegisterAuth
   42.51MB  2.23% 28.10%    42.51MB  2.23%  strings.(*Builder).grow (inline)
   42.50MB  2.23% 30.33%    42.50MB  2.23%  bytes.(*Buffer).String (inline)
   27.62MB  1.45% 31.78%    27.62MB  1.45%  sync.(*Map).dirtyLocked
      26MB  1.37% 33.14%    28.42MB  1.49%  time.AfterFunc
      24MB  1.26% 34.40%   274.44MB 14.41%  github.com/hashicorp/vault/vault.(*ExpirationManager).updatePendingInternal
   14.50MB  0.76% 35.16%       15MB  0.79%  github.com/hashicorp/vault/vault.(*Core).fetchEntityAndDerivedPolicies
   12.50MB  0.66% 35.82%    12.50MB  0.66%  fmt.Sprintf
   12.50MB  0.66% 36.48%    12.50MB  0.66%  github.com/hashicorp/vault/vendor/github.com/hashicorp/vault/sdk/helper/base62.RandomWithReader
   11.50MB   0.6% 37.08%   768.57MB 40.35%  github.com/hashicorp/vault/vault.(*Core).handleLoginRequest
      11MB  0.58% 37.66%    11.50MB   0.6%  github.com/hashicorp/vault/vendor/github.com/hashicorp/vault/sdk/helper/strutil.RemoveDuplicates
      10MB  0.53% 38.18%       10MB  0.53%  math/big.(*Int).Text (inline)
       7MB  0.37% 38.55%        7MB  0.37%  context.(*cancelCtx).Done

This PR:

Active filters:
   ignore=BarrierView
Showing nodes accounting for 345.99MB, 22.80% of 1517.76MB total
Dropped 179 nodes (cum <= 7.59MB)
Showing top 15 nodes out of 92
      flat  flat%   sum%        cum   cum%
  182.05MB 11.99% 11.99%   182.05MB 11.99%  github.com/hashicorp/vault/vault.(*ExpirationManager).leaseTimesForExport (inline)
   46.51MB  3.06% 15.06%    46.51MB  3.06%  strings.(*Builder).grow
   28.50MB  1.88% 16.94%    31.25MB  2.06%  time.AfterFunc
   28.50MB  1.88% 18.81%   271.34MB 17.88%  github.com/hashicorp/vault/vault.(*ExpirationManager).updatePendingInternal
   27.62MB  1.82% 20.63%    27.62MB  1.82%  sync.(*Map).dirtyLocked
       7MB  0.46% 21.10%        7MB  0.46%  context.(*cancelCtx).Done
    4.50MB   0.3% 21.39%    14.09MB  0.93%  context.WithDeadline
    4.50MB   0.3% 21.69%    34.63MB  2.28%  sync.(*Map).Store
       4MB  0.26% 21.95%        4MB  0.26%  context.WithCancel
    3.54MB  0.23% 22.19%     3.54MB  0.23%  github.com/beorn7/perks/quantile.newStream (inline)
    2.75MB  0.18% 22.37%     2.75MB  0.18%  time.startTimer
    2.51MB  0.17% 22.53%     2.51MB  0.17%  bufio.NewReaderSize
    2.50MB  0.16% 22.70%     2.50MB  0.16%  sync.newEntry (inline)
       1MB 0.066% 22.76%     4.55MB   0.3%  github.com/prometheus/client_golang/prometheus.newSummary
    0.50MB 0.033% 22.80%        4MB  0.26%  crypto/x509.parseCertificate

#10666:

Active filters:
   ignore=BarrierView
Showing nodes accounting for 200.68MB, 15.14% of 1325.70MB total
Dropped 93 nodes (cum <= 6.63MB)
Showing top 15 nodes out of 108
      flat  flat%   sum%        cum   cum%
   65.01MB  4.90%  4.90%   132.01MB  9.96%  github.com/hashicorp/vault/vault.(*ExpirationManager).updatePendingInternal
   40.50MB  3.06%  7.96%    40.50MB  3.06%  strings.(*Builder).grow
      33MB  2.49% 10.45%    36.93MB  2.79%  time.AfterFunc
   27.62MB  2.08% 12.53%    27.62MB  2.08%  sync.(*Map).dirtyLocked
       6MB  0.45% 12.98%        6MB  0.45%  context.(*cancelCtx).Done
    4.55MB  0.34% 13.33%     4.55MB  0.34%  github.com/beorn7/perks/quantile.newStream
       4MB   0.3% 13.63%    11.06MB  0.83%  context.WithDeadline
       4MB   0.3% 13.93%    34.13MB  2.57%  sync.(*Map).Store
    3.93MB   0.3% 14.23%     3.93MB   0.3%  time.startTimer
    2.50MB  0.19% 14.42%     2.50MB  0.19%  context.WithCancel
    2.50MB  0.19% 14.61%     2.50MB  0.19%  sync.newEntry (inline)
    2.05MB  0.15% 14.76%     2.05MB  0.15%  github.com/beorn7/perks/quantile.(*stream).merge
       2MB  0.15% 14.91%        2MB  0.15%  encoding/json.(*Decoder).refill
    1.51MB  0.11% 15.02%     1.51MB  0.11%  bufio.NewReaderSize
    1.50MB  0.11% 15.14%     1.50MB  0.11%  net/http.(*http2Framer).readMetaFrame.func1

@ncabatoff ncabatoff added this to the 1.6.2 milestone Jan 19, 2021
Copy link
Contributor

@mgritter mgritter left a comment

Choose a reason for hiding this comment

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

This looks very good, but isn't there another ExpireLeaseStrategy around somewhere? Maybe that's enterprise?

@mgritter
Copy link
Contributor

The other function whose signature would have to be altered is expireLeaseStrategyRemoveFromCache in expiration_util_ent.go.

@ncabatoff
Copy link
Collaborator Author

The other function whose signature would have to be altered is expireLeaseStrategyRemoveFromCache in expiration_util_ent.go.

Yup, I'm aware but thanks for the reminder. It's trivial to adapt it as it only cares about leaseID.

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