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

MapOf: Generic Keys #46

Closed
wants to merge 2 commits into from
Closed

Conversation

iamcalledrob
Copy link
Contributor

Support for generic keys, and an accompanying hasher func, as discussed in #17

This shouldn't be a breaking change for function calls, but since the MapOf type has changed from
type MapOf[V any] struct to type MapOf[K, V any] struct, this isn't a fully backwards-compatible change.

Usage:

// Existing string-keyed API is unchanged
m := NewMapOf[Widget]()

// For arbitrary key types
hasher := func(k User) uint64 { ... }
m := NewTypedMapOf[User, Widget](hasher)
m.Store(User{}, Widget{})

// UInt64 hasher is noop
hasher := func(k uint64) uint64 { return k }
m := NewTypedMapOf[UInt64, Widget](hasher)
m.Store(1234, Widget{})
Benchmarks are unchanged
name                              old time/op  new time/op  delta
Counter-8                         4.35ns ± 0%  2.35ns ± 0%   ~     (p=1.000 n=1+1)
AtomicInt64-8                     65.8ns ± 0%  66.9ns ± 0%   ~     (p=1.000 n=1+1)
Map_NoWarmUp/99%-reads-8          28.8ns ± 0%  28.5ns ± 0%   ~     (p=1.000 n=1+1)
Map_NoWarmUp/90%-reads-8          42.4ns ± 0%  42.5ns ± 0%   ~     (p=1.000 n=1+1)
Map_NoWarmUp/75%-reads-8          56.1ns ± 0%  47.5ns ± 0%   ~     (p=1.000 n=1+1)
Map_NoWarmUp/50%-reads-8          85.3ns ± 0%  57.5ns ± 0%   ~     (p=1.000 n=1+1)
Map_NoWarmUp/0%-reads-8           83.2ns ± 0%  73.1ns ± 0%   ~     (p=1.000 n=1+1)
MapStandard_NoWarmUp/99%-reads-8   386ns ± 0%   346ns ± 0%   ~     (p=1.000 n=1+1)
MapStandard_NoWarmUp/90%-reads-8   452ns ± 0%   430ns ± 0%   ~     (p=1.000 n=1+1)
MapStandard_NoWarmUp/75%-reads-8   532ns ± 0%   426ns ± 0%   ~     (p=1.000 n=1+1)
MapStandard_NoWarmUp/50%-reads-8   418ns ± 0%   449ns ± 0%   ~     (p=1.000 n=1+1)
MapStandard_NoWarmUp/0%-reads-8    496ns ± 0%   464ns ± 0%   ~     (p=1.000 n=1+1)
Map_WarmUp/100%-reads-8           49.5ns ± 0%  48.6ns ± 0%   ~     (p=1.000 n=1+1)
Map_WarmUp/99%-reads-8            51.6ns ± 0%  47.8ns ± 0%   ~     (p=1.000 n=1+1)
Map_WarmUp/90%-reads-8            48.0ns ± 0%  46.8ns ± 0%   ~     (p=1.000 n=1+1)
Map_WarmUp/75%-reads-8            51.1ns ± 0%  48.3ns ± 0%   ~     (p=1.000 n=1+1)
Map_WarmUp/50%-reads-8            59.4ns ± 0%  53.6ns ± 0%   ~     (p=1.000 n=1+1)
Map_WarmUp/0%-reads-8             68.8ns ± 0%  66.5ns ± 0%   ~     (p=1.000 n=1+1)
MapStandard_WarmUp/100%-reads-8    126ns ± 0%   111ns ± 0%   ~     (p=1.000 n=1+1)
MapStandard_WarmUp/99%-reads-8     240ns ± 0%   162ns ± 0%   ~     (p=1.000 n=1+1)
MapStandard_WarmUp/90%-reads-8     214ns ± 0%   199ns ± 0%   ~     (p=1.000 n=1+1)
MapStandard_WarmUp/75%-reads-8     294ns ± 0%   283ns ± 0%   ~     (p=1.000 n=1+1)
MapStandard_WarmUp/50%-reads-8     442ns ± 0%   443ns ± 0%   ~     (p=1.000 n=1+1)
MapStandard_WarmUp/0%-reads-8      616ns ± 0%   614ns ± 0%   ~     (p=1.000 n=1+1)
MapRange-8                        6.35ms ± 0%  6.34ms ± 0%   ~     (p=1.000 n=1+1)
MapRangeStandard-8                6.11ms ± 0%  6.06ms ± 0%   ~     (p=1.000 n=1+1)
MapOf_NoWarmUp/99%-reads-8        28.3ns ± 0%  28.9ns ± 0%   ~     (p=1.000 n=1+1)
MapOf_NoWarmUp/90%-reads-8        39.1ns ± 0%  41.6ns ± 0%   ~     (p=1.000 n=1+1)
MapOf_NoWarmUp/75%-reads-8        47.0ns ± 0%  48.2ns ± 0%   ~     (p=1.000 n=1+1)
MapOf_NoWarmUp/50%-reads-8        63.6ns ± 0%  62.9ns ± 0%   ~     (p=1.000 n=1+1)
MapOf_NoWarmUp/0%-reads-8         76.5ns ± 0%  71.8ns ± 0%   ~     (p=1.000 n=1+1)
MapOf_WarmUp/100%-reads-8         50.9ns ± 0%  51.1ns ± 0%   ~     (p=1.000 n=1+1)
MapOf_WarmUp/99%-reads-8          49.7ns ± 0%  52.9ns ± 0%   ~     (p=1.000 n=1+1)
MapOf_WarmUp/90%-reads-8          59.6ns ± 0%  51.0ns ± 0%   ~     (p=1.000 n=1+1)
MapOf_WarmUp/75%-reads-8          55.7ns ± 0%  51.9ns ± 0%   ~     (p=1.000 n=1+1)
MapOf_WarmUp/50%-reads-8          56.4ns ± 0%  56.2ns ± 0%   ~     (p=1.000 n=1+1)
MapOf_WarmUp/0%-reads-8           69.2ns ± 0%  67.3ns ± 0%   ~     (p=1.000 n=1+1)
MapOfRange-8                      7.83ms ± 0%  7.25ms ± 0%   ~     (p=1.000 n=1+1)
QueueProdCons-8                    164ns ± 0%   172ns ± 0%   ~     (p=1.000 n=1+1)
QueueProdConsWork100-8             191ns ± 0%   181ns ± 0%   ~     (p=1.000 n=1+1)
ChanProdCons-8                    74.1ns ± 0%  75.4ns ± 0%   ~     (p=1.000 n=1+1)
ChanProdConsWork100-8              419ns ± 0%   402ns ± 0%   ~     (p=1.000 n=1+1)
RBMutexReadOnly-8                 2.45ns ± 0%  2.42ns ± 0%   ~     (p=1.000 n=1+1)
RBMutexWrite10000-8               8.44ns ± 0%  9.57ns ± 0%   ~     (p=1.000 n=1+1)
RBMutexWrite1000-8                25.6ns ± 0%  25.3ns ± 0%   ~     (p=1.000 n=1+1)
RBMutexWrite100-8                 60.1ns ± 0%  58.9ns ± 0%   ~     (p=1.000 n=1+1)
RBMutexWorkReadOnly-8             21.4ns ± 0%  20.0ns ± 0%   ~     (p=1.000 n=1+1)
RBMutexWorkWrite10000-8           30.1ns ± 0%  31.3ns ± 0%   ~     (p=1.000 n=1+1)
RBMutexWorkWrite1000-8            60.5ns ± 0%  58.7ns ± 0%   ~     (p=1.000 n=1+1)
RBMutexWorkWrite100-8              125ns ± 0%   125ns ± 0%   ~     (p=1.000 n=1+1)
RWMutexReadOnly-8                  127ns ± 0%   139ns ± 0%   ~     (p=1.000 n=1+1)
RWMutexWrite10000-8                128ns ± 0%   123ns ± 0%   ~     (p=1.000 n=1+1)
RWMutexWrite1000-8                84.7ns ± 0%  83.0ns ± 0%   ~     (p=1.000 n=1+1)
RWMutexWrite100-8                 44.0ns ± 0%  43.1ns ± 0%   ~     (p=1.000 n=1+1)
RWMutexWorkReadOnly-8              150ns ± 0%   152ns ± 0%   ~     (p=1.000 n=1+1)
RWMutexWorkWrite10000-8            151ns ± 0%   142ns ± 0%   ~     (p=1.000 n=1+1)
RWMutexWorkWrite1000-8             132ns ± 0%   123ns ± 0%   ~     (p=1.000 n=1+1)
RWMutexWorkWrite100-8              103ns ± 0%    97ns ± 0%   ~     (p=1.000 n=1+1)

