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

performance issues yq #261

Closed
Morriz opened this issue Dec 22, 2020 · 15 comments · Fixed by #288
Closed

performance issues yq #261

Morriz opened this issue Dec 22, 2020 · 15 comments · Fixed by #288
Assignees
Labels
problem Something seems not right

Comments

@Morriz
Copy link
Contributor

Morriz commented Dec 22, 2020

The 5 bats tests are not performing well. They should run under 5 seconds imo. Can this be optimized?

@Morriz Morriz added the problem Something seems not right label Dec 22, 2020
@0-sv
Copy link
Contributor

0-sv commented Dec 23, 2020

Yes, I agree. It can be optimized, I wasn't sure if it was inside the scope in #203.

I have some ideas:

  • Test everything from bootstrap.sh in one function, but this reduces test coverage.
  • The true bottleneck is in what is being tested in bootstrap.sh. Currently, it's just one big script. We could write functions for the logic being tested and test those functions separately.
  • The Github Actions runner could run on a dedicated server with more horsepower, since all the tests combined take a long time anyway.
  • For some of the logic in bootstrap.sh, it's not quite DRY. I think the original intention was to setup everything for the first time, which makes sense. However, I'm also quite annoyed by the performance of this script, so I've just sym-linked the files I need into my otomi-values repo and now I never run it. Decoupling this script into multiple scripts that each have their own purpose would be great imo, and then they can be tested separately too, which will increase performance.

@Morriz
Copy link
Contributor Author

Morriz commented Dec 23, 2020

Nah, you are overthinking it as the script only copies some files. Please understand that symlinking is for devs only, so no solution here. We need to time the operations. Is the file mount on the container inducing so much latency when it comes to copying? Instrument the code to show times with -x or something

@Morriz
Copy link
Contributor Author

Morriz commented Jan 13, 2021

I think we have to look into decryption. Is that happening every call?

@0-sv
Copy link
Contributor

0-sv commented Jan 14, 2021

The latency in bin/bootstrap.sh occurs over here:

+ yq d - properties.toolsVersion
+ yq d /Users/sebastiaanverbeek/Projects/redkubes/dev/otomi-core/values-schema.yaml '**.required.'
+ yq d - properties.cluster
****LATENCY****
+ '[' /Users/sebastiaanverbeek/Projects/redkubes/dev/otomi-core '!=' /home/app/stack ']'
+ cp ./env/.vscode/values-schema.yaml .values/
+ echo 'Stored JSON schema at: ./env/.vscode/values-schema.yaml'

I added that line with 'LATENCY' of course. I don't know what to make of it though.

@0-sv
Copy link
Contributor

0-sv commented Jan 14, 2021

Reproduce it by running:

local targetPath="$ENV_DIR/.vscode/values-schema.yaml"                                                       
local sourcePath="$PWD/values-schema.yaml"
yq d $sourcePath '**.required.' | yq d - 'properties.toolsVersion' | yq d - 'properties.cluster' >$targetPath

@0-sv 0-sv changed the title performance issues bats performance issues yq Jan 18, 2021
@0-sv
Copy link
Contributor

0-sv commented Jan 18, 2021

Related: mikefarah/yq#422

Is it possible to follow the suggestion and downgrade yq even more? I'm not sure about the new version, maybe use that where appropriate

@j-zimnowoda
Copy link
Contributor

j-zimnowoda commented Jan 18, 2021

I see that mikefarah/yq#422 is solved in version 3.3.1
The otomi-tools is using version 3.3.2, why do you think that downgrading yq will improve performance?

@0-sv
Copy link
Contributor

0-sv commented Jan 18, 2021

Well apparently there was a version where this wasn't an issue: "after downgrade to yq 2.4.1 --> zippy fast again (ie: 30s to parse yaml file in 3.x became sub second response time with 2.4.1)"

@0-sv
Copy link
Contributor

0-sv commented Jan 18, 2021

The otomi-tools is using version 3.3.2, why do you think that downgrading yq will improve performance?

Well the issue isn't solved in 3.3.4 which is what I'm running:

yq version 3.4.1

@0-sv
Copy link
Contributor

0-sv commented Jan 18, 2021

Let's wait for #269 as discussed in Slack.

@Dunky13
Copy link
Contributor

Dunky13 commented Jan 20, 2021

#271 Doesn't solve it, does the following command work:
yq r -j $sourcePath | jq "del(.. | .required?)" | jq "del( .properties.toolsVersion? )" | jq "del( .properties.cluster)" | yq r --prettyPrint -
Could use a little polishing, maybe combine the jq commands into one?

@0-sv
Copy link
Contributor

0-sv commented Jan 21, 2021

Yes, if these are equivalent then it this is a solution, the performance is fine now. Do you want to create a pr?

@Dunky13
Copy link
Contributor

Dunky13 commented Jan 21, 2021

Maybe this works: yq r -j $sourcePath | jq "del(.. | .required?) | del( .properties.toolsVersion? ) | del( .properties.cluster)" | yq r --prettyPrint -

@Morriz
Copy link
Contributor Author

Morriz commented Jan 22, 2021

@Dunky13 can you create that PR then so we can benefit soon?

Dunky13 pushed a commit that referenced this issue Jan 22, 2021
Using jq for faster recursive key deletion.

fix #261
@Dunky13
Copy link
Contributor

Dunky13 commented Jan 22, 2021

@Dunky13 can you create that PR then so we can benefit soon?

PR is open, ready for check & merge

Dunky13 pushed a commit that referenced this issue Jan 22, 2021
Removed "unneeded chars"

#288 #261
Dunky13 pushed a commit that referenced this issue Jan 29, 2021
Write a bats test to check whether yq & jq work correctly.

#261 #288
Dunky13 pushed a commit that referenced this issue Jan 29, 2021
Removed a couple of "configurable" options and made them hard coded as well as fixing a couple of
bash issues like `"$@"`

#261 #288
Morriz pushed a commit that referenced this issue Feb 4, 2021
Dunky13 added a commit that referenced this issue May 27, 2021
Ani1357 pushed a commit that referenced this issue Oct 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
problem Something seems not right
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants