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

Replace util.SyncStore implementation to use singleflight and sync.Map. #5016

Merged
merged 10 commits into from
Nov 13, 2020

Conversation

tejal29
Copy link
Contributor

@tejal29 tejal29 commented Nov 10, 2020

This is follow up of #4896

In #4896, I used a library sync/singleflight to make sure getDependencies results were cached and only computed once for dockerfile builder.
sync/singleflight ensure same function for given input key executes only once across multiple goroutines at a given time. Along with sync.Map, we can make sure at any given time, the function for given input key only executes once across multiple subroutines.
This is ensured by, wrapping the result of the function we want to execute and storing it in a thread safe sync.Map
For cache miss, execute the func and store the result.

During review @gsquared94 mentioned, he added util.Store which uses sync.Once and sync.Map to achieve th same thing.

sync/singleflight is safer for a corner case when same key is looked up exactly at the same point in different subroutines.

Hence replacing the sync.Store implementation with sync/singleflight

No User facing changes

@tejal29 tejal29 requested a review from gsquared94 November 10, 2020 22:57
@tejal29 tejal29 requested a review from a team as a code owner November 10, 2020 22:57
@google-cla google-cla bot added the cla: yes label Nov 10, 2020
@codecov
Copy link

codecov bot commented Nov 10, 2020

Codecov Report

Merging #5016 (5a1c85a) into master (f2879db) will increase coverage by 0.01%.
The diff coverage is 76.92%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5016      +/-   ##
==========================================
+ Coverage   72.15%   72.16%   +0.01%     
==========================================
  Files         365      365              
  Lines       12807    12837      +30     
==========================================
+ Hits         9241     9264      +23     
- Misses       2879     2884       +5     
- Partials      687      689       +2     
Impacted Files Coverage Δ
pkg/skaffold/build/cache/hash.go 71.28% <50.00%> (-2.12%) ⬇️
pkg/skaffold/docker/dependencies.go 73.33% <77.77%> (-1.67%) ⬇️
pkg/skaffold/util/store.go 100.00% <100.00%> (ø)
pkg/skaffold/build/result.go 83.33% <0.00%> (-3.34%) ⬇️
pkg/skaffold/build/bazel/build.go 67.24% <0.00%> (-3.13%) ⬇️
pkg/skaffold/server/server.go 45.36% <0.00%> (ø)
pkg/skaffold/build/jib/maven.go 100.00% <0.00%> (ø)
pkg/skaffold/build/jib/gradle.go 100.00% <0.00%> (ø)
pkg/skaffold/color/formatter.go 100.00% <0.00%> (+10.00%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f2879db...5a1c85a. Read the comment docs.

@tejal29 tejal29 added the kokoro:force-run forces a kokoro re-run on a PR label Nov 11, 2020
@kokoro-team kokoro-team removed the kokoro:force-run forces a kokoro re-run on a PR label Nov 11, 2020
@gsquared94 gsquared94 merged commit e2a4106 into GoogleContainerTools:master Nov 13, 2020
@tejal29 tejal29 deleted the use_one_sf branch April 15, 2021 07:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants