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

[CHANGED] avoid unnecessary data copy in processMsg #788

Merged
merged 2 commits into from
Aug 9, 2021
Merged

[CHANGED] avoid unnecessary data copy in processMsg #788

merged 2 commits into from
Aug 9, 2021

Conversation

ninjken
Copy link
Contributor

@ninjken ninjken commented Aug 5, 2021

This PR intends to address the github issue

may avoid unnecessary data copy in processMsg for large message payloads #787

I've also tweaked the benchmark test in file test/bench_test.go for local testing, but not included it in the PR because I am not sure of its necessity.

func BenchmarkPubSubSpeed(b *testing.B)
...
    //msg := []byte("Hello World")
    msg := []byte(strings.Repeat("Hello", 1000))

Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

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

LGTM but small comment if we should reset this boolean after the other call to processMsg() (although I believe it is not needed, but same for nc.ps.msg.Buf which is clearly already nil, yet we reset).

@@ -167,7 +168,7 @@ func (nc *Conn) parse(buf []byte) error {
if nc.ps.msgBuf != nil {
if len(nc.ps.msgBuf) >= nc.ps.ma.size {
nc.processMsg(nc.ps.msgBuf)
nc.ps.argBuf, nc.ps.msgBuf, nc.ps.state = nil, nil, MSG_END
nc.ps.argBuf, nc.ps.msgBuf, nc.ps.msgCopied, nc.ps.state = nil, nil, false, MSG_END
Copy link
Member

Choose a reason for hiding this comment

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

For consistency, I wonder if we should also reset this boolean here: https://github.com/nats-io/nats.go/pull/788/files#diff-83eb8e32639d01cf443d6d8bde24c1c8be78766090d8c5f8586c36250cfedca6R194
I agree that at this location it means that the message fit in the current read buffer, so it is clearly not copied, so this boolean should not be true..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At first I thought it was not needed, but I think I agree with you we should reset the flag as well for code consistency.
PR updated :-)

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 86.933% when pulling 9b7c017 on Endeavourken:master into 91bdffe on nats-io:master.

Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you for the contribution!

@kozlovic kozlovic merged commit 6856f4a into nats-io:master Aug 9, 2021
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.

3 participants