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

chore: Replace sync.Mutex with sync.Map #1197

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

piyushroshan
Copy link
Contributor

@piyushroshan piyushroshan commented Nov 5, 2024

Sync.Map provides a key level locking to allow concurrent key writes.

Thank you for contributing to Coraza WAF, your effort is greatly appreciated
Before submitting check if what you want to add to coraza list meets quality standards before sending pull request. Thanks!

Make sure that you've checked the boxes below before you submit PR:

Thanks for your contribution ❤️

Benchmarks based on mutex vs sync.Map
sync.Mutex

goos: darwin
goarch: arm64
pkg: github.com/corazawaf/coraza/v3/internal/corazawaf
cpu: Apple M1 Pro
BenchmarkAddTransformationUnique-10        38994            282718 ns/op          766436 B/op          5 allocs/op
BenchmarkAddTransformationSame-10         777428              9162 ns/op             242 B/op          5 allocs/op
PASS
ok      github.com/corazawaf/coraza/v3/internal/corazawaf       19.642s

sync.Map

goos: darwin
goarch: arm64
pkg: github.com/corazawaf/coraza/v3/internal/corazawaf
cpu: Apple M1 Pro
BenchmarkAddTransformationUnique-10        32040            220037 ns/op          627028 B/op          9 allocs/op
BenchmarkAddTransformationSame-10         846592              6618 ns/op             366 B/op          9 allocs/op
PASS
ok      github.com/corazawaf/coraza/v3/internal/corazawaf       13.973s

@piyushroshan piyushroshan requested a review from a team as a code owner November 5, 2024 15:32
@jcchavezs
Copy link
Member

I think this might perform slightly better but I wonder if we could write a benchmark

@jptosso
Copy link
Member

jptosso commented Nov 5, 2024

IMO it's a hard case to benchmark bc it requires a lot of concurrency and we cannot use gh actions for it
A sync mutex blocks read and write, and sync map just blocks at key level

@fzipi
Copy link
Member

fzipi commented Nov 5, 2024

Another option is to use sync.RWMutex and use RLock for the ones which are read-only.

@jcchavezs
Copy link
Member

jcchavezs commented Nov 6, 2024

To be honest without evidence this being anyhow better I would not proceed with the change. sync.RWMutext an easier change to land.

@piyushroshan
Copy link
Contributor Author

To be honest without evidence this being anyhow better I would not proceed with the change. sync.RWMutext an easier change to land.

Added the benchmark

@fzipi
Copy link
Member

fzipi commented Nov 8, 2024

So the benchmark shows this is indeed faster for the tests. The question remains about what happens with real concurrency, but I think it will only behave better because there is no full blocking. @jcchavezs Do you want to make benchmakrs with sync.RWMutex and push them here also to compare? 🙏

@fzipi
Copy link
Member

fzipi commented Nov 10, 2024

ping @jcchavezs

@jcchavezs
Copy link
Member

I will do that tomorrow.

@jptosso
Copy link
Member

jptosso commented Nov 14, 2024

LGTM

Copy link

codecov bot commented Nov 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.63%. Comparing base (a5cc4f0) to head (a79c5ae).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1197      +/-   ##
==========================================
- Coverage   81.64%   81.63%   -0.01%     
==========================================
  Files         168      168              
  Lines        9637     9634       -3     
==========================================
- Hits         7868     7865       -3     
  Misses       1519     1519              
  Partials      250      250              
Flag Coverage Δ
coraza.rule.case_sensitive_args_keys 81.59% <100.00%> (-0.01%) ⬇️
coraza.rule.multiphase_valuation 81.63% <100.00%> (-0.01%) ⬇️
default 81.63% <100.00%> (-0.01%) ⬇️
examples+ 16.44% <0.00%> (+<0.01%) ⬆️
examples+coraza.rule.case_sensitive_args_keys 81.59% <100.00%> (-0.01%) ⬇️
examples+coraza.rule.multiphase_valuation 81.52% <100.00%> (-0.01%) ⬇️
examples+memoize_builders 81.64% <100.00%> (-0.01%) ⬇️
examples+no_fs_access 80.96% <100.00%> (-0.01%) ⬇️
ftw 81.63% <100.00%> (-0.01%) ⬇️
memoize_builders 81.73% <100.00%> (-0.01%) ⬇️
no_fs_access 81.07% <100.00%> (-0.01%) ⬇️
tinygo 81.60% <100.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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.

4 participants