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

Concurrent performance improvements; add performance settings field; support IgnoreCollisions #384

Merged
merged 10 commits into from
Feb 15, 2023

Conversation

jmacd
Copy link
Contributor

@jmacd jmacd commented Feb 15, 2023

Description: This streamlines the concurrent access path.

Link to tracking Issue:

Fixes #377.

Documentation: New section in lightstep/sdk/metric/README.md.

Testing: New test added. New benchmarks added, see below. Results are no change for standard configuration, potential 10% improvement with new option.

Performance claim evaluated by raising the Concurrent test load factor:

diff --git a/lightstep/sdk/metric/internal/syncstate/sync_test.go b/lightstep/sdk/metric/internal/syncstate/sync_test.go
index 47abff3..da42016 100644
--- a/lightstep/sdk/metric/internal/syncstate/sync_test.go
+++ b/lightstep/sdk/metric/internal/syncstate/sync_test.go
@@ -111,9 +111,9 @@ func testSyncStateConcurrency[N number.Any, Traits number.Traits[N]](t *testing.
 	// tests are needed.
 	const (
 		numReaders  = 2
-		numRoutines = 10
-		numAttrs    = 10
-		numUpdates  = 1e5
+		numRoutines = 20
+		numAttrs    = 30
+		numUpdates  = 5e6
 	)
 
 	var traits Traits

Performance after:

=== RUN   TestSyncStateDeltaConcurrencyInt
--- PASS: TestSyncStateDeltaConcurrencyInt (2.16s)
=== RUN   TestSyncStateCumulativeConcurrencyInt
--- PASS: TestSyncStateCumulativeConcurrencyInt (3.66s)
=== RUN   TestSyncStateCumulativeConcurrencyIntFiltered
--- PASS: TestSyncStateCumulativeConcurrencyIntFiltered (2.88s)
=== RUN   TestSyncStateDeltaConcurrencyFloat
--- PASS: TestSyncStateDeltaConcurrencyFloat (2.09s)
=== RUN   TestSyncStateCumulativeConcurrencyFloat
--- PASS: TestSyncStateCumulativeConcurrencyFloat (3.68s)
=== RUN   TestSyncStateCumulativeConcurrencyFloatFiltered
--- PASS: TestSyncStateCumulativeConcurrencyFloatFiltered (2.89s)

Performance before shows each test was substantially slower:

=== RUN   TestSyncStateDeltaConcurrencyInt
--- PASS: TestSyncStateDeltaConcurrencyInt (2.87s)
=== RUN   TestSyncStateCumulativeConcurrencyInt
--- PASS: TestSyncStateCumulativeConcurrencyInt (5.86s)
=== RUN   TestSyncStateCumulativeConcurrencyIntFiltered
--- PASS: TestSyncStateCumulativeConcurrencyIntFiltered (4.03s)
=== RUN   TestSyncStateDeltaConcurrencyFloat
--- PASS: TestSyncStateDeltaConcurrencyFloat (3.09s)
=== RUN   TestSyncStateCumulativeConcurrencyFloat
--- PASS: TestSyncStateCumulativeConcurrencyFloat (5.91s)
=== RUN   TestSyncStateCumulativeConcurrencyFloatFiltered
--- PASS: TestSyncStateCumulativeConcurrencyFloatFiltered (3.84s)

Benchmarks after are about the same as below, using the default, safe configuration.

BenchmarkCounterAddNoAttrs-10                   	34563432	        34.50 ns/op	       0 B/op	       0 allocs/op
BenchmarkCounterAddOneAttr-10                   	13396021	        88.49 ns/op	      64 B/op	       1 allocs/op
BenchmarkCounterAddOneInvalidAttr-10            	 8608786	       139.1 ns/op	     128 B/op	       1 allocs/op
BenchmarkCounterAddManyAttrs-10                 	 1000000	      1084 ns/op	     569 B/op	       6 allocs/op
BenchmarkCounterAddManyInvalidAttrs-10          	  770696	      1758 ns/op	    1088 B/op	      10 allocs/op
BenchmarkCounterAddManyFilteredAttrs-10         	 1000000	      1409 ns/op	     954 B/op	       8 allocs/op
BenchmarkCounterCollectOneAttrNoReuse-10        	 2520536	       470.0 ns/op	     400 B/op	       7 allocs/op
BenchmarkCounterCollectOneAttrWithReuse-10      	 3608900	       331.6 ns/op	     136 B/op	       3 allocs/op
BenchmarkCounterCollectTenAttrs-10              	  691329	      1702 ns/op	     712 B/op	      12 allocs/op
BenchmarkCounterCollectTenAttrsTenTimes-10      	   70843	     16864 ns/op	    7128 B/op	     120 allocs/op

New benchmarks use the unsafe code path, indicating about a 10% time improvement vs the corresponding benchmark above. Also note one less allocation which is because when ignoring collisions there is no need to copy the original attribute slice.

BenchmarkCounterAddManyAttrsUnsafe-10           	 1000000	      1003 ns/op	     505 B/op	       5 allocs/op
BenchmarkCounterAddOneAttrUnsafe-10             	15834658	        75.12 ns/op	      64 B/op	       1 allocs/op
BenchmarkCounterAddManyInvalidAttrsUnsafe-10    	  794605	      1719 ns/op	     957 B/op	       9 allocs/op

Benchmarks before:

BenchmarkCounterAddNoAttrs-10                 	35009677	        34.38 ns/op	       0 B/op	       0 allocs/op
BenchmarkCounterAddOneAttr-10                 	13377559	        89.41 ns/op	      64 B/op	       1 allocs/op
BenchmarkCounterAddOneInvalidAttr-10          	 8423766	       142.4 ns/op	     128 B/op	       1 allocs/op
BenchmarkCounterAddManyAttrs-10               	 1000000	      1116 ns/op	     569 B/op	       6 allocs/op
BenchmarkCounterAddManyInvalidAttrs-10        	  779258	      1763 ns/op	    1087 B/op	      10 allocs/op
BenchmarkCounterAddManyFilteredAttrs-10       	 1000000	      1367 ns/op	     953 B/op	       8 allocs/op
BenchmarkCounterCollectOneAttrNoReuse-10      	 2531304	       471.5 ns/op	     400 B/op	       7 allocs/op
BenchmarkCounterCollectOneAttrWithReuse-10    	 3621260	       330.8 ns/op	     136 B/op	       3 allocs/op
BenchmarkCounterCollectTenAttrs-10            	  681168	      1686 ns/op	     712 B/op	      12 allocs/op
BenchmarkCounterCollectTenAttrsTenTimes-10    	   69726	     16827 ns/op	    7128 B/op	     120 allocs/op

@jmacd
Copy link
Contributor Author

jmacd commented Feb 15, 2023

(of the 32-bit build)

panic: unaligned 64-bit atomic operation
	panic: unaligned 64-bit atomic operation

@jmacd jmacd changed the title Add performance settings field; support IgnoreCollisions Concurrent performance improvements; add performance settings field; support IgnoreCollisions Feb 15, 2023
Copy link
Contributor

@jaronoff97 jaronoff97 left a comment

Choose a reason for hiding this comment

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

One nit, one question. Very excited for this!! 🚀

lightstep/sdk/metric/internal/syncstate/sync.go Outdated Show resolved Hide resolved
@jmacd jmacd merged commit d6beb6a into main Feb 15, 2023
@jmacd jmacd deleted the jmacd/ignorecollision branch February 15, 2023 22:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Performance improvements in concurrent update code path
3 participants