Skip to content

perf: use reflect to test equity of message headers#1814

Closed
Aden-Q wants to merge 1 commit intonats-io:mainfrom
Aden-Q:main
Closed

perf: use reflect to test equity of message headers#1814
Aden-Q wants to merge 1 commit intonats-io:mainfrom
Aden-Q:main

Conversation

@Aden-Q
Copy link

@Aden-Q Aden-Q commented Mar 8, 2025

This patch uses reflection to test equality of message headers. This apporach is more efficient in the case when:

  • Both m.Header and msg.Header are non-nil
  • m.Header and msg.Header share the same underlying object, so the equality can be caught before testing the content of the map, compared to the original approach. For reference, map reflect implementation in go source code:
	case Map:
		if v1.IsNil() != v2.IsNil() {
			return false
		}
		if v1.Len() != v2.Len() {
			return false
		}
		if v1.UnsafePointer() == v2.UnsafePointer() {
			return true
		}
		iter := v1.MapRange()
		for iter.Next() {
			val1 := iter.Value()
			val2 := v2.MapIndex(iter.Key())
			if !val1.IsValid() || !val2.IsValid() || !deepValueEqual(val1, val2, visited) {
				return false
			}
		}
		return true

@piotrpio
Copy link
Collaborator

piotrpio commented Apr 1, 2025

Hello @Aden-Q - thanks for the contribution, but I am not sure about this - using reflection has its inherent cost, and while it's true that e.g. if we're comparing the same underlying object it may be faster, for more standard comparison (different header contents), it has worse performance.

Take this benchmark:

func BenchmarkMsgEqual(b *testing.B) {
	b.StopTimer()
	s := RunDefaultServer()
	defer s.Shutdown()
	nc := NewDefaultConnection(b)
	defer nc.Close()
	m1 := &nats.Msg{
		Subject: "foo",
		Data:    []byte("Hello World"),
		Header: nats.Header{
			"foo": []string{"bar"},
		},
	}
	m2 := &nats.Msg{
		Subject: "foo",
		Data:    []byte("Hello World"),
		Header: nats.Header{
			"bar": []string{"baz", "quxx"},
		},
	}
	b.StartTimer()
	b.ReportAllocs()
	for i := 0; i < b.N; i++ {
		m1.Equal(m2)
	}
}

These are the results with the current implementation (no reflection):

goos: darwin
goarch: arm64
pkg: github.com/nats-io/nats.go/test
cpu: Apple M1 Pro
BenchmarkMsgEqual
BenchmarkMsgEqual-10    	31758997	        38.86 ns/op	       0 B/op	       0 allocs/op

And with your changes (using reflection):

goos: darwin
goarch: arm64
pkg: github.com/nats-io/nats.go/test
cpu: Apple M1 Pro
BenchmarkMsgEqual
BenchmarkMsgEqual-10    	 7698056	       156.5 ns/op	      40 B/op	       2 allocs/op

Also note that using reflection we also add allocations.

@Aden-Q
Copy link
Author

Aden-Q commented Apr 1, 2025

Hi @piotrpio, thanks for taking a look and benchmarking it. Agreed that reflection is a less efficient fallback for complex data structure due to reflection overhead. While the current implementation performs better. Closing this. Thanks for taking a look!

@Aden-Q Aden-Q closed this Apr 1, 2025
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.

2 participants