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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Oct 17, 2024
Merged

Remove logging string interpolations from hot path #45

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d2b5375
Remove logging string interpolations from hot path
knopp Oct 16, 2024
60baa58
reformat
knopp Oct 16, 2024
d7e3920
More
knopp Oct 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 28 additions & 27 deletions lib/src/follower.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -387,7 +387,8 @@ class RenderFollower extends RenderProxyBox {
return;
}

FtlLogs.follower.finest("Follower's LeaderLink reported a change: $_link. Requesting Follower child repaint.");
Copy link

@brian-superlist brian-superlist Oct 16, 2024 •
edited
Loading

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).

Copy link
Contributor Author

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).

FtlLogs.follower
.finest(() => "Follower's LeaderLink reported a change: $_link. Requesting Follower child repaint.");
child?.markNeedsPaint();
}

Expand Down Expand Up @@ -505,13 +506,13 @@ class RenderFollower extends RenderProxyBox {
} else {
size = child!.size;
}
FtlLogs.follower.finer(" - Follower bounds layout size: $size");
FtlLogs.follower.finer(" - Follower content size: ${child?.size}");
FtlLogs.follower.finer(() => " - Follower bounds layout size: $size");
FtlLogs.follower.finer(() => " - Follower content size: ${child?.size}");
}