Copy link
Owner

@puzpuzpuz puzpuzpuz left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution. I'd like to address the second consideration from #17 (comment) before merging this one. I'm going to address it, but probably not in the nearest term. Will post an update here once it's done.

mapof.go Outdated Show resolved Hide resolved
@iamcalledrob
Copy link
Contributor Author

Thanks for the contribution. I'd like to address the second consideration from #17 (comment) before merging this one. I'm going to address it, but probably not in the nearest term. Will post an update here once it's done.

No worries, just throwing this up for discussion. No pressure to merge :)

@iamcalledrob
Copy link
Contributor Author

It's worth noting that I'm seeing VERY poor performance for certain keys. This could be an issue I've introduced here, or it could be that the string hashes were just unlikely to generate problematic keys.

Here's an example that's very problematic on my machine (Apple M1 Pro):

// Benchmark32768000000/xsync.Map-8         	     142	  20367105 ns/op
// Benchmark32768000000/sync.Map-8          	 4306820	       385.9 ns/op
func Benchmark32768000000(b *testing.B) {
	b.Run("xsync.Map", func(b *testing.B) {
		m := NewIntegerMapOf[uint64, int]()
		for i := 0; i < b.N; i++ {
			m.Store(uint64(i*32768000000), 0)
		}
	})
	b.Run("sync.Map", func(b *testing.B) {
		m := sync.Map{}
		for i := 0; i < b.N; i++ {
			m.Store(uint64(i*32768000000), 0)
		}
	})
}

// Benchmark32768000001/xsync.Map-8         	21440982	        48.54 ns/op
// Benchmark32768000001/sync.Map-8          	 4475781	       385.2 ns/op
func Benchmark32768000001(b *testing.B) {
	b.Run("xsync.Map", func(b *testing.B) {
		m := NewIntegerMapOf[uint64, int]()
		for i := 0; i < b.N; i++ {
			m.Store(uint64(i*32768000001), 0)
		}
	})
	b.Run("sync.Map", func(b *testing.B) {
		m := sync.Map{}
		for i := 0; i < b.N; i++ {
			m.Store(uint64(i*32768000001), 0)
		}
	})
}

@puzpuzpuz
Copy link
Owner

I'd like to address the second consideration from #17 (comment) before merging this one. I'm going to address it, but probably not in the nearest term. Will post an update here once it's done.

@iamcalledrob I've just merged #48, so this PR should be unblocked at last. Sorry for taking that long. 🥲

Let me know if you're willing to give another try with this PR, otherwise I'll pick it up myself.

@puzpuzpuz
Copy link
Owner

puzpuzpuz commented Oct 15, 2022

It's worth noting that I'm seeing VERY poor performance for certain keys.

This is very likely due to hash collisions and #48 should make this problem less significant.

The problem with 32768000000 * i values is that all of them will have lots of zeros in LSBs (least significant bits), so they were ending up in the same bucket leading to lots of map resize operations.

The proper way to get rid of the problem is to apply hash64() (murmurhash3 64-bit finalizer) function to hash function result. This will mix LSBs and MSBs, so that keys end up in different buckets, as they should.

@puzpuzpuz puzpuzpuz added the enhancement New feature or request label Oct 22, 2022
@puzpuzpuz
Copy link
Owner

Closing this one in favor of #49 to speed up things.

@iamcalledrob many thanks for your contribution!

@puzpuzpuz puzpuzpuz closed this Oct 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants