-
Couldn't load subscription status.
- Fork 612
fix: Don't scan profile events if listener is not set #1686
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
base: main
Are you sure you want to change the base?
Conversation
|
Taking a look at the test failures 👀 |
|
I have some tests failing for me on |
b264936 to
ce6ffb5
Compare
|
Anything missing from the PR or from the issue? Is there another approach I should take? 3x improvement in memory allocations seems quite good. I can add more benchmarks/screenshots if needed |
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.
Pull Request Overview
This PR optimizes memory allocation by skipping the scanning and processing of profile events when no profile event listener is registered. The change addresses a performance issue where profile events were being unnecessarily parsed even when the client wasn't interested in them, resulting in significant memory allocations.
Key changes:
- Modified profile event handling to conditionally scan events only when a listener is set
- Refactored
onProcessinitialization to conditionally set theprofileEventshandler
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/issues/1685_test.go | Adds benchmark test to verify the performance improvement for the profile events optimization |
| context.go | Refactors onProcess() to conditionally assign profileEvents handler only when listener is present |
| conn_profile_events.go | Adds scanEvents parameter to skip event parsing when no listener is registered |
| conn_process.go | Updates profile events handling to pass scan flag and conditionally invoke handler |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if scanEvents { | ||
| on.profileEvents(events) | ||
| } |
Copilot
AI
Oct 25, 2025
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.
The scanEvents check on line 192 is redundant. When scanEvents is false, on.profileEvents is nil (as set in context.go), so calling it would panic. However, since profileEvents() already returns nil when scanEvents is false (line 28 of conn_profile_events.go), this condition will never execute with a nil handler. Remove this check and call on.profileEvents(events) unconditionally, relying on the nil check in context.go.
| if scanEvents { | |
| on.profileEvents(events) | |
| } | |
| on.profileEvents(events) |
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.
this condition will never execute with a nil handler
This is incorrect, if scanEvents is false on.profileEvents is nil and the call will panic (double checked by removing the condition and re-running the test)
Co-authored-by: Copilot <[email protected]>
Summary
Fixes #1685. A better way would be to be able to disable these events altogether from being returned to the client.
Also, if this PR is accepted we might want to do the same for other listeners, though I haven't seen a big issue with allocations in other places (see issue for memory profiles).
For the benchmark in the issue this is over a 3x improvement
Checklist