Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Trace calls to Canvas::saveLayer #29444

Merged
merged 2 commits into from
Nov 5, 2021
Merged

Trace calls to Canvas::saveLayer #29444

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
49760a8
Trace calls to Canvas::saveLayer
dnfield Nov 1, 2021
d90de5d
Merge remote-tracking branch 'upstream/master' into trace_save_layer
dnfield Nov 5, 2021
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
2 changes: 1 addition & 1 deletion flow/compositor_context.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ RasterStatus CompositorContext::ScopedFrame::Raster(
}

if (needs_save_layer) {
FML_LOG(INFO) << "Using SaveLayer to protect non-readback surface";
TRACE_EVENT0("flutter", "Canvas::saveLayer");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cost of the save layer is not just isolated to this scope. There will be significant time taken in the flush waiting on the GPU to service this workload. So, perhaps a more actionable metric is to have a counter that tracks the number of save layers?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TRACE_COUNTER instead of TRACE_EVENT I mean.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Counters won't show up in systracing. My idea here is that devtools could manually count these, and developers would be able to more easily see what callstacks were resulting in them. Then we'd want something like debugging the SKP that resulted and you could visually see what parts of the draw used a savelayer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(so yes, these trace events will be very small, but devtools will be able to scan for them and know they result in more expensive operations later)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not just that the bulk of the work happens in a different phase of the pipeline (i.e. flush vs LayerTree.Paint), it's that even the saveLayer call itself doesn't really do the work. It remembers a few items you told it, then later the restore applies that work. So there are many reasons why this timed event isn't capturing any real work.

Why doesn't devtools scan for the counter events? They are just another type of event in the Chrome Tracing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why doesn't devtools scan for the counter events?

Discussing this in person, it looks like devtools should be able to detect counter events. systrace is an issue though and I am investigating if counters are not supported by systrace or whether this is a limitation with our trace logger to systrace. In any case, it looks like our use case ought to be address by just tracing to the timeline.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it would be great to switch this to a counter as these events are adding noise to the trace right now without clear user value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We didn't use counters because it wouldn't be seen by users. The action a user should take is to turn on debugTracePaintEnabled and remove or rewrite the render object adding this call.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... without clear user value.

I don't remember the full context of this discussion---if these are trace events we explicitly added for the benefit of end-users, then we should certainly try to make them more useful---but in general, it's been our practice to add trace events as needed to assist Engine developers in zeroing in on issues, be they performance issues or otherwise. So (in general, maybe not for this specific case) if a trace event is too noisy, the corrective action would be to filter it before an end-user sees it.

SkRect bounds = SkRect::Make(layer_tree.frame_size());
SkPaint paint;
paint.setBlendMode(SkBlendMode::kSrc);
Expand Down
1 change: 1 addition & 0 deletions flow/layers/clip_path_layer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ void ClipPathLayer::Paint(PaintContext& context) const {
clip_behavior_ != Clip::hardEdge);

if (UsesSaveLayer()) {
TRACE_EVENT0("flutter", "Canvas::saveLayer");
context.internal_nodes_canvas->saveLayer(paint_bounds(), nullptr);
}
PaintChildren(context);
Expand Down
1 change: 1 addition & 0 deletions flow/layers/clip_rect_layer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ void ClipRectLayer::Paint(PaintContext& context) const {
clip_behavior_ != Clip::hardEdge);

if (UsesSaveLayer()) {
TRACE_EVENT0("flutter", "Canvas::saveLayer");
context.internal_nodes_canvas->saveLayer(clip_rect_, nullptr);
}
PaintChildren(context);
Expand Down
1 change: 1 addition & 0 deletions flow/layers/clip_rrect_layer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ void ClipRRectLayer::Paint(PaintContext& context) const {
clip_behavior_ != Clip::hardEdge);

if (UsesSaveLayer()) {
TRACE_EVENT0("flutter", "Canvas::saveLayer");
context.internal_nodes_canvas->saveLayer(paint_bounds(), nullptr);
}
PaintChildren(context);
Expand Down
5 changes: 3 additions & 2 deletions flow/layers/physical_shape_layer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,10 +97,11 @@ void PhysicalShapeLayer::Paint(PaintContext& context) const {
case Clip::antiAlias:
context.internal_nodes_canvas->clipPath(path_, true);
break;
case Clip::antiAliasWithSaveLayer:
case Clip::antiAliasWithSaveLayer: {
TRACE_EVENT0("flutter", "Canvas::saveLayer");
context.internal_nodes_canvas->clipPath(path_, true);
context.internal_nodes_canvas->saveLayer(paint_bounds(), nullptr);
break;
} break;
case Clip::none:
break;
}
Expand Down
2 changes: 2 additions & 0 deletions lib/ui/painting/canvas.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@ void Canvas::saveLayerWithoutBounds(const Paint& paint,
if (!canvas_) {
return;
}
TRACE_EVENT0("flutter", "Canvas::saveLayer");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is incorrect. This line is only recording a saveLayer and has nothing to do with the execution of saveLayers.

The recorded picture can be reused on every single frame from here on and it won't be recorded (there's not much that can be done for that with SkP, but DL could record them as they execute).

The recorded picture could be a mistake and never used even once.

This trace will appear on the build thread, not the raster thread.

There is everything wrong with this event.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know that this record won't be enough, but in most cases it will provide signal about where in developer code someone actually triggered a saveLayer. My more recent concern is that it might not be enough, since things like drawColor and drawPaint might also trigger a saveLayer.

How would you suggest achieving this?

canvas_->saveLayer(nullptr, paint.paint());
}

