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.

Sign up for GitHub

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

Jump to bottom

[WIP][FIXED] Fix request/reply performance when using allow_responses perms #6064

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

[WIP][FIXED] Fix request/reply performance when using allow_responses perms #6064

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
48c5cac
modified reply map pruning logic to prune every x messages or after t…
jack7803m Oct 31, 2024
77db464
removed unnecessary time.Now() call
jack7803m Nov 1, 2024
4f15d56
only track reply subject perms if the client is not already allowed t…
jack7803m Nov 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion server/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,6 +262,9 @@ type client struct {
last time.Time
lastIn time.Time

repliesSincePrune uint16
lastReplyPrune time.Time

headers bool

rtt time.Duration
Expand Down Expand Up @@ -422,6 +425,7 @@ const (
pruneSize = 32
routeTargetInit = 8
replyPermLimit = 4096
replyPruneTime = time.Second
)

// Represent read cache booleans with a bitmask
Expand Down Expand Up @@ -3636,7 +3640,8 @@ func (c *client) deliverMsg(prodIsMQTT bool, sub *subscription, acc *Account, su
// do that accounting here. We only look at client.replies which will be non-nil.
if client.replies != nil && len(reply) > 0 {
client.replies[string(reply)] = &resp{time.Now(), 0}
if len(client.replies) > replyPermLimit {
client.repliesSincePrune++
if client.repliesSincePrune > replyPermLimit || time.Since(client.lastReplyPrune) > replyPruneTime {
Copy link
Member

Choose a reason for hiding this comment

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

Do we have a sense under heaby load how much more memory this will hold onto?

Copy link
Author

Choose a reason for hiding this comment

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

The original issue was it holding onto too much memory and looping through all of it every message.

Just added some debug statements and found that if the reply subject is already allowed (eg. "pub": ">"), then the reply counter never actually goes up and therefore it never is able to prune that subject out until it expires by time. Just noting this because I'm going to be looking at a fix for that too but not sure if it'll have broader effects (hopefully not).

Assuming that the subjects are getting pruned as they're replied to, at most it should only be able to get the replies map to an extra replyPermLimit size than what it could've possible been before in the worst case scenario. Even for that to happen it would have to fill the map with subjects, attempt to prune, then expire them all by the next message - in that case, pruning for every message over the replyPermLimit would cause it to immediately prune whereas this solution would hold onto that memory for the next replyPermLimit messages, making the map size replyPermLimit * 2.

Under normal heavy load it shouldn't make any significant difference, as the current behavior typically should only run the prune once every replyPermLimit messages anyway when it's configured properly.

client.pruneReplyPerms()
}
}
Expand Down Expand Up @@ -3760,6 +3765,9 @@ func (c *client) pruneReplyPerms() {
delete(c.replies, k)
}
}

c.repliesSincePrune = 0
c.lastReplyPrune = time.Now()
Copy link
Member

Choose a reason for hiding this comment

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

Can just now from above.

Copy link
Author

Choose a reason for hiding this comment

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

Good point! Just fixed it

}

// pruneDenyCache will prune the deny cache via randomly
Expand Down