OCPBUGS-17157: cache/json: use shared buffers for JSON decoding #1140

stevekuznetsov · 2023-09-06T14:56:02Z

This commit uses a shared buffer in a shared decoder for the ListBundles call, which should reduce our memory footprint when we're asked for this data.

I also changed the sorting of the key-set to be an explicit call to sort.Slice, instead of an implicit side-effect from sets.New().List().

This commit uses a shared buffer in a shared decoder for the ListBundles call, which should reduce our memory footprint when we're asked for this data. I also changed the sorting of the key-set to be an explicit call to sort.Slice, instead of an implicit side-effect from sets.New().List(). Signed-off-by: Steve Kuznetsov <[email protected]>

openshift-ci-robot · 2023-09-06T14:56:07Z

@stevekuznetsov: This pull request references Jira Issue OCPBUGS-17157, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.14.0) matches configured target version for branch (4.14.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @jianzhangbjz

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

This commit uses a shared buffer in a shared decoder for the ListBundles call, which should reduce our memory footprint when we're asked for this data.

I also changed the sorting of the key-set to be an explicit call to sort.Slice, instead of an implicit side-effect from sets.New().List().

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

stevekuznetsov · 2023-09-06T14:56:13Z

/assign @joelanford @grokspawn

stevekuznetsov · 2023-09-06T15:46:18Z

Local testing only shows modest improvements, but these will be true regardless of GC performance, whereas os.ReadFile() can only perform this well when called N times if we have time to GC.

Here's the pprof diff while running this in the background:

while true; do grpcurl -plaintext 127.0.0.1:50051 api.Registry/ListBundles; sleep 1; done

$ go tool pprof -diff_base /home/stevekuznetsov/pprof/pprof.opm.alloc_objects.alloc_space.inuse_objects.inuse_space.015.pb.gz /home/stevekuznetsov/pprof/pprof.opm.alloc_objects.alloc_space.inuse_objects.inuse_space.012.pb.gz
File: opm
Build ID: f7be9c041bb04f86b16d09314008a9af727f3b81
Type: inuse_space
Time: Sep 6, 2023 at 9:26am (MDT)
Entering interactive mode (type "help" for commands, "o" for options)
(pprof) top 
Showing nodes accounting for -571.39kB, 7.90% of 7234.88kB total
Showing top 10 nodes out of 98
      flat  flat%   sum%        cum   cum%
-1319.97kB 18.24% 18.24% -1319.97kB 18.24%  os.ReadFile
 1184.27kB 16.37%  1.88%  1184.27kB 16.37%  encoding/json.(*Decoder).refill
-1025.63kB 14.18% 16.05% -1025.63kB 14.18%  regexp/syntax.(*compiler).inst
 -627.57kB  8.67% 24.73%  -627.57kB  8.67%  encoding/json.unquoteBytes
  600.16kB  8.30% 16.43%   -27.41kB  0.38%  encoding/json.(*decodeState).literalStore
  557.26kB  7.70%  8.73%   557.26kB  7.70%  github.com/russross/blackfriday/v2.init
  553.04kB  7.64%  1.08%   553.04kB  7.64%  github.com/gogo/protobuf/proto.RegisterType
  536.37kB  7.41%  6.33%   536.37kB  7.41%  k8s.io/apimachinery/pkg/api/meta.init
 -516.01kB  7.13%   0.8%  -516.01kB  7.13%  regexp/syntax.appendRange
 -513.31kB  7.09%  7.90%  -513.31kB  7.09%  k8s.io/apimachinery/pkg/conversion.ConversionFuncs.AddUntyped

pkg/cache/json.go

grokspawn · 2023-09-07T16:04:19Z

pkg/cache/json.go

+			// The SQLite-based server
+			// configures its querier to
+			// omit these fields when
+			// key path is set.


nit:

Suggested change

// key path is set.

// bundle path is set.

openshift-ci · 2023-09-07T16:05:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: grokspawn, stevekuznetsov

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [grokspawn]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

awgreene

One question, looks good otherwise.

awgreene · 2023-09-07T16:13:17Z

pkg/cache/json.go

+	sort.Slice(keys, func(i, j int) bool {
+		if keys[i].chName != keys[j].chName {
+			return keys[i].chName < keys[j].chName
+		}
+		if keys[i].pkgName != keys[j].pkgName {
+			return keys[i].pkgName < keys[j].pkgName
+		}
+		return keys[i].name < keys[j].name
+	})


Why are we ordering the list here?

The ordering existed before, I just made it explicit. I imagine consumers (or tests) of this output benefit from a stable list.

For e.g. #1069

openshift-ci-robot · 2023-09-07T16:27:56Z

@stevekuznetsov: Jira Issue OCPBUGS-17157: Some pull requests linked via external trackers have merged:

The following pull requests linked via external trackers have not merged:

operator-framework/operator-marketplace#530 is open

These pull request must merge or be unlinked from the Jira bug in order for it to move to the next state. Once unlinked, request a bug refresh with /jira refresh.

Jira Issue OCPBUGS-17157 has not been moved to the MODIFIED state.

In response to this:

This commit uses a shared buffer in a shared decoder for the ListBundles call, which should reduce our memory footprint when we're asked for this data.

I also changed the sorting of the key-set to be an explicit call to sort.Slice, instead of an implicit side-effect from sets.New().List().

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Sep 6, 2023

openshift-ci-robot added the jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. label Sep 6, 2023

openshift-ci bot requested review from grokspawn, joelanford and jianzhangbjz September 6, 2023 14:56

openshift-ci bot assigned grokspawn and joelanford Sep 6, 2023

stevekuznetsov mentioned this pull request Aug 28, 2023

Improve Memory Utilization for Microshift Deployment openshift/operator-framework-olm#530

Closed

38 tasks

joelanford reviewed Sep 6, 2023

View reviewed changes

pkg/cache/json.go Show resolved Hide resolved

joelanford reviewed Sep 6, 2023

View reviewed changes

pkg/cache/json.go Show resolved Hide resolved

grokspawn reviewed Sep 7, 2023

View reviewed changes

grokspawn approved these changes Sep 7, 2023

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 7, 2023

awgreene reviewed Sep 7, 2023

View reviewed changes

stevekuznetsov added the lgtm Indicates that a PR is ready to be merged. label Sep 7, 2023

openshift-merge-robot merged commit 5ca4a9f into operator-framework:master Sep 7, 2023
10 checks passed

github-actions bot mentioned this pull request Oct 20, 2023

[operator-framework/operator-registry] v1.30.0 vdemeester/chapeau-rouge#50

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OCPBUGS-17157: cache/json: use shared buffers for JSON decoding #1140

OCPBUGS-17157: cache/json: use shared buffers for JSON decoding #1140

stevekuznetsov commented Sep 6, 2023

openshift-ci-robot commented Sep 6, 2023

stevekuznetsov commented Sep 6, 2023

stevekuznetsov commented Sep 6, 2023 •

edited

Loading

grokspawn Sep 7, 2023 •

edited

Loading

openshift-ci bot commented Sep 7, 2023

awgreene left a comment

awgreene Sep 7, 2023

stevekuznetsov Sep 7, 2023

grokspawn Sep 8, 2023

openshift-ci-robot commented Sep 7, 2023

OCPBUGS-17157: cache/json: use shared buffers for JSON decoding #1140

OCPBUGS-17157: cache/json: use shared buffers for JSON decoding #1140

Conversation

stevekuznetsov commented Sep 6, 2023

openshift-ci-robot commented Sep 6, 2023

stevekuznetsov commented Sep 6, 2023

stevekuznetsov commented Sep 6, 2023 • edited Loading

grokspawn Sep 7, 2023 • edited Loading

Choose a reason for hiding this comment

openshift-ci bot commented Sep 7, 2023

awgreene left a comment

Choose a reason for hiding this comment

awgreene Sep 7, 2023

Choose a reason for hiding this comment

stevekuznetsov Sep 7, 2023

Choose a reason for hiding this comment

grokspawn Sep 8, 2023

Choose a reason for hiding this comment

openshift-ci-robot commented Sep 7, 2023

stevekuznetsov commented Sep 6, 2023 •

edited

Loading

grokspawn Sep 7, 2023 •

edited

Loading