Add SyncMap package and use it for graph stop/remove #25311
Conversation
openshift-ci
bot
added
release-note-none
approved
pkg/syncmap/syncmap.go
Outdated
| delete(m.data, key) | ||
| } | ||
|
|
||
| // ToMap returns a shallow copy of the underlying data of the SyncMap. |
There was a problem hiding this comment.
Do we want to actually clone that here? You did not clone them before so cloneing them adds some overhead, sure not enough to notice but still unnecessary. I guess that is my way of complainging that we cannot do this like you would in rust where that caller would have to take ownership of it and it would be clear that the syncmap is not used afterwards...
There was a problem hiding this comment.
I'll add a separate method to get the underlying map for perf reasons, with a caveat that doing so breaks all guarantees of thread safety. Doesn't matter for the way we're using it, where it is only ever accessed by a single thread after a certain point.
pkg/syncmap/syncmap.go
Outdated
| // performance. | ||
| // SyncMap should always be passed by reference, not by value, to ensure thread | ||
| // safety is maintained. | ||
| type SyncMap[K comparable, V any] struct { |
There was a problem hiding this comment.
oh one other thing syncmap.SyncMap kinda sucks as type name, maybe we just name it Map so it is more similar to sync.Map?
pkg/syncmap/syncmap.go
Outdated
| // not here; thus, SyncMap should not be used in truly performance sensitive | ||
| // areas, but places where code cleanliness is more important than raw | ||
| // performance. | ||
| // SyncMap should always be passed by reference, not by value, to ensure thread |
This greatly simplifies the locking around these two functions, and things end up looking a lot more elegant. This should prevent the race flakes we were seeing before. Fixes containers#25289 Signed-off-by: Matt Heon <mheon@redhat.com>
|
@containers/podman-maintainers PTAL and merge |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: giuseppe, Luap99, mheon The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This greatly simplifies the locking around these two functions, and things end up looking a lot more elegant. This should prevent the race flakes we were seeing before.
Fixes #25289
First time I've worked with Go generics - nice to finally have the ability to do generic data structures.
Does this PR introduce a user-facing change?