-
Notifications
You must be signed in to change notification settings - Fork 7
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
Remove logging string interpolations from hot path #45
Remove logging string interpolations from hot path #45
Conversation
@@ -387,7 +387,8 @@ class RenderFollower extends RenderProxyBox { | |||
return; | |||
} | |||
|
|||
FtlLogs.follower.finest("Follower's LeaderLink reported a change: $_link. Requesting Follower child repaint."); |
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 solution might also add some GC pressure / jank since it will create extra closures that are never used. Maybe we just do:
if (FtlLogs.follower.isLoggable(Level.FINEST)) {
FtlLogs.follower.finest("Follower's LeaderLink reported a change: $_link. Requesting Follower child repaint.");
}
WDYT? Not sure, adds a bit of extra cpu pressure for potential savings on memory pressure (and eventually gc pressure).
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 extra closures have been issue in the element visitor, but I haven't seen it in profile here. There are much less invocations I'd assume.
It won't hurt but I don't think it's worth complicating the code except in tight loops (i.e. _collectTransformForLayerChain
).
@matthew-carroll Ping on the follow the leader performance improvement |
@knopp - Do you know if the widget state properties are now in stable? If so, can you update the handful of analysis errors? |
Seems like it. I'll make another PR. |
I merged the other PR. Please rebase this one. |
d87e2df
to
d7e3920
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
8106677
into
Flutter-Bounty-Hunters:main
No description provided.