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

proposal: runtime: maps: add optional concurrency #63513

Closed
metux opened this issue Oct 12, 2023 · 6 comments
Closed

proposal: runtime: maps: add optional concurrency #63513

metux opened this issue Oct 12, 2023 · 6 comments
Labels
Proposal WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Milestone

Comments

@metux
Copy link

metux commented Oct 12, 2023

The current map implementation currently does not support concurrency - just detects concurrent calls and panics. This is very unfortunate for a language desinged for parrallel computing. Concurrent code cannot use maps directly - it needs to do an own implementation, at least by some wrapper. This drastically devalues the whole existance of the builtin map type. Thefore builtin maps should support concurrency.

Proposal: add rwlock to hmap

Simple and straightforward implementation can just take the lock on the entry of the (user-facing) functions (mapaccess et al). A more sophisticated one can do it on indivual pieces (eg per bucket), eg. skipping if just an existing entry is updated, and take the hmap lock only if necessary.

Performance & backwards compat:

Locking has some (little) performance impact. It would mostly impact concurrent reads, since concurrent writes are currently forbidden anyways.

If this really is a concern, we can add a global bit to enable locking - if unset, the locking is skipped entirely (checking a single bit shouldn't introduce any measurable penalty). Any package that wants it, just sets this bit - applications w/o such packages wont have locking at all, thus no performance impact for existing code.
This global bit could be set eg by some (inlinable) runtime.MapsEnableLocking().

@metux metux added the Proposal label Oct 12, 2023
@gopherbot gopherbot added this to the Proposal milestone Oct 12, 2023
@mvdan
Copy link
Member

mvdan commented Oct 12, 2023

Have you seen https://pkg.go.dev/sync#Map and #47657?

@mvdan
Copy link
Member

mvdan commented Oct 12, 2023

Also, since this looks like a proposal to change the map type as defined in the spec, please note that you should fill https://github.com/golang/proposal/blob/master/go2-language-changes.md when proposing a language change.

@mvdan mvdan added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Oct 12, 2023
@metux
Copy link
Author

metux commented Oct 12, 2023

Have you seen https://pkg.go.dev/sync#Map and #47657?

yes, of course :)
but unfortunately, this (and all the other similar implementations out there) doesn't solve the actual problem:

  • can't be used as a map (eg. index, range, ...)
  • complicated to integrate with existing code thats already based on maps (possibly nested ones, ...)

@metux
Copy link
Author

metux commented Oct 12, 2023

Also, since this looks like a proposal to change the map type as defined in the spec, please note that you should fill https://github.com/golang/proposal/blob/master/go2-language-changes.md when proposing a language change.

My goal was doing it w/o changing the language itself, just the map piece of the standard library.
(If I understand it correctly, map is nothing more than a struct having pointer to hmap with some syntactic sugar for convenient access.)

@seankhliao
Copy link
Member

Duplicate of #1682

@seankhliao seankhliao marked this as a duplicate of #1682 Oct 12, 2023
@seankhliao seankhliao closed this as not planned Won't fix, can't repro, duplicate, stale Oct 12, 2023
@mvdan
Copy link
Member

mvdan commented Oct 12, 2023

(If I understand it correctly, map is nothing more than a struct having pointer to hmap with some syntactic sugar for convenient access.)

Maps are defined in the spec, along with all the operations that can be done on them: https://go.dev/ref/spec#Map_types

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Proposal WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Projects
None yet
Development

No branches or pull requests

4 participants