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

Feat: enhance message encode #726

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Conversation

ethan256
Copy link

@ethan256 ethan256 commented May 18, 2024

Using stream reduces memory allocation during message encoding and improves performance.
benchstat:

goos: linux
goarch: amd64
pkg: github.com/gopcua/opcua/uasc
cpu: AMD Ryzen 7 5800H with Radeon Graphics         
                │   old.txt    │               new.txt               │
                │    sec/op    │   sec/op     vs base                │
EncodeMessage-6   12.710µ ± 2%   6.748µ ± 2%  -46.91% (p=0.000 n=10)

                │   old.txt    │               new.txt               │
                │     B/op     │     B/op      vs base               │
EncodeMessage-6   4.375Ki ± 0%   4.492Ki ± 0%  +2.68% (p=0.000 n=10)

                │   old.txt   │              new.txt               │
                │  allocs/op  │ allocs/op   vs base                │
EncodeMessage-6   169.00 ± 0%   87.00 ± 0%  -48.52% (p=0.000 n=10)

@ethan256
Copy link
Author

ethan256 commented May 20, 2024

@magiconair Can you help review it?

@danomagnum
Copy link
Contributor

Is there a technical reason to not go all the way and replace the buffer allocations in all the write functions also?

func (b *Stream) WriteUint16(n uint16) {
	d := make([]byte, 2)
	binary.LittleEndian.PutUint16(d, n)
	b.Write(d)
}

could become

func (b *Stream) WriteUint16(n uint16) {
	b.buf = binary.LittleEndian.AppendUint16(b.buf, n)
}

Just a thought.

@ethan256
Copy link
Author

Is there a technical reason to not go all the way and replace the buffer allocations in all the write functions also?

func (b *Stream) WriteUint16(n uint16) {
	d := make([]byte, 2)
	binary.LittleEndian.PutUint16(d, n)
	b.Write(d)
}

could become

func (b *Stream) WriteUint16(n uint16) {
	b.buf = binary.LittleEndian.AppendUint16(b.buf, n)
}

Just a thought.

I think it's a good idea, I'll give it a try

@kung-foo kung-foo added the enhancement New feature or request label May 30, 2024
Signed-off-by: yuanliang <yuanliang_zh@163.com>
ethan256 added 2 commits June 1, 2024 14:07
Signed-off-by: yuanliang <yuanliang_zh@163.com>
Signed-off-by: yuanliang <yuanliang_zh@163.com>
@ethan256
Copy link
Author

ethan256 commented Jun 2, 2024

commit 855932c benchstats results as follow:

goos: linux
goarch: amd64
pkg: github.com/gopcua/opcua/uasc
cpu: AMD Ryzen 7 5800H with Radeon Graphics         
                │   old.txt    │              codec.txt              │
                │    sec/op    │   sec/op     vs base                │
EncodeMessage-6   12.710µ ± 2%   3.613µ ± 1%  -71.57% (p=0.000 n=10)

                │   old.txt    │              codec.txt               │
                │     B/op     │     B/op      vs base                │
EncodeMessage-6   4.375Ki ± 0%   1.539Ki ± 0%  -64.82% (p=0.000 n=10)

                │   old.txt   │             codec.txt              │
                │  allocs/op  │ allocs/op   vs base                │
EncodeMessage-6   169.00 ± 0%   39.00 ± 0%  -76.92% (p=0.000 n=10)

ethan256 added 5 commits June 2, 2024 19:34
Signed-off-by: yuanliang <yuanliang_zh@163.com>
Signed-off-by: yuanliang <yuanliang_zh@163.com>
Signed-off-by: yuanliang <yuanliang_zh@163.com>
Signed-off-by: yuanliang <yuanliang_zh@163.com>
Signed-off-by: yuanliang <yuanliang_zh@163.com>
@magiconair magiconair added this to the 0.7.0 milestone Dec 3, 2024
@magiconair
Copy link
Member

How does this relate to #729 and sync.Pool? Do we get similar benefits without breaking all signatures?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants