Speedup jsonnet generation by running in parallel #1908

sthaha · 2023-03-02T02:30:23Z

I added CHANGELOG entry for this change.
No user facing changes, so no entry in CHANGELOG was needed.

jan--f · 2023-03-07T16:10:38Z

Do you have some performance numbers on the speedup you're seeing? I did a make clean; time make generate (run multiple times in a row) on master and on your PR with counter intuitive results:
master:

make generate  32.63s user 4.52s system 65% cpu 56.911 total
make generate  33.86s user 4.96s system 63% cpu 1:01.18 total

This PR:

make generate  43.42s user 6.48s system 114% cpu 43.555 total
make generate  43.82s user 5.42s system 125% cpu 39.260 total

sthaha · 2023-03-08T06:33:07Z

Do you have some performance numbers on the speedup you're seeing? I did a make clean; time make generate (run multiple times in a row) on master and on your PR with counter intuitive results: master:

I don't think make generate is the correct target to run since it does more than just generate yaml from jsonnet. Please try
time -p make build-jsonnet which produces the following on my machine. Are you running this in a container with only one cpu allocated by any chance?

time -p make build-jsonnet

# master 
  attempt 1: real 28.69. user 27.60  sys 4.08
  attempt 2: real 28.40 user 27.30 sys 4.11

# this pr 
  attempt 1: real 10.62 user 34.76 sys 5.13
  attempt 2: real 10.68 user 34.92 sys 5.15

If you checkout the pr and run this locally, the difference is quite perceivable (at least one my 8 core linux box).

May be try to look at only the total time

export TIMEFMT='%E'; time make build-jsonnet

On my machine, I see that this PR makes it at least 3 times faster 28.88s vs 10.81s.

jan--f · 2023-03-08T09:50:00Z

I still get very similar results with make clean; export TIMEFMT='%E'; time make build-jsonnet:
main:

47.94s 47.37s

PR:

45.35s 39.62s

sthaha · 2023-03-08T11:53:38Z

make clean; export TIMEFMT='%E'; time make build-jsonnet

I think this could be the issue, that make clean deletes everything, so the time is actually measuring all the download of tools , jsonnet bundles etc .. E.g.

❯ make clean; export TIMEFMT='%E'; time make build-jsonnet
rm -rf jsonnet/vendor operator .hack-operator-image tmp/
mkdir -p /home/sthaha/dev/rh/monitoring/downstream/cmo/wt/improve-jsonnet-gen/tmp/bin
Installing tools from hack/tools.go
go build '-mod=mod' -o cmo/wt/improve-jsonnet-gen/tmp/bin github.com/brancz/gojsontoyaml
go build '-mod=mod' -o cmo/wt/improve-jsonnet-gen/tmp/bin github.com/campoy/embedmd
go build '-mod=mod' -o cmo/wt/improve-jsonnet-gen/tmp/bin github.com/google/go-jsonnet/cmd/jsonnet
go build '-mod=mod' -o cmo/wt/improve-jsonnet-gen/tmp/bin github.com/google/go-jsonnet/cmd/jsonnetfmt
go build '-mod=mod' -o cmo/wt/improve-jsonnet-gen/tmp/bin github.com/jsonnet-bundler/jsonnet-bundler/cmd/jb
go build '-mod=mod' -o cmo/wt/improve-jsonnet-gen/tmp/bin github.com/prometheus/prometheus/cmd/promtool
cd jsonnet && cmo/wt/improve-jsonnet-gen/tmp/bin/jb install
GET https://github.com/prometheus-operator/kube-prometheus/archive/74cb76d338f021bdca9af635c2eef6b196804b57.tar.gz 200
GET https://github.com/prometheus-operator/prometheus-operator/archive/f337e4ecf88d2c228c2325249bf338c6aa999488.tar.gz 200
GET https://github.com/openshift/openshift-state-metrics/archive/7beb8807e2c0092b5e580a29903ff060140d8ff0.tar.gz 200
GET https://github.com/openshift/telemeter/archive/ee1ba4699b82ecb2033f886af12b9e2e451d2296.tar.gz 200

I think a better way is to time the script itself. Could you please try this?

diff --git a/Makefile b/Makefile
index b77423bc..2cf60da1 100644
--- a/Makefile
+++ b/Makefile
@@ -130,7 +130,7 @@ $(ASSETS): build-jsonnet
 
 .PHONY: build-jsonnet
 build-jsonnet: $(JSONNET_BIN) $(GOJSONTOYAML_BIN) $(JSONNET_SRC) $(JSONNET_VENDOR) json-manifests json-crds
-	./hack/build-jsonnet.sh
+	time ./hack/build-jsonnet.sh
 
 $(JSON_MANIFESTS): $(MANIFESTS)
 	cat $(MANIFESTS_DIR)/$(patsubst %.json,%.yaml,$(@F)) | $(GOJSONTOYAML_BIN) -yamltojson > $@

jan--f · 2023-03-08T13:48:43Z

Yeah that looks more promising:

master:

real	0m19.456s
user	0m19.835s
sys	0m1.445s
52.00s

real	0m20.376s
user	0m20.634s
sys	0m1.533s
49.92s

PR:

real	0m8.408s
user	0m31.937s
sys	0m2.293s
38.19s

real	0m8.448s
user	0m31.868s
sys	0m2.275s
39.01s

jan--f · 2023-03-08T13:59:13Z

hack/build-jsonnet.sh

+for file in "${files[@]}"; do
+	(
+		dir=$(dirname "${file}")
+		path="${prefix}/${dir}"
+		mkdir -p "${path}"
+
+		# convert file name from camelCase to snake-case
+		fullfile=$(echo "${file}" | sed 's/\(.\)\([A-Z]\)/\1-\2/g' | tr '[:upper:]' '[:lower:]')
+		jq -r ".[\"${file}\"]" "${TMP}/main.json" | gojsontoyaml > "${prefix}/${fullfile}.yaml"
+	)&


Suggested change

for file in "${files[@]}"; do

(

dir=$(dirname "${file}")

path="${prefix}/${dir}"

mkdir -p "${path}"

# convert file name from camelCase to snake-case

fullfile=$(echo "${file}" | sed 's/$.$$[A-Z]$/\1-\2/g' | tr '[:upper:]' '[:lower:]')

jq -r ".[\"${file}\"]" "${TMP}/main.json" | gojsontoyaml > "${prefix}/${fullfile}.yaml"

)&

for file in "${files[@]}"; do

dir=$(dirname "${file}")

path="${prefix}/${dir}"

mkdir -p "${path}"

# convert file name from camelCase to snake-case

fullfile=$(echo "${file}" | sed 's/$.$$[A-Z]$/\1-\2/g' | tr '[:upper:]' '[:lower:]')

jq -r ".[\"${file}\"]" "${TMP}/main.json" | gojsontoyaml > "${prefix}/${fullfile}.yaml" &

I get slightly better results with this change (shave a few seconds off the results fairly consistently). Can you give this a try as well?

This was my first version :) but I felt starting a subshell at the start of for loop makes it easier to reason with. Since it boils down to

for f in files { ( process this file) & // in background wait if there are more than N processes }

I think, we can achieve the same without starting a subshell by using a block and run that in background.

Previously, jsonnet to yaml conversion was done sequentially and this took quite some time. The patch speeds that up by running them in parallel . The number of jobs to run in parallel is determined by the number of CPU cores (thus requires nproc to be in path). Signed-off-by: Sunil Thaha <[email protected]>

sthaha · 2023-03-09T02:44:08Z

/retest required

openshift-ci · 2023-03-09T02:44:11Z

@sthaha: The /retest command does not accept any targets.
The following commands are available to trigger required jobs:

/test e2e-agnostic-operator
/test e2e-aws-ovn
/test e2e-aws-ovn-upgrade
/test generate
/test go-fmt
/test images
/test jsonnet-fmt
/test rules
/test shellcheck
/test unit
/test vendor
/test verify

The following commands are available to trigger optional jobs:

/test e2e-aws-ovn-single-node
/test versions

Use /test all to run all jobs.

In response to this:

/retest required

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.

sthaha · 2023-03-09T02:51:39Z

/retest-required

sthaha · 2023-03-09T06:40:49Z

hack/build-jsonnet.sh

+		# convert file name from camelCase to snake-case
+		fullfile=$(echo "${file}" | sed 's/\(.\)\([A-Z]\)/\1-\2/g' | tr '[:upper:]' '[:lower:]')
+		jq -r ".[\"${file}\"]" "${TMP}/main.json" | gojsontoyaml > "${prefix}/${fullfile}.yaml"
+	}&


@jan--f this should give us the same perf as jq -r ".[\"${file}\"]" "${TMP}/main.json" | gojsontoyaml > "${prefix}/${fullfile}.yaml" & with the additional benefit of easier to reason with. I.E. consider the entire block as a function.

sthaha · 2023-03-09T08:14:12Z

/retest-required

sthaha · 2023-03-10T00:34:36Z

/retest-required

sthaha · 2023-03-13T00:07:08Z

/retest-required

raptorsun · 2023-03-13T10:52:45Z

/retest-required

raptorsun · 2023-03-14T22:58:06Z

/retest-required

sthaha · 2023-03-27T03:02:42Z

/retest-required

sthaha · 2023-04-03T03:43:26Z

/retest-required

sthaha · 2023-04-03T07:22:23Z

@jan--f now that the CI gods have blessed this, can merge this one please?

jan--f · 2023-04-03T13:20:47Z

yes, sorry for the lag!
just did a run again and get consistently numbers like real 0m19.300s prior to this change, real 0m7.420s with this change.
/lgtm
/label docs-approved
/label px-approved
/label qe-approved
Adding labels for an internal build optimization.

openshift-ci · 2023-04-03T13:23:24Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jan--f, sthaha

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~~ [jan--f,sthaha]

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

openshift-ci · 2023-04-03T15:55:05Z

@sthaha: all tests passed!

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

openshift-ci bot requested review from raptorsun and simonpasquier March 2, 2023 02:30

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

jan--f reviewed Mar 8, 2023

View reviewed changes

sthaha force-pushed the improve-jsonnet-gen branch from 3305ebf to 03f4e79 Compare March 9, 2023 00:54

sthaha force-pushed the improve-jsonnet-gen branch from 03f4e79 to 7eb4068 Compare March 9, 2023 00:55

sthaha commented Mar 9, 2023

View reviewed changes

openshift-ci bot added docs-approved Signifies that Docs has signed off on this PR px-approved Signifies that Product Support has signed off on this PR qe-approved Signifies that QE has signed off on this PR labels Apr 3, 2023

openshift-ci bot assigned jan--f Apr 3, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 3, 2023

openshift-merge-robot merged commit 0142eb7 into openshift:master Apr 3, 2023

Speedup jsonnet generation by running in parallel #1908

Speedup jsonnet generation by running in parallel #1908

Uh oh!

Conversation

sthaha commented Mar 2, 2023

Uh oh!

jan--f commented Mar 7, 2023

Uh oh!

sthaha commented Mar 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jan--f commented Mar 8, 2023

Uh oh!

sthaha commented Mar 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jan--f commented Mar 8, 2023

Uh oh!

jan--f Mar 8, 2023

Choose a reason for hiding this comment

Uh oh!

sthaha Mar 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sthaha commented Mar 9, 2023

Uh oh!

openshift-ci bot commented Mar 9, 2023

Uh oh!

sthaha commented Mar 9, 2023

Uh oh!

sthaha Mar 9, 2023

Choose a reason for hiding this comment

Uh oh!

sthaha commented Mar 9, 2023

Uh oh!

sthaha commented Mar 10, 2023

Uh oh!

sthaha commented Mar 13, 2023

Uh oh!

raptorsun commented Mar 13, 2023

Uh oh!

raptorsun commented Mar 14, 2023

Uh oh!

sthaha commented Mar 27, 2023

Uh oh!

sthaha commented Apr 3, 2023

Uh oh!

sthaha commented Apr 3, 2023

Uh oh!

jan--f commented Apr 3, 2023

Uh oh!

openshift-ci bot commented Apr 3, 2023

Uh oh!

openshift-ci bot commented Apr 3, 2023

Uh oh!

Uh oh!

sthaha commented Mar 8, 2023 •

edited

Loading

sthaha commented Mar 8, 2023 •

edited

Loading

sthaha Mar 9, 2023 •

edited

Loading