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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 9 commits into from
Dec 30, 2024
Merged

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

merged 9 commits into from
Dec 30, 2024

Conversation

piyushroshan
Copy link
Contributor

@piyushroshan piyushroshan commented Nov 5, 2024 •
edited
Loading

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 •
edited
Loading

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

maybe something like https://medium.com/@nikolay.dimitrov/comparing-go-performance-of-mutex-sync-map-and-channels-for-syncing-a-map-of-counters-61810c201e17 ?

@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 •
edited
Loading

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

@codecov Codecov
Copy link

codecov bot commented Nov 14, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.69%. Comparing base (842b8f6) to head (5e5a822).
Report is 1 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1197      +/-   ##
==========================================
- Coverage   81.69%   81.69%   -0.01%     
==========================================
  Files         169      169              
  Lines        9770     9767       -3     
==========================================
- Hits         7982     7979       -3     
  Misses       1537     1537              
  Partials      251      251
Flag Coverage Δ
coraza.rule.case_sensitive_args_keys 81.65% <100.00%> (-0.01%) ⬇️
coraza.rule.multiphase_valuation 81.69% <100.00%> (-0.01%) ⬇️
coraza.rule.no_regex_multiline 81.63% <100.00%> (-0.01%) ⬇️
default 81.69% <100.00%> (-0.01%) ⬇️
examples+ 16.55% <0.00%> (+<0.01%) ⬆️
examples+coraza.rule.case_sensitive_args_keys 81.65% <100.00%> (-0.01%) ⬇️
examples+coraza.rule.multiphase_valuation 81.52% <100.00%> (-0.01%) ⬇️
examples+coraza.rule.no_regex_multiline 81.55% <100.00%> (-0.01%) ⬇️
examples+memoize_builders 81.65% <100.00%> (-0.01%) ⬇️
examples+no_fs_access 80.97% <100.00%> (-0.01%) ⬇️
ftw 81.69% <100.00%> (-0.01%) ⬇️
memoize_builders 81.78% <100.00%> (-0.01%) ⬇️
no_fs_access 81.14% <100.00%> (-0.01%) ⬇️
tinygo 81.66% <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.

fzipi
fzipi approved these changes Dec 29, 2024
View reviewed changes
Copy link
Member

@fzipi fzipi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Let's merge this one and cherry pick for v4 (@jptosso)

@piyushroshan piyushroshan merged commit d5a0d6d into main Dec 30, 2024
71 checks passed
@piyushroshan piyushroshan deleted the mutex_to_sync_map branch December 30, 2024 06:10
@jcchavezs
Copy link
Member

The benchmark always fail on my PC (Apple M3), can @fzipi ensure the benchmark runs correctly?

@fzipi fzipi restored the mutex_to_sync_map branch December 30, 2024 11:09
fzipi added a commit that referenced this pull request Dec 30, 2024
@fzipi
Revert "chore: Replace sync.Mutex with sync.Map (#1197)"
a6b435f 
This reverts commit d5a0d6d.
@fzipi fzipi mentioned this pull request Dec 30, 2024
Merged
jcchavezs pushed a commit that referenced this pull request Dec 31, 2024
@fzipi
Revert "chore: Replace sync.Mutex with sync.Map" (#1262)
edad234 
Revert "chore: Replace sync.Mutex with sync.Map (#1197)"

This reverts commit d5a0d6d.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fzipi fzipi fzipi approved these changes

Assignees
No one assigned
Labels
2024-project-summit pending-v4-cherrypick
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@piyushroshan @jcchavezs @jptosso @fzipi