Expand All @@ -117,6 +118,7 @@ void Canvas::saveLayer(double left,
if (!canvas_) {
return;
}
TRACE_EVENT0("flutter", "Canvas::saveLayer");
SkRect bounds = SkRect::MakeLTRB(left, top, right, bottom);
canvas_->saveLayer(&bounds, paint.paint());
}
Expand Down
5 changes: 4 additions & 1 deletion testing/dart/observatory/BUILD.gn
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,10 @@

import("//flutter/testing/dart/compile_test.gni")

tests = [ "skp_test.dart" ]
tests = [
"skp_test.dart",
"tracing_test.dart",
]

foreach(test, tests) {
compile_flutter_dart_test("compile_$test") {
Expand Down
59 changes: 59 additions & 0 deletions testing/dart/observatory/tracing_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
// Copyright 2013 The Flutter Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

import 'dart:async';
import 'dart:developer' as developer;
import 'dart:ui';

import 'package:litetest/litetest.dart';
import 'package:vm_service/vm_service.dart' as vms;
import 'package:vm_service/vm_service_io.dart';

void main() {
test('Canvas.saveLayer emits tracing', () async {
final developer.ServiceProtocolInfo info = await developer.Service.getInfo();

if (info.serverUri == null) {
fail('This test must not be run with --disable-observatory.');
}

final vms.VmService vmService = await vmServiceConnectUri(
'ws://localhost:${info.serverUri!.port}${info.serverUri!.path}ws',
);

final Completer<void> completer = Completer<void>();
window.onBeginFrame = (Duration timeStamp) {
final PictureRecorder recorder = PictureRecorder();
final Canvas canvas = Canvas(recorder);
canvas.saveLayer(null, Paint());
canvas.saveLayer(const Rect.fromLTWH(0, 0, 100, 100), Paint());
canvas.drawRect(const Rect.fromLTRB(10, 10, 20, 20), Paint());
canvas.restore();
canvas.restore();
final Picture picture = recorder.endRecording();

final SceneBuilder builder = SceneBuilder();
builder.addPicture(Offset.zero, picture);
final Scene scene = builder.build();

window.render(scene);
scene.dispose();
completer.complete();
};
window.scheduleFrame();
await completer.future;

final vms.Timeline timeline = await vmService.getVMTimeline();
await vmService.dispose();

int saveLayerCount = 0;
for (final vms.TimelineEvent event in timeline.traceEvents!) {
final Map<String, dynamic> json = event.json!;
if (json['name'] == 'Canvas::saveLayer' && json['ph'] == 'B') {
saveLayerCount += 1;
}
}
expect(saveLayerCount, 2);
});
}