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

Fix getSnippetHash not considering all files #765

Merged
merged 3 commits into from
Sep 28, 2022

Conversation

julienduchesne
Copy link
Member

@julienduchesne julienduchesne commented Sep 27, 2022

Made a stupid mistake in the previous PR: #759 🤦 🤦. Instead of a continue, I put a return nil, so whenever a file is already handled, the function doesn't consider other imports in the file

This fixes it and adds another benchmark test to ensure it doesn't happen again.

I also removed the Github Actions benchmark test, as it's not really useful, anytime we change the tests, we'll get erroneous results which will be annoying.
Instead, I added the benchmark tests to the Drone run, we can compare whenever we want.

Benchmark comparison with v0.22.1:

$ benchstat test-0.22.1.txt test-new.txt 
name                                     old time/op    new time/op    delta
pkg:github.com/grafana/tanka/pkg/jsonnet goos:darwin goarch:amd64
GetSnippetHash/all-imported-from-main-8    40.6ms ±10%    30.1ms ± 4%  -25.81%  (p=0.008 n=5+5)
GetSnippetHash/deeply-nested-8             42.7ms ± 4%    30.0ms ± 6%  -29.64%  (p=0.008 n=5+5)

name                                     old alloc/op   new alloc/op   delta
pkg:github.com/grafana/tanka/pkg/jsonnet goos:darwin goarch:amd64
GetSnippetHash/all-imported-from-main-8    15.4MB ± 2%     6.2MB ± 0%  -60.06%  (p=0.008 n=5+5)
GetSnippetHash/deeply-nested-8             15.6MB ± 1%     6.2MB ± 0%  -60.69%  (p=0.008 n=5+5)

name                                     old allocs/op  new allocs/op  delta
pkg:github.com/grafana/tanka/pkg/jsonnet goos:darwin goarch:amd64
GetSnippetHash/all-imported-from-main-8      152k ± 2%       47k ± 0%  -68.86%  (p=0.008 n=5+5)
GetSnippetHash/deeply-nested-8               154k ± 1%       47k ± 0%  -69.17%  (p=0.008 n=5+5)

whereas the same test just fails on the main branch. I also verified with our largest projects here at Grafana Labs, and the hashes are now consistent with v0.22.1. Runtime goes from 1.8s to 200ms

Made a stupid mistake in the previous PR: #759

This fixes it and adds another benchmark test to ensure it doesn't happen again.
I also removed the Github Actions benchmark test, as it's not really useful, anytime we change the tests, we'll get erroneous results which will be annoying.
Instead, I added the benchmark tests to the Drone run, we can compare whenever we want.
@julienduchesne julienduchesne marked this pull request as ready for review September 27, 2022 18:54
@julienduchesne julienduchesne requested review from a team, Duologic and sh0rez September 27, 2022 18:54
@julienduchesne julienduchesne merged commit 904a7c5 into main Sep 28, 2022
@julienduchesne julienduchesne deleted the julienduchesne/fix-get-snippet-hash branch September 28, 2022 10:26
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