-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Move event traces to detailed_trace! #7732
Conversation
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 generally seems like a good idea to me - this looks to be deeply engine internals logging that a user won't need to see (and they can always enable the feature flag if they must), and the EventReader seems like the kind of thing that will end up in a hot loop with many iterations.
This may have an impact outside of just user-facing events. Since #7713, this also impacts component removal: |
When setting up the benchmarks for #8251, I found nearly 5x improvements to event sending. Didn't impact event iteration.
|
Yikes (or yay?) 5x improvement! |
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.
Makes sense, and appreciate the benchmarks.
Objective
Noticed while writing #7728 that we are using
trace!
logs in our event functions. This has shown to have significant overhead, even trace level logs are disabled globally, as seen in #7639.Solution
Use the
detailed_trace!
macro introduced in #7639. Also removed theevent_trace
function that was only used in one location.Changelog
Changed: Event trace logs are now feature gated behind the
detailed-trace
feature.