@override
void paint(PaintingContext context, Offset offset) {
FtlLogs.follower.finer("Painting Follower - paint offset: $offset");
FtlLogs.follower.finer(() => "Painting Follower - paint offset: $offset");
if (child == null) {
return;
}
Expand All @@ -537,14 +538,14 @@ class RenderFollower extends RenderProxyBox {
return;
}

FtlLogs.follower
.finer("Is leader connected? ${link.leaderConnected}, follower offset from leader: $_followerOffsetFromLeader");
FtlLogs.follower.finer(
() => "Is leader connected? ${link.leaderConnected}, follower offset from leader: $_followerOffsetFromLeader");

if (link.leaderConnected) {
FtlLogs.follower.finer("Calculating follower offset");
_calculateFollowerOffset();
}
FtlLogs.follower.fine("Final follower offset relative to leader: $_followerOffsetFromLeader");
FtlLogs.follower.fine(() => "Final follower offset relative to leader: $_followerOffsetFromLeader");

if (layer == null) {
FtlLogs.follower.finer("Creating new FollowerLayer");
Expand Down Expand Up @@ -734,11 +735,11 @@ class RenderFollower extends RenderProxyBox {
Offset(globalLeaderTopLeftVec.x, globalLeaderTopLeftVec.y),
Offset(globalLeaderBottomRightVec.x, globalLeaderBottomRightVec.y),
);
FtlLogs.follower.finer(" - Global leader rect: $globalLeaderRect");
FtlLogs.follower.finer(() => " - Global leader rect: $globalLeaderRect");

final Size? leaderSize = link.leaderSize;
FtlLogs.follower.finer(" - Leader size: $leaderSize");
FtlLogs.follower.finer(" - Leader layer offset: ${link.leader!.offset}");
FtlLogs.follower.finer(() => " - Leader size: $leaderSize");
FtlLogs.follower.finer(() => " - Leader layer offset: ${link.leader!.offset}");

final followerAlignment = aligner.align(globalLeaderRect, child!.size);
final leaderAnchor = followerAlignment.leaderAnchor;
Expand All @@ -750,15 +751,15 @@ class RenderFollower extends RenderProxyBox {
final followerScale =
(followerTransform.transform3(Vector3(1, 0, 0)) - followerTransform.transform3(Vector3.zero())).x;
final followerSize = child!.size * followerScale;
FtlLogs.follower.finer(" - Follower size: $followerSize ($followerScale scale)");
FtlLogs.follower.finer(() => " - Follower size: $followerSize ($followerScale scale)");

final followerOffsetRelativeToLeader = (leaderSize == null
? Offset.zero
: leaderAnchor.alongSize(leaderSize) - followerAnchor.alongSize(followerSize)) +
followerAlignment.followerOffset;

_followerOffsetFromLeader = followerOffsetRelativeToLeader;
FtlLogs.follower.finer(" - (Non-constrained) Follower offset relative to leader: $_followerOffsetFromLeader");
FtlLogs.follower.finer(() => " - (Non-constrained) Follower offset relative to leader: $_followerOffsetFromLeader");
}

// This is what's used by localToGlobal() and globalToLocal()
Expand All @@ -775,13 +776,13 @@ class RenderFollower extends RenderProxyBox {
/// [Matrix4.identity].
Matrix4 _getCurrentTransform() {
FtlLogs.follower.finest("RenderFollower - getCurrentTransform()");
FtlLogs.follower
.finest(" - has FollowerLayer? ${layer != null}, has existing transform? ${layer?.getLastTransform() != null}");
FtlLogs.follower
.finest(" - follower origin in screen-space (according to localToGlobal): ${localToGlobal(Offset.zero)}");
FtlLogs.follower.finest(
() => " - has FollowerLayer? ${layer != null}, has existing transform? ${layer?.getLastTransform() != null}");
FtlLogs.follower
.finest(() => " - follower origin in screen-space (according to localToGlobal): ${localToGlobal(Offset.zero)}");
FtlLogs.follower.finest(() =>
" - delta from follower content to follower origin (according to FollowerLayer): ${layer?._transformOffset(Offset.zero)}");
FtlLogs.follower.finest(" - follower offset from leader: $_followerOffsetFromLeader");
FtlLogs.follower.finest(() => " - follower offset from leader: $_followerOffsetFromLeader");
final transform = layer?.getLastTransform() ?? Matrix4.identity();

if (_followerOffsetFromLeader != null) {
Expand Down Expand Up @@ -1041,7 +1042,7 @@ class FollowerLayer extends ContainerLayer {
}

FtlLogs.follower.finest("Establishing FollowerLayer transform");
FtlLogs.follower.finest(" - follower linked offset: $linkedOffset");
FtlLogs.follower.finest(() => " - follower linked offset: $linkedOffset");
final previousTransform = _lastTransform;
_lastTransform = null;
final LeaderLayer? leader = _leaderHandle!.leader;
Expand Down Expand Up @@ -1070,7 +1071,7 @@ class FollowerLayer extends ContainerLayer {
);
FtlLogs.follower.finest(" - Leader ancestor path:");
for (final layer in leaderToAncestorLayers) {
FtlLogs.follower.finest(" - $layer");
FtlLogs.follower.finest(() => " - $layer");
}

_pathToRoot(
Expand All @@ -1079,7 +1080,7 @@ class FollowerLayer extends ContainerLayer {
);
FtlLogs.follower.finest(" - Follower ancestor path");
for (final layer in followerToAncestorLayers) {
FtlLogs.follower.finest(" - $layer");
FtlLogs.follower.finest(() => " - $layer");
}

final Matrix4 leaderTransform = _collectTransformForLayerChain(leaderToAncestorLayers);
Expand All @@ -1089,24 +1090,24 @@ class FollowerLayer extends ContainerLayer {
// up above gets us to the top-left of the LeaderLayer, but we want
// leaderTransform to get us to the top-left of the content inside the LeaderLayer.
leader.applyTransform(null, leaderTransform);
FtlLogs.follower.finest(" - Leader transform to screen-space \n$leaderTransform");
FtlLogs.follower.finest(() => " - Leader transform to screen-space \n$leaderTransform");

final Matrix4 screenToFollowerTransform = _collectTransformForLayerChain(followerToAncestorLayers);
if (screenToFollowerTransform.invert() == 0.0) {
// We are in a degenerate transform, so there's not much we can do.
return;
}
FtlLogs.follower.finest(" - Follower transform to screen-space \n$screenToFollowerTransform");
FtlLogs.follower.finest(() => " - Follower transform to screen-space \n$screenToFollowerTransform");

// Calculate the leader and follower scale so that we can un-apply the
// leader scale, and add the follower scale. We do this because we don't
// want to force the follower to always be the scale of the leader.
final leaderScale = (leaderTransform.transform3(Vector3(1, 0, 0)) - leaderTransform.transform3(Vector3.zero())).x;
FtlLogs.follower.finest(" - Leader scale: $leaderScale");
FtlLogs.follower.finest(() => " - Leader scale: $leaderScale");
final followerScale = 1 /
(screenToFollowerTransform.transform3(Vector3(1, 0, 0)) - screenToFollowerTransform.transform3(Vector3.zero()))
.x; // We invert the scale because the transform is an inverse
FtlLogs.follower.finest(" - Follower scale: $followerScale");
FtlLogs.follower.finest(() => " - Follower scale: $followerScale");

// Put follower transform into leader space. This operation would be all
// we need, if we didn't want to undo the leader's scale factor.
Expand All @@ -1116,10 +1117,10 @@ class FollowerLayer extends ContainerLayer {
// final followerOffset = Offset(followerOffsetVector.x, followerOffsetVector.y);
final leaderOffsetVector = leaderTransform.transform3(Vector3.zero());
final leaderOffset = Offset(leaderOffsetVector.x, leaderOffsetVector.y);
FtlLogs.follower.finest(" - Leader origin in screen space: $leaderOffset");
FtlLogs.follower.finest(() => " - Leader origin in screen space: $leaderOffset");

final leaderSize = _link!.leaderSize! * leaderScale;
FtlLogs.follower.finest(" - leader size: $leaderSize");
FtlLogs.follower.finest(() => " - leader size: $leaderSize");

final anchorMetrics = _calculateAlignerAnchorMetrics(
leaderSize: leaderSize,
Expand Down Expand Up @@ -1224,7 +1225,7 @@ class FollowerLayer extends ContainerLayer {
// Apply each layer to the matrix in turn, starting from the last layer,
// and providing the previous layer as the child.
for (int index = layers.length - 1; index > 0; index -= 1) {
FtlLogs.follower.finest("Calling applyTransform() on layer: ${layers[index]}");
FtlLogs.follower.finest(() => "Calling applyTransform() on layer: ${layers[index]}");
layers[index]?.applyTransform(layers[index - 1], result);
}
return result;
Expand Down
26 changes: 13 additions & 13 deletions lib/src/leader.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,27 +137,27 @@ class RenderLeader extends RenderProxyBox {

@override
void performLayout() {
FtlLogs.leader.finer("Laying out Leader - $hashCode");
FtlLogs.leader.finer(() => "Laying out Leader - $hashCode");
super.performLayout();
FtlLogs.leader.finer(" - leader size: $size");
FtlLogs.leader.finer(() => " - leader size: $size");
_previousLayoutSize = size;
link.leaderSize = size;
}

@override
void paint(PaintingContext context, Offset offset) {
FtlLogs.leader.finer("Painting Leader ($hashCode)");
FtlLogs.leader.finer(() => "Painting Leader ($hashCode)");

final globalOffset = localToGlobal(Offset.zero);
final scale = (localToGlobal(const Offset(1, 0)) - localToGlobal(Offset.zero)).dx;
final halfChildSize = child != null ? Offset(child!.size.width / 2, child!.size.height / 2) : Offset.zero;
final scaledOffset = offset + halfChildSize - (halfChildSize * scale);
FtlLogs.leader.finer(" - paint offset: $offset");
FtlLogs.leader.finer(" - child: $child");
FtlLogs.leader.finer(" - scaled paint offset: $scaledOffset");
FtlLogs.leader.finer(" - global offset: $globalOffset");
FtlLogs.leader.finer(" - follower content size (unscaled): ${child?.size}");
FtlLogs.leader.finer(" - scale: $scale");
FtlLogs.leader.finer(() => " - paint offset: $offset");
FtlLogs.leader.finer(() => " - child: $child");
FtlLogs.leader.finer(() => " - scaled paint offset: $scaledOffset");
FtlLogs.leader.finer(() => " - global offset: $globalOffset");
FtlLogs.leader.finer(() => " - follower content size (unscaled): ${child?.size}");
FtlLogs.leader.finer(() => " - scale: $scale");

final leaderToScreenTransform = getTransformTo(null);

Expand Down Expand Up @@ -185,7 +185,7 @@ class RenderLeader extends RenderProxyBox {
}

context.pushLayer(layer!, (paintContext, offset) {
FtlLogs.leader.finer("Painting leader content within LeaderLayer. Paint offset: $offset");
FtlLogs.leader.finer(() => "Painting leader content within LeaderLayer. Paint offset: $offset");
super.paint(paintContext, offset);
}, Offset.zero);
assert(layer != null);
Expand Down Expand Up @@ -255,15 +255,15 @@ class LeaderLayer extends ContainerLayer {

@override
void attach(Object owner) {
FtlLogs.leader.finer("Attaching LeaderLayer to owner: $owner");
FtlLogs.leader.finer(() => "Attaching LeaderLayer to owner: $owner");
super.attach(owner);
_lastOffset = null;
link.leader = this;
}

@override
void detach() {
FtlLogs.leader.finer("Detaching LeaderLayer from owner");
FtlLogs.leader.finer(() => "Detaching LeaderLayer from owner");
link.leader = null;
_lastOffset = null;
super.detach();
Expand All @@ -284,7 +284,7 @@ class LeaderLayer extends ContainerLayer {

@override
void addToScene(ui.SceneBuilder builder) {
FtlLogs.leader.finer("Adding LeaderLayer to scene. Offset: $offset");
FtlLogs.leader.finer(() => "Adding LeaderLayer to scene. Offset: $offset");
_lastOffset = offset;
if (offset != Offset.zero) {
engineLayer = builder.pushTransform(
Expand Down
Loading