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

sync: improve Map performance with lock free list #54720

Open
alphadose opened this issue Aug 28, 2022 · 11 comments
Open

sync: improve Map performance with lock free list #54720

alphadose opened this issue Aug 28, 2022 · 11 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Milestone

Comments

@alphadose
Copy link

Reference -> https://github.com/alphadose/haxmap

Hashing algorithm used -> https://github.com/Cyan4973/xxHash

Map buckets were implemented using Harris lock-free list

@gopherbot gopherbot added this to the Proposal milestone Aug 28, 2022
@ianlancetaylor
Copy link
Contributor

If there are no API changes, this doesn't need to be a proposal, so taking out of the proposal process.

CC @bcmills

@ianlancetaylor ianlancetaylor changed the title proposal: sync: improve sync.Map performance sync: improve Map performance with lock free list Aug 28, 2022
@ianlancetaylor ianlancetaylor added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. and removed Proposal labels Aug 28, 2022
@ianlancetaylor ianlancetaylor modified the milestones: Proposal, Backlog Aug 28, 2022
@mvdan
Copy link
Member

mvdan commented Aug 29, 2022

Worth noting that this uses the xxhash hashing algorithm, which I believe is not in the standard library right now. Could we use Go's maphash instead, perhaps?

@egonelbre
Copy link
Contributor

egonelbre commented Aug 29, 2022

Looks neat. Although, while testing the implementation, this benchmark doesn't seem to complete:

func BenchmarkHaxMapReadsConcurrentWritesEmpty(b *testing.B) {
	const mapSize = 256
	const epochs = 10000

	m := haxmap.New[uintptr, uintptr](mapSize)
	var writer uintptr
	b.RunParallel(func(pb *testing.PB) {
		if atomic.AddUintptr(&writer, 1)&1 == 1 {
			for pb.Next() {
				for i := uintptr(0); i < epochs; i++ {
					m.Set(i, i)
				}
			}
		} else {
			for pb.Next() {
				for i := uintptr(0); i < epochs; i++ {
					m.Get(i)
				}
			}
		}
	})
}

Running with concurrency 32, amd64. If I change mapSize = 1 in the tests, then other tests also seem to fail.

I also recommend using the same resize algorithm as map uses, otherwise you might be benchmarking parts of the sizing algorithms instead of the lock-free part.

And as @mvdan mentioned, try benchmarking using the same hashing algorithm as std; this shows that the benefit isn't just from replacing the hashing algorithm.

@alphadose
Copy link
Author

alphadose commented Aug 29, 2022

Running with concurrency 32, amd64. If I change mapSize = 1 in the tests, then other tests also seem to fail.

Will look into it

I also recommend using the same resize algorithm as map uses, otherwise you might be benchmarking parts of the sizing algorithms instead of the lock-free part.

Agreed I was also looking to improve the current resizing algorithm of haxmap as well

And as @mvdan mentioned, try benchmarking using the same hashing algorithm as std; this shows that the benefit isn't just from replacing the hashing algorithm.

Got it, I will try benchmarking with golang std hash as well and compare it with the current xxHash based implementation

@alphadose
Copy link
Author

Looks neat. Although, while testing the implementation, this benchmark doesn't seem to complete:

This happens because of small map size and lots of grow operations which run as goroutines. These goroutines cause the slowdown.

One option is handle grow() synchronously with the writer threads

There is an open issue for this as well alphadose/haxmap#2

@egonelbre
Copy link
Contributor

Also, I recommend comparing with #54766

@bcmills
Copy link
Contributor

bcmills commented Aug 31, 2022

Compare #47643 (CC @puzpuzpuz), #21035, #21032.

How would this affect pointer overheads in the steady state? (See #21031.)

@bcmills bcmills added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Aug 31, 2022
@gopherbot gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 31, 2022
@bcmills bcmills added WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Aug 31, 2022
@gopherbot
Copy link

Timed out in state WaitingForInfo. Closing.

(I am just a bot, though. Please speak up if this is a mistake or you have the requested information.)

@alphadose
Copy link
Author

I was fixing most of the issues in haxmap to make it a stable version as in https://github.com/alphadose/haxmap/releases/tag/v1.0.0

Wanted it to work correctly before making any comparisons

Hence there is no need to close this issue, it is being actively worked upon

@mvdan mvdan removed the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Oct 1, 2022
@mvdan
Copy link
Member

mvdan commented Oct 1, 2022

That's fine. Typically we use the bot to close issues which were waiting on a reply for too long because the majority simply become stale and non-actionable.

@mvdan mvdan reopened this Oct 1, 2022
@alphadose
Copy link
Author

@mvdan comparison of xxHash vs golang maphash in the context of haxmap over 20 cases

name                      time/op
XXHashReadsOnly-8         7.18µs ± 6%
XXHashReadsWithWrites-8   8.50µs ± 5%
MapHashReadsOnly-8        9.64µs ± 3%
MapHashReadsWithWrites-8  11.3µs ± 3%

name                      alloc/op
XXHashReadsOnly-8          0.00B
XXHashReadsWithWrites-8   1.24kB ± 4%
MapHashReadsOnly-8         0.00B
MapHashReadsWithWrites-8  1.38kB ± 5%

name                      allocs/op
XXHashReadsOnly-8           0.00
XXHashReadsWithWrites-8      155 ± 4%
MapHashReadsOnly-8          0.00
MapHashReadsWithWrites-8     172 ± 5%

Benchmarking code is available here https://github.com/alphadose/haxmap/blob/hash-comparison/benchmarks/map_test.go, kindly crosscheck to ensure the comparisons are done fairly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Projects
None yet
Development

No branches or pull requests

6 participants