-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Only encode & send advisories when there is interest #6341
Conversation
ba52614
to
0f7032f
Compare
server/consumer.go
Outdated
o.acc.mu.RLock() | ||
sl := o.acc.sl | ||
o.acc.mu.RUnlock() | ||
if !sl.HasInterest(subject) && !o.srv.hasGatewayInterest(o.acc.Name, subject) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefer us to hook updates for this somehow vs checking each time. Also if we know o.acc is not nil, can it go back to nil? Meaning safe to reference without a lock?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe not too bad check each time..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've checked and don't think o.acc
can go back to being nil, so I've removed the lock. I don't think the check for interest is expensive enough to worry about for now given that we now have HasInterest
which is non-allocating.
@@ -1605,13 +1623,8 @@ func (o *consumer) sendDeleteAdvisoryLocked() { | |||
Domain: o.srv.getOpts().JetStreamDomain, | |||
} | |||
|
|||
j, err := json.Marshal(e) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I did this here on purpose..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure it matters where the marshaling takes place, but if we can avoid doing it until after the interest check, that saves us some CPU time & allocs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should do it after the check for sure. But I need to recall why I put it there? Maybe use more cores?
Signed-off-by: Neil Twigg <[email protected]>
0f7032f
to
48c3c47
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This stops us from spending time & CPU cycles encoding advisory JSONs when there is no one listening for them.
Signed-off-by: Neil Twigg [email protected]