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

Use MarshalJSON directly on notification #201

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

moredure
Copy link
Contributor

@moredure moredure commented Apr 2, 2022

Actually on nil notification it panics before as well, so maybe adding nil check to MarshalJSON or on other function may be good, what do you think?

@coveralls
Copy link

coveralls commented Apr 2, 2022

Coverage Status

Coverage remained the same at 100.0% when pulling 0094dba on moredure:patch-7 into a09d4b5 on sideshow:master.

@moredure moredure force-pushed the patch-7 branch 2 times, most recently from d10cfda to 6efb6af Compare April 2, 2022 19:56
client.go Outdated Show resolved Hide resolved
@moredure moredure force-pushed the patch-7 branch 3 times, most recently from c31e118 to d7242da Compare April 3, 2022 09:46
@moredure moredure changed the title Utilise payloads pooling Use MarshalJSON directly on notification Apr 3, 2022
@moredure moredure requested a review from sideshow April 3, 2022 12:28
@moredure moredure force-pushed the patch-7 branch 2 times, most recently from 5825cff to 7faf20f Compare April 4, 2022 14:40
@moredure
Copy link
Contributor Author

moredure commented Apr 4, 2022

package main_test

import (
	"encoding/json"
	"testing"
)

type S struct {
	X []byte `json:"x"`
}

func (s *S) MarshalJSON() ([]byte, error) {
	return s.X, nil
}

func BenchmarkRun(b *testing.B) {
	s := &S{
		X: []byte("1"),
	}
	b.Run("one", func(b *testing.B) {
		b.ReportAllocs()
		for i := 0; i < b.N; i++ {
			json.Marshal(s)
		}
	})
	b.Run("one", func(b *testing.B) {
		b.ReportAllocs()
		for i := 0; i < b.N; i++ {
			s.MarshalJSON()
		}
	})
}
BenchmarkRun/one
BenchmarkRun/one-4         	 5523985	       197.8 ns/op	       8 B/op	       1 allocs/op
BenchmarkRun/one#01
BenchmarkRun/one#01-4      	1000000000	         0.3392 ns/op	       0 B/op	       0 allocs/op

At least one additional allocation per payload (leaking param)

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.

None yet

3 participants