lib/model: Use a single lock #9275
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
I grow weary of the fmut/pmut shenanigans. This consolidates both under one lock; I'm not convinced there are any significant performance differences with this approach since we're literally just protecting map juggling... - The locking goes away when we were already under an appropriate fmut lock. - Where we had fmut.RLock()+pmut.Lock() it gets upgraded to an fmut.Lock(). - Otherwise s/pmut/fmut/. In order to avoid diff noise for an important change I did not do the following cleanups, which will be filed in a PR after this one, if accepted: - Renaming fmut to just mut - Renaming methods that refer to being "PRLocked" and stuff like that. - Removing the no longer relevant deadlock detector.
* main: lib/nat: Fix test build failure (ref syncthing#9010)
AudriusButkevicius
approved these changes
Dec 11, 2023
Member
AudriusButkevicius
left a comment
There was a problem hiding this comment.
We should have done this ages ago, on the other hand, I think we claim things are now stable'ish?
Member
Author
|
Like a canoe, it is perfectly stable for the competent user... |
imsodin
approved these changes
Dec 11, 2023
Member
imsodin
left a comment
There was a problem hiding this comment.
We claim things are stable-ish now, as in it's time to start meddle with model locks again?! :D
calmh
added a commit
that referenced
this pull request
Dec 11, 2023
Cleanup after #9275. This renames `fmut` -> `mut`, removes the deadlock detector and associated plumbing, renames some things from `...PRLocked` to `...RLocked` and similar, and updates comments. Apart from the removal of the deadlock detection machinery, no functional code changes... i.e. almost 100% diff noise, have fun reviewing.
calmh
added a commit
to calmh/syncthing
that referenced
this pull request
Dec 16, 2023
* main: (89 commits) build: Update quic-go (fixes syncthing#9287) lib/model: Only handle relevant folder summaries (kqueue) (fixes syncthing#9183) (syncthing#9288) lib/model: Use a single lock (phase two: cleanup) (syncthing#9276) build(deps): bump actions/setup-go from 4 to 5 (syncthing#9279) lib/model: Use a single lock (syncthing#9275) cmd/syncthing: Better cli stdin handling (ref syncthing#9166) (syncthing#9281) cmd/syncthing: Mostly replace urfave/cli command line parser with alecthomas/kong (syncthing#9166) lib/nat: Fix test build failure (ref syncthing#9010) lib/model: Add pmut locking for DeviceStatistics (fixes syncthing#9274) lib/model: Add pmut locking for DeviceStatistics (fixes syncthing#9274) lib/model: Remove spurious "replacing service" failure event (ref syncthing#9271) lib/model: Remove spurious "replacing service" failure event (ref syncthing#9271) lib/nat, lib/upnp: IPv6 UPnP support (syncthing#9010) gui, man, authors: Update docs, translations, and contributors gui: Show folder/device status on small screens (syncthing#8643) lib/model: Remove runner during folder cleanup (fixes syncthing#9269) (syncthing#9271) build: Update dependencies (syncthing#9265) build: Revert specifics for Go 1.21.4, build using Go 1.21.5 (syncthing#9264) lib/fs: Reduce memory usage in xattrs handling (syncthing#9251) lib/model: Improve LastSeen handling (syncthing#9256) ...
calmh
added a commit
to calmh/syncthing
that referenced
this pull request
Jan 14, 2024
* main: lib/model: Use a single lock (phase two: cleanup) (syncthing#9276) build(deps): bump actions/setup-go from 4 to 5 (syncthing#9279) lib/model: Use a single lock (syncthing#9275) cmd/syncthing: Better cli stdin handling (ref syncthing#9166) (syncthing#9281) cmd/syncthing: Mostly replace urfave/cli command line parser with alecthomas/kong (syncthing#9166) lib/nat: Fix test build failure (ref syncthing#9010) lib/model: Add pmut locking for DeviceStatistics (fixes syncthing#9274) lib/model: Add pmut locking for DeviceStatistics (fixes syncthing#9274) lib/model: Remove spurious "replacing service" failure event (ref syncthing#9271) lib/model: Remove spurious "replacing service" failure event (ref syncthing#9271) lib/nat, lib/upnp: IPv6 UPnP support (syncthing#9010) gui, man, authors: Update docs, translations, and contributors gui: Show folder/device status on small screens (syncthing#8643) lib/model: Remove runner during folder cleanup (fixes syncthing#9269) (syncthing#9271)
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I'm tired of the fmut/pmut shenanigans. This consolidates both under one lock; I'm not convinced there are any significant performance differences with this approach since we're literally just protecting map juggling...
In order to avoid diff noise for an important change I did not do the following cleanups, which will be filed in a PR after this one, if accepted: