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

[client][performance] Regression after #446 #568

Open
bsless opened this issue May 21, 2024 · 3 comments · May be fixed by #569
Open

[client][performance] Regression after #446 #568

bsless opened this issue May 21, 2024 · 3 comments · May be fixed by #569

Comments

@bsless
Copy link
Collaborator

bsless commented May 21, 2024

We hit an edge case at work where

keepalives.remove(job.addr.toString() + job.host);

hogged the client's event loop and caused a server to fall over with relatively light load.
Looks like this change is related to #446

The root of the problem is in how PersistentConn implements equals, which is by:

return (addr.toString() + host).equals(obj) || key.equals(obj);

So for each object that checks for equality against a PersistentConn, we do a not insignificant amount of string allocation. Address and host can be compared directly. Same for key.

Possible fix:
edited
Disregard previous suggestion, this can be fixed by implementing equals differently for PersistentConn
The priority queue uses an object's equals. In our case the object can be one of three - PersistentConn, SelectionKey, Request

We can check by instanceof and dispatch to a specialized equality implementation for each case.

wdyt?

@bsless
Copy link
Collaborator Author

bsless commented May 21, 2024

See draft PR for potential solution

@ptaoussanis
Copy link
Member

@bsless Hi Ben, thanks for pinging about this - and apologies for the regression.

To help make this issue easier for me and others to parse (and therefore review or comment on), can you please provide some more context?

Define a class which will hold addr and job and defines equals when they're both equal

For example here- could you please describe what you believe the problem actually is, and why you believe this approach would solve the problem?

Thanks! 🙏

@bsless
Copy link
Collaborator Author

bsless commented May 21, 2024

@ptaoussanis I realized this can be solved by just implementing equals dynamically, edited original issue and the PR contains the proposed solution

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants