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

Replace weighted semaphore with channel #2900

Closed
wants to merge 2 commits into from

Conversation

StephenButtolph
Copy link
Contributor

Why this should be merged

Before:

Benchmark_Semaphore_Acquire-12                	79965123	        15.26 ns/op	       0 B/op	       0 allocs/op
Benchmark_Semaphore_Release-12                	77948438	        15.40 ns/op	       0 B/op	       0 allocs/op
Benchmark_Semaphore_TryAcquire_Success-12     	78838228	        15.01 ns/op	       0 B/op	       0 allocs/op
Benchmark_Semaphore_TryAcquire_Failure-12     	99081432	        11.77 ns/op	       0 B/op	       0 allocs/op

After:

Benchmark_Semaphore_Acquire-12               	56607668	        20.70  ns/op	       0 B/op	       0 allocs/op
Benchmark_Semaphore_Release-12               	55961013	        21.51  ns/op	       0 B/op	       0 allocs/op
Benchmark_Semaphore_TryAcquire_Success-12    	56662573	        21.54  ns/op	       0 B/op	       0 allocs/op
Benchmark_Semaphore_TryAcquire_Failure-12    	318408416	         3.779 ns/op	       0 B/op	       0 allocs/op

Because TryAcquire_Failure is the common case when hashing, we should optimize this path.

How this works

Replaces the semaphore package with a simple wrapper around a channel.

How this was tested

  • New benchmark
  • CI

@StephenButtolph StephenButtolph added this to the v1.11.4 milestone Apr 1, 2024
@StephenButtolph StephenButtolph self-assigned this Apr 1, 2024
@StephenButtolph
Copy link
Contributor Author

We can wait until #2899 is merged to get a better e2e benchmark

@StephenButtolph
Copy link
Contributor Author

StephenButtolph commented Apr 1, 2024

(Running benchmarks on top of #2899, this does seem to be an improvement for larger tries when parallelism is enabled:

TL;DR: 5% improvement for large trie's. 10% regression for small trie's.

Before:

Benchmark_HashChangedNodes/1-12  		 3299204	       358.3 ns/op	     144 B/op	       3 allocs/op
Benchmark_HashChangedNodes/10-12 		   58080	     20863   ns/op	     721 B/op	      25 allocs/op
Benchmark_HashChangedNodes/100-12         	   10000	    114356   ns/op	    2872 B/op	     111 allocs/op
Benchmark_HashChangedNodes/1000-12        	    1384	    912711   ns/op	   17346 B/op	     813 allocs/op
Benchmark_HashChangedNodes/10000-12       	     152	   7474488   ns/op	   91583 B/op	    4331 allocs/op
Benchmark_HashChangedNodes/100000-12      	      14	  79664562   ns/op	 1156965 B/op	   65749 allocs/op

After:

Benchmark_HashChangedNodes/1-12  		 2710024	       400.2 ns/op	     144 B/op	       3 allocs/op
Benchmark_HashChangedNodes/10-12 		   52284	     22970   ns/op	     722 B/op	      25 allocs/op
Benchmark_HashChangedNodes/100-12         	    8569	    139937   ns/op	    2888 B/op	     111 allocs/op
Benchmark_HashChangedNodes/1000-12        	    1370	    926422   ns/op	   15252 B/op	     739 allocs/op
Benchmark_HashChangedNodes/10000-12       	     172	   7068256   ns/op	   89346 B/op	    4253 allocs/op
Benchmark_HashChangedNodes/100000-12      	      14	  75760193   ns/op	 1060367 B/op	   62297 allocs/op

It is a minor performance degradation for smaller tries as more of the TryAcquire calls succeed which is slower with this implementation.

@StephenButtolph StephenButtolph marked this pull request as ready for review April 1, 2024 16:55
@StephenButtolph
Copy link
Contributor Author

This PR is being taken over by the PR to optimize key buffer reuse #2902

@StephenButtolph StephenButtolph deleted the optimize-semaphore branch July 24, 2024 20:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

1 